From fadd9a1e2d2ab1d63bd05c30a0d845e837deb9be Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 24 Nov 2016 11:14:56 +0000 Subject: [PATCH] Verify that extensions are used in the correct context Perl changes reviewed by Richard Levitte. Non-perl changes reviewed by Rich Salz Reviewed-by: Rich Salz Reviewed-by: Richard Levitte --- include/openssl/ssl.h | 1 + ssl/ssl_err.c | 1 + ssl/statem/statem_extensions.c | 145 ++++++++++++++++++++++++++++++++- ssl/statem/statem_locl.h | 15 +++- ssl/statem/statem_srvr.c | 5 +- 5 files changed, 159 insertions(+), 8 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 7e7c3b1971..4c306ac8bf 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2324,6 +2324,7 @@ int ERR_load_SSL_strings(void); # define SSL_R_BAD_DIGEST_LENGTH 111 # define SSL_R_BAD_ECC_CERT 304 # define SSL_R_BAD_ECPOINT 306 +# define SSL_R_BAD_EXTENSION 110 # define SSL_R_BAD_HANDSHAKE_LENGTH 332 # define SSL_R_BAD_HELLO_REQUEST 105 # define SSL_R_BAD_KEY_SHARE 108 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index 14a44a5278..68856eec48 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -354,6 +354,7 @@ static ERR_STRING_DATA SSL_str_reasons[] = { {ERR_REASON(SSL_R_BAD_DIGEST_LENGTH), "bad digest length"}, {ERR_REASON(SSL_R_BAD_ECC_CERT), "bad ecc cert"}, {ERR_REASON(SSL_R_BAD_ECPOINT), "bad ecpoint"}, + {ERR_REASON(SSL_R_BAD_EXTENSION), "bad extension"}, {ERR_REASON(SSL_R_BAD_HANDSHAKE_LENGTH), "bad handshake length"}, {ERR_REASON(SSL_R_BAD_HELLO_REQUEST), "bad hello request"}, {ERR_REASON(SSL_R_BAD_KEY_SHARE), "bad key share"}, diff --git a/ssl/statem/statem_extensions.c b/ssl/statem/statem_extensions.c index 78526bb687..21ce34ce71 100644 --- a/ssl/statem/statem_extensions.c +++ b/ssl/statem/statem_extensions.c @@ -11,6 +11,107 @@ #include "../ssl_locl.h" #include "statem_locl.h" +typedef struct { + /* The ID for the extension */ + unsigned int type; + int (*server_parse)(SSL *s, PACKET *pkt); + int (*client_parse)(SSL *s, PACKET *pkt); + unsigned int context; +} EXTENSION_DEFINITION; + + +static const EXTENSION_DEFINITION ext_defs[] = { + { + TLSEXT_TYPE_renegotiate, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + }, + { + TLSEXT_TYPE_server_name, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + | EXT_TLS1_3_ENCRYPTED_EXTENSIONS + }, + { + TLSEXT_TYPE_srp, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + }, + { + TLSEXT_TYPE_ec_point_formats, + NULL, NULL, + EXT_CLIENT_HELLO + }, + { + TLSEXT_TYPE_supported_groups, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_3_ENCRYPTED_EXTENSIONS + }, + { + TLSEXT_TYPE_session_ticket, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + }, + { + TLSEXT_TYPE_signature_algorithms, + NULL, NULL, + EXT_CLIENT_HELLO + }, + { + TLSEXT_TYPE_status_request, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_3_CERTIFICATE + }, + { + TLSEXT_TYPE_next_proto_neg, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + }, + { + TLSEXT_TYPE_application_layer_protocol_negotiation, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + | EXT_TLS1_3_ENCRYPTED_EXTENSIONS + }, + { + TLSEXT_TYPE_use_srtp, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + | EXT_TLS1_3_ENCRYPTED_EXTENSIONS | EXT_DTLS_ONLY + }, + { + TLSEXT_TYPE_encrypt_then_mac, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + }, + { + TLSEXT_TYPE_signed_certificate_timestamp, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_3_CERTIFICATE + }, + { + TLSEXT_TYPE_extended_master_secret, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO + }, + { + TLSEXT_TYPE_supported_versions, + NULL, NULL, + EXT_CLIENT_HELLO + }, + { + TLSEXT_TYPE_padding, + NULL, NULL, + EXT_CLIENT_HELLO + }, + { + TLSEXT_TYPE_key_share, + NULL, NULL, + EXT_CLIENT_HELLO | EXT_TLS1_3_SERVER_HELLO + | EXT_TLS1_3_HELLO_RETRY_REQUEST + } +}; + /* * Comparison function used in a call to qsort (see tls_collect_extensions() * below.) @@ -35,8 +136,37 @@ static int compare_extensions(const void *p1, const void *p2) } /* - * Gather a list of all the extensions. We don't actually process the content - * of the extensions yet, except to check their types. + * Verify whether we are allowed to use the extension |type| in the current + * |context|. Returns 1 to indicate the extension is allowed or unknown or 0 to + * indicate the extension is not allowed. + */ +static int verify_extension(SSL *s, unsigned int context, unsigned int type) +{ + size_t i; + + for (i = 0; i < OSSL_NELEM(ext_defs); i++) { + if (type == ext_defs[i].type) { + /* Check we're allowed to use this extension in this context */ + if ((context & ext_defs[i].context) == 0) + return 0; + /* Make sure we don't use DTLS extensions in TLS */ + if ((ext_defs[i].context & EXT_DTLS_ONLY) && !SSL_IS_DTLS(s)) + return 0; + + return 1; + } + } + + /* Unknown extension. We allow it */ + return 1; +} + +/* + * Gather a list of all the extensions from the data in |packet]. |context| + * tells us which message this extension is for. The raw extension data is + * stored in |*res| with the number of found extensions in |*numfound|. In the + * event of an error the alert type to use is stored in |*ad|. We don't actually + * process the content of the extensions yet, except to check their types. * * Per http://tools.ietf.org/html/rfc5246#section-7.4.1.4, there may not be * more than one extension of the same type in a ClientHello or ServerHello. @@ -48,8 +178,8 @@ static int compare_extensions(const void *p1, const void *p2) * TODO(TLS1.3): Refactor ServerHello extension parsing to use this and then * remove tls1_check_duplicate_extensions() */ -int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res, - size_t *numfound, int *ad) +int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context, + RAW_EXTENSION **res, size_t *numfound, int *ad) { PACKET extensions = *packet; size_t num_extensions = 0, i = 0; @@ -62,9 +192,16 @@ int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res, if (!PACKET_get_net_2(&extensions, &type) || !PACKET_get_length_prefixed_2(&extensions, &extension)) { + SSLerr(SSL_F_TLS_COLLECT_EXTENSIONS, SSL_R_BAD_EXTENSION); *ad = SSL_AD_DECODE_ERROR; goto err; } + /* Verify this extension is allowed */ + if (!verify_extension(s, context, type)) { + SSLerr(SSL_F_TLS_COLLECT_EXTENSIONS, SSL_R_BAD_EXTENSION); + *ad = SSL_AD_ILLEGAL_PARAMETER; + goto err; + } num_extensions++; } diff --git a/ssl/statem/statem_locl.h b/ssl/statem/statem_locl.h index 32f55cca43..dd815c7174 100644 --- a/ssl/statem/statem_locl.h +++ b/ssl/statem/statem_locl.h @@ -26,6 +26,17 @@ /* Max should actually be 36 but we are generous */ #define FINISHED_MAX_LENGTH 64 +/* Extension context codes */ +#define EXT_DTLS_ONLY 0x01 +#define EXT_CLIENT_HELLO 0x02 +/* Really means TLS1.2 or below */ +#define EXT_TLS1_2_SERVER_HELLO 0x04 +#define EXT_TLS1_3_SERVER_HELLO 0x08 +#define EXT_TLS1_3_ENCRYPTED_EXTENSIONS 0x10 +#define EXT_TLS1_3_HELLO_RETRY_REQUEST 0x20 +#define EXT_TLS1_3_CERTIFICATE 0x40 +#define EXT_TLS1_3_NEW_SESSION_TICKET 0x80 + /* Message processing return codes */ typedef enum { /* Something bad happened */ @@ -88,8 +99,8 @@ __owur int tls_construct_finished(SSL *s, WPACKET *pkt); __owur WORK_STATE tls_finish_handshake(SSL *s, WORK_STATE wst); __owur WORK_STATE dtls_wait_for_dry(SSL *s); -int tls_collect_extensions(PACKET *packet, RAW_EXTENSION **res, - size_t *numfound, int *ad); +int tls_collect_extensions(SSL *s, PACKET *packet, unsigned int context, + RAW_EXTENSION **res, size_t *numfound, int *ad); /* some client-only functions */ __owur int tls_construct_client_hello(SSL *s, WPACKET *pkt); diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 50caa42951..d3491060a9 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1249,8 +1249,9 @@ MSG_PROCESS_RETURN tls_process_client_hello(SSL *s, PACKET *pkt) /* Preserve the raw extensions PACKET for later use */ extensions = clienthello.extensions; - if (!tls_collect_extensions(&extensions, &clienthello.pre_proc_exts, - &clienthello.num_extensions, &al)) { + if (!tls_collect_extensions(s, &extensions, EXT_CLIENT_HELLO, + &clienthello.pre_proc_exts, + &clienthello.num_extensions, &al)) { /* SSLerr already been called */ goto f_err; } -- 2.25.1