diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 36d17dd224f21bc6effb4ad66bc6551f759af3af..9709103d5fb984dda3003a7e0cd77460b1ceb2d6 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2123,6 +2123,7 @@ void ERR_load_SSL_strings(void); # define SSL_F_STATE_MACHINE 353 # define SSL_F_TLS12_CHECK_PEER_SIGALG 333 # define SSL_F_TLS1_CHANGE_CIPHER_STATE 209 +# define SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS 341 # define SSL_F_TLS1_CHECK_SERVERHELLO_TLSEXT 274 # define SSL_F_TLS1_EXPORT_KEYING_MATERIAL 314 # define SSL_F_TLS1_GET_CURVELIST 338 diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c index e6b9bbdb9cc813c7e9cfe447cdd8d441088aaece..46f483febe29b1270efcb248761fd4253c9f3581 100644 --- a/ssl/ssl_err.c +++ b/ssl/ssl_err.c @@ -273,6 +273,8 @@ static ERR_STRING_DATA SSL_str_functs[] = { {ERR_FUNC(SSL_F_STATE_MACHINE), "state_machine"}, {ERR_FUNC(SSL_F_TLS12_CHECK_PEER_SIGALG), "tls12_check_peer_sigalg"}, {ERR_FUNC(SSL_F_TLS1_CHANGE_CIPHER_STATE), "tls1_change_cipher_state"}, + {ERR_FUNC(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS), + "tls1_check_duplicate_extensions"}, {ERR_FUNC(SSL_F_TLS1_CHECK_SERVERHELLO_TLSEXT), "TLS1_CHECK_SERVERHELLO_TLSEXT"}, {ERR_FUNC(SSL_F_TLS1_EXPORT_KEYING_MATERIAL), diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 7a2047dcca1e5c7d676fc7b9238f2ff5fda79d57..db5f0f6b442a86b535a79276fccf491a8df8e410 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -109,6 +109,7 @@ */ #include +#include #include #include #include @@ -124,7 +125,7 @@ static int tls_decrypt_ticket(SSL *s, const unsigned char *tick, int ticklen, const unsigned char *sess_id, int sesslen, SSL_SESSION **psess); static int ssl_check_clienthello_tlsext_early(SSL *s); -int ssl_check_serverhello_tlsext(SSL *s); +static int ssl_check_serverhello_tlsext(SSL *s); SSL3_ENC_METHOD const TLSv1_enc_data = { tls1_enc, @@ -1037,6 +1038,79 @@ static int tls_use_ticket(SSL *s) return ssl_security(s, SSL_SECOP_TICKET, 0, 0, NULL); } +static int compare_uint(const void *p1, const void *p2) { + unsigned int u1 = *((const unsigned int *)p1); + unsigned int u2 = *((const unsigned int *)p2); + if (u1 < u2) + return -1; + else if (u1 > u2) + return 1; + else + return 0; +} + +/* + * 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. + * This function does an initial scan over the extensions block to filter those + * out. It returns 1 if all extensions are unique, and 0 if the extensions + * contain duplicates, could not be successfully parsed, or an internal error + * occurred. + */ +static int tls1_check_duplicate_extensions(const PACKET *packet) { + PACKET extensions = *packet; + size_t num_extensions = 0, i = 0; + unsigned int *extension_types = NULL; + int ret = 0; + + /* First pass: count the extensions. */ + while (PACKET_remaining(&extensions) > 0) { + unsigned int type; + PACKET extension; + if (!PACKET_get_net_2(&extensions, &type) || + !PACKET_get_length_prefixed_2(&extensions, &extension)) { + goto done; + } + num_extensions++; + } + + if (num_extensions <= 1) + return 1; + + extension_types = OPENSSL_malloc(sizeof(unsigned int) * num_extensions); + if (extension_types == NULL) { + SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_MALLOC_FAILURE); + goto done; + } + + /* Second pass: gather the extension types. */ + extensions = *packet; + for (i = 0; i < num_extensions; i++) { + PACKET extension; + if (!PACKET_get_net_2(&extensions, &extension_types[i]) || + !PACKET_get_length_prefixed_2(&extensions, &extension)) { + /* This should not happen. */ + SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_INTERNAL_ERROR); + goto done; + } + } + + if (PACKET_remaining(&extensions) != 0) { + SSLerr(SSL_F_TLS1_CHECK_DUPLICATE_EXTENSIONS, ERR_R_INTERNAL_ERROR); + goto done; + } + /* Sort the extensions and make sure there are no duplicates. */ + qsort(extension_types, num_extensions, sizeof(unsigned int), compare_uint); + for (i = 1; i < num_extensions; i++) { + if (extension_types[i - 1] == extension_types[i]) + goto done; + } + ret = 1; + done: + OPENSSL_free(extension_types); + return ret; +} + unsigned char *ssl_add_clienthello_tlsext(SSL *s, unsigned char *buf, unsigned char *limit, int *al) { @@ -1837,6 +1911,9 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al) if (PACKET_remaining(pkt) != len) goto err; + if (!tls1_check_duplicate_extensions(pkt)) + goto err; + while (PACKET_get_net_2(pkt, &type) && PACKET_get_net_2(pkt, &size)) { PACKET subpkt; @@ -2301,6 +2378,11 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al) return 0; } + if (!tls1_check_duplicate_extensions(pkt)) { + *al = SSL_AD_DECODE_ERROR; + return 0; + } + while (PACKET_get_net_2(pkt, &type) && PACKET_get_net_2(pkt, &size)) { const unsigned char *data; PACKET spkt; diff --git a/test/recipes/70-test_sslcertstatus.t b/test/recipes/70-test_sslcertstatus.t index 9a0c5f8eac406535b350c0f808eba6e6c861e299..303de5e56635cfec038868c7f711c24766723862 100755 --- a/test/recipes/70-test_sslcertstatus.t +++ b/test/recipes/70-test_sslcertstatus.t @@ -99,7 +99,7 @@ sub certstatus_filter if ($message->mt == TLSProxy::Message::MT_SERVER_HELLO) { #Add the status_request to the ServerHello even though we are not #going to send a CertificateStatus message - $message->set_extension(TLSProxy::ClientHello::EXT_STATUS_REQUEST, + $message->set_extension(TLSProxy::Message::EXT_STATUS_REQUEST, ""); $message->repack(); diff --git a/test/recipes/70-test_sslextension.t b/test/recipes/70-test_sslextension.t index 4582c5c72087aa13815812192b9336f90906a5c7..c253f748e0d84dc4d50877336ec19a53f83fc2c6 100755 --- a/test/recipes/70-test_sslextension.t +++ b/test/recipes/70-test_sslextension.t @@ -78,9 +78,9 @@ my $proxy = TLSProxy::Proxy->new( (!$ENV{HARNESS_ACTIVE} || $ENV{HARNESS_VERBOSE}) ); -plan tests => 1; +plan tests => 3; -#Test 1: Sending a zero length extension block should pass +# Test 1: Sending a zero length extension block should pass $proxy->start(); ok(TLSProxy::Message->success, "Zero extension length test"); @@ -95,13 +95,64 @@ sub extension_filter foreach my $message (@{$proxy->message_list}) { if ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) { - #Remove all extensions and set the extension len to zero + # Remove all extensions and set the extension len to zero $message->extension_data({}); $message->extensions_len(0); - #Extensions have been removed so make sure we don't try to use them + # Extensions have been removed so make sure we don't try to use them $message->process_extensions(); $message->repack(); } } } + +# Test 2-3: Sending a duplicate extension should fail. +sub inject_duplicate_extension +{ + my ($proxy, $message_type) = @_; + + foreach my $message (@{$proxy->message_list}) { + if ($message->mt == $message_type) { + my %extensions = %{$message->extension_data}; + # Add a duplicate (unknown) extension. + $message->set_extension(TLSProxy::Message::EXT_DUPLICATE_EXTENSION, ""); + $message->set_extension(TLSProxy::Message::EXT_DUPLICATE_EXTENSION, ""); + $message->repack(); + } + } +} + +sub inject_duplicate_extension_clienthello +{ + my $proxy = shift; + + # We're only interested in the initial ClientHello + if ($proxy->flight != 0) { + return; + } + + inject_duplicate_extension($proxy, TLSProxy::Message::MT_CLIENT_HELLO); +} + +sub inject_duplicate_extension_serverhello +{ + my $proxy = shift; + + # We're only interested in the initial ServerHello + if ($proxy->flight != 1) { + return; + } + + inject_duplicate_extension($proxy, TLSProxy::Message::MT_SERVER_HELLO); +} + +$proxy->clear(); +$proxy->filter(\&inject_duplicate_extension_clienthello); +$proxy->start(); +ok(TLSProxy::Message->fail(), "Duplicate ClientHello extension"); + +$proxy->clear(); +$proxy->filter(\&inject_duplicate_extension_serverhello); +$proxy->start(); +ok(TLSProxy::Message->fail(), "Duplicate ServerHello extension"); + diff --git a/test/recipes/70-test_sslsessiontick.t b/test/recipes/70-test_sslsessiontick.t index 8a361fd80387e4f119ccc6ae7c0edb00b3196a23..4e9c85f82fc47cc5b43a388a86f99b2cc71cc950 100755 --- a/test/recipes/70-test_sslsessiontick.t +++ b/test/recipes/70-test_sslsessiontick.t @@ -198,7 +198,7 @@ sub inject_empty_ticket_filter { foreach my $message (@{$proxy->message_list}) { push @new_message_list, $message; if ($message->mt == TLSProxy::Message::MT_SERVER_HELLO) { - $message->set_extension(TLSProxy::ClientHello::EXT_SESSION_TICKET, ""); + $message->set_extension(TLSProxy::Message::EXT_SESSION_TICKET, ""); $message->repack(); # Tack NewSessionTicket onto the ServerHello record. # This only works if the ServerHello is exactly one record. @@ -226,7 +226,7 @@ sub checkmessages($$$$$$) #Get the extensions data my %extensions = %{$message->extension_data}; if (defined - $extensions{TLSProxy::ClientHello::EXT_SESSION_TICKET}) { + $extensions{TLSProxy::Message::EXT_SESSION_TICKET}) { if ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) { $chellotickext = 1; } else { diff --git a/test/recipes/70-test_tlsextms.t b/test/recipes/70-test_tlsextms.t index 82cb85676d18151a71f7c6529fc08b1d3ffdac48..022b3a8d6c5f643b8192ef801ac2aa2123761257 100644 --- a/test/recipes/70-test_tlsextms.t +++ b/test/recipes/70-test_tlsextms.t @@ -215,11 +215,11 @@ sub extms_filter foreach my $message (@{$proxy->message_list}) { if ($crmextms && $message->mt == TLSProxy::Message::MT_CLIENT_HELLO) { - $message->delete_extension(TLSProxy::ClientHello::EXT_EXTENDED_MASTER_SECRET); + $message->delete_extension(TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET); $message->repack(); } if ($srmextms && $message->mt == TLSProxy::Message::MT_SERVER_HELLO) { - $message->delete_extension(TLSProxy::ClientHello::EXT_EXTENDED_MASTER_SECRET); + $message->delete_extension(TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET); $message->repack(); } } @@ -237,7 +237,7 @@ sub checkmessages($$$$$) #Get the extensions data my %extensions = %{$message->extension_data}; if (defined - $extensions{TLSProxy::ClientHello::EXT_EXTENDED_MASTER_SECRET}) { + $extensions{TLSProxy::Message::EXT_EXTENDED_MASTER_SECRET}) { if ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) { $cextms = 1; } else { diff --git a/util/TLSProxy/ClientHello.pm b/util/TLSProxy/ClientHello.pm index 72661129ae0f6dcc23d01f02d8320f7000e9b84f..383062842c8da49b1b50631144af097bf50b61a7 100644 --- a/util/TLSProxy/ClientHello.pm +++ b/util/TLSProxy/ClientHello.pm @@ -57,13 +57,6 @@ package TLSProxy::ClientHello; use parent 'TLSProxy::Message'; -use constant { - EXT_STATUS_REQUEST => 5, - EXT_ENCRYPT_THEN_MAC => 22, - EXT_EXTENDED_MASTER_SECRET => 23, - EXT_SESSION_TICKET => 35 -}; - sub new { my $class = shift; @@ -90,7 +83,7 @@ sub new $self->{comp_meth_len} = 0; $self->{comp_meths} = []; $self->{extensions_len} = 0; - $self->{extensions_data} = ""; + $self->{extension_data} = ""; return $self; } @@ -161,7 +154,7 @@ sub process_extensions #Clear any state from a previous run TLSProxy::Record->etm(0); - if (exists $extensions{&EXT_ENCRYPT_THEN_MAC}) { + if (exists $extensions{TLSProxy::Message::EXT_ENCRYPT_THEN_MAC}) { TLSProxy::Record->etm(1); } } @@ -187,6 +180,11 @@ sub set_message_contents $extensions .= pack("n", $key); $extensions .= pack("n", length($extdata)); $extensions .= $extdata; + if ($key == TLSProxy::Message::EXT_DUPLICATE_EXTENSION) { + $extensions .= pack("n", $key); + $extensions .= pack("n", length($extdata)); + $extensions .= $extdata; + } } $data .= pack('n', length($extensions)); @@ -276,6 +274,11 @@ sub extension_data } return $self->{extension_data}; } +sub set_extension +{ + my ($self, $ext_type, $ext_data) = @_; + $self->{extension_data}{$ext_type} = $ext_data; +} sub delete_extension { my ($self, $ext_type) = @_; diff --git a/util/TLSProxy/Message.pm b/util/TLSProxy/Message.pm index ddd0a6d3a840e4059be9fef94f0076da551e8b9a..bbb0ad70619a14038db824e001c51714c3ba293f 100644 --- a/util/TLSProxy/Message.pm +++ b/util/TLSProxy/Message.pm @@ -101,6 +101,16 @@ my %message_type = ( MT_NEXT_PROTO, "NextProto" ); +use constant { + EXT_STATUS_REQUEST => 5, + EXT_ENCRYPT_THEN_MAC => 22, + EXT_EXTENDED_MASTER_SECRET => 23, + EXT_SESSION_TICKET => 35, + # This extension does not exist and isn't recognised by OpenSSL. + # We use it to test handling of duplicate extensions. + EXT_DUPLICATE_EXTENSION => 1234 +}; + my $payload = ""; my $messlen = -1; my $mt; diff --git a/util/TLSProxy/ServerHello.pm b/util/TLSProxy/ServerHello.pm index 487538ab1286fcfe631b19f46bf663cb8b4159bf..7cf753508d1dda62473ddb8a2a0e9309fb8ff87a 100644 --- a/util/TLSProxy/ServerHello.pm +++ b/util/TLSProxy/ServerHello.pm @@ -80,7 +80,7 @@ sub new $self->{session} = ""; $self->{ciphersuite} = 0; $self->{comp_meth} = 0; - $self->{extensions_data} = ""; + $self->{extension_data} = ""; return $self; } @@ -161,6 +161,11 @@ sub set_message_contents $extensions .= pack("n", $key); $extensions .= pack("n", length($extdata)); $extensions .= $extdata; + if ($key == TLSProxy::Message::EXT_DUPLICATE_EXTENSION) { + $extensions .= pack("n", $key); + $extensions .= pack("n", length($extdata)); + $extensions .= $extdata; + } } $data .= pack('n', length($extensions));