The branch master has been updated via bb982ce7532eb5f5f8d66211d556940a7f407496 (commit) via 78fcddbb8d690c515f4525b5220fc63448ecd1a9 (commit) via a2a0c86bb0d602253d02ded2a848ed69e8cc425a (commit) via a01c86a25198921c5b8adb45c9379088ace4e42e (commit) via 44efb88a21d464dba3ac5084c8d4553d696fab33 (commit) from c35d339d98f969aa88b75124389ba86344eb7e2a (commit)
- Log ----------------------------------------------------------------- commit bb982ce7532eb5f5f8d66211d556940a7f407496 Author: Matt Caswell <m...@openssl.org> Date: Thu Aug 4 11:31:57 2016 +0100 Remove a stray unneeded line in 70-test_sslrecords.t Reviewed-by: Tim Hudson <t...@openssl.org> commit 78fcddbb8d690c515f4525b5220fc63448ecd1a9 Author: Matt Caswell <m...@openssl.org> Date: Wed Aug 3 13:03:25 2016 +0100 Address feedback on SSLv2 ClientHello processing Reviewed-by: Tim Hudson <t...@openssl.org> commit a2a0c86bb0d602253d02ded2a848ed69e8cc425a Author: Matt Caswell <m...@openssl.org> Date: Tue Aug 2 17:24:54 2016 +0100 Add some SSLv2 ClientHello tests Test that we handle a TLS ClientHello in an SSLv2 record correctly. Reviewed-by: Tim Hudson <t...@openssl.org> commit a01c86a25198921c5b8adb45c9379088ace4e42e Author: Matt Caswell <m...@openssl.org> Date: Tue Aug 2 17:43:32 2016 +0100 Send an alert if we get a non-initial record with the wrong version If we receive a non-initial record but the version number isn't right then we should send an alert. Reviewed-by: Tim Hudson <t...@openssl.org> commit 44efb88a21d464dba3ac5084c8d4553d696fab33 Author: Matt Caswell <m...@openssl.org> Date: Mon Aug 1 17:15:13 2016 +0100 Address feedback on SSLv2 ClientHello processing Feedback on the previous SSLv2 ClientHello processing fix was that it breaks layering by reading init_num in the record layer. It also does not detect if there was a previous non-fatal warning. This is an alternative approach that directly tracks in the record layer whether this is the first record. GitHub Issue #1298 Reviewed-by: Tim Hudson <t...@openssl.org> ----------------------------------------------------------------------- Summary of changes: ssl/record/rec_layer_s3.c | 1 + ssl/record/record.h | 3 + ssl/record/record_locl.h | 3 + ssl/record/ssl3_record.c | 29 +++--- test/recipes/70-test_sslrecords.t | 197 +++++++++++++++++++++++++++++++++++++- util/TLSProxy/Message.pm | 3 +- util/TLSProxy/Record.pm | 14 ++- util/TLSProxy/ServerHello.pm | 20 +++- 8 files changed, 248 insertions(+), 22 deletions(-) diff --git a/ssl/record/rec_layer_s3.c b/ssl/record/rec_layer_s3.c index 2d0fca2..c2f9666 100644 --- a/ssl/record/rec_layer_s3.c +++ b/ssl/record/rec_layer_s3.c @@ -33,6 +33,7 @@ void RECORD_LAYER_init(RECORD_LAYER *rl, SSL *s) { rl->s = s; + RECORD_LAYER_set_first_record(&s->rlayer); SSL3_RECORD_clear(rl->rrec, SSL_MAX_PIPELINES); } diff --git a/ssl/record/record.h b/ssl/record/record.h index 9177fb4..ce60a15 100644 --- a/ssl/record/record.h +++ b/ssl/record/record.h @@ -199,6 +199,9 @@ typedef struct record_layer_st { unsigned char read_sequence[SEQ_NUM_SIZE]; unsigned char write_sequence[SEQ_NUM_SIZE]; + /* Set to true if this is the first record in a connection */ + unsigned int is_first_record; + DTLS_RECORD_LAYER *d; } RECORD_LAYER; diff --git a/ssl/record/record_locl.h b/ssl/record/record_locl.h index 435e92a..f9dbc33 100644 --- a/ssl/record/record_locl.h +++ b/ssl/record/record_locl.h @@ -31,6 +31,9 @@ #define RECORD_LAYER_reset_empty_record_count(rl) \ ((rl)->empty_record_count = 0) #define RECORD_LAYER_get_empty_record_count(rl) ((rl)->empty_record_count) +#define RECORD_LAYER_is_first_record(rl) ((rl)->is_first_record) +#define RECORD_LAYER_set_first_record(rl) ((rl)->is_first_record = 1) +#define RECORD_LAYER_clear_first_record(rl) ((rl)->is_first_record = 0) #define DTLS_RECORD_LAYER_get_r_epoch(rl) ((rl)->d->r_epoch) __owur int ssl3_read_n(SSL *s, int n, int max, int extend, int clearold); diff --git a/ssl/record/ssl3_record.c b/ssl/record/ssl3_record.c index 49c6756..5f9ce7a 100644 --- a/ssl/record/ssl3_record.c +++ b/ssl/record/ssl3_record.c @@ -159,19 +159,9 @@ int ssl3_get_record(SSL *s) p = RECORD_LAYER_get_packet(&s->rlayer); /* - * Check whether this is a regular record or an SSLv2 style record. - * The latter can only be used in the first record of an initial - * ClientHello for old clients. Initial ClientHello means - * s->first_packet is set and s->server is true. The first record - * means we've not received any data so far (s->init_num == 0) and - * have had no empty records. We check s->read_hash and - * s->enc_read_ctx to ensure this does not apply during - * renegotiation. + * The first record received by the server may be a V2ClientHello. */ - if (s->first_packet && s->server - && s->init_num == 0 - && RECORD_LAYER_get_empty_record_count(&s->rlayer) == 0 - && s->read_hash == NULL && s->enc_read_ctx == NULL + if (s->server && RECORD_LAYER_is_first_record(&s->rlayer) && (p[0] & 0x80) && (p[2] == SSL2_MT_CLIENT_HELLO)) { /* * SSLv2 style record @@ -239,7 +229,7 @@ int ssl3_get_record(SSL *s) } if ((version >> 8) != SSL3_VERSION_MAJOR) { - if (s->first_packet) { + if (RECORD_LAYER_is_first_record(&s->rlayer)) { /* Go back to start of packet, look at the five bytes * that we have. */ p = RECORD_LAYER_get_packet(&s->rlayer); @@ -254,9 +244,17 @@ int ssl3_get_record(SSL *s) SSL_R_HTTPS_PROXY_REQUEST); goto err; } + + /* Doesn't look like TLS - don't send an alert */ + SSLerr(SSL_F_SSL3_GET_RECORD, + SSL_R_WRONG_VERSION_NUMBER); + goto err; + } else { + SSLerr(SSL_F_SSL3_GET_RECORD, + SSL_R_WRONG_VERSION_NUMBER); + al = SSL_AD_PROTOCOL_VERSION; + goto f_err; } - SSLerr(SSL_F_SSL3_GET_RECORD, SSL_R_WRONG_VERSION_NUMBER); - goto err; } if (rr[num_recs].length > @@ -335,6 +333,7 @@ int ssl3_get_record(SSL *s) /* we have pulled in a full packet so zero things */ RECORD_LAYER_reset_packet_length(&s->rlayer); + RECORD_LAYER_clear_first_record(&s->rlayer); } while (num_recs < max_recs && rr[num_recs-1].type == SSL3_RT_APPLICATION_DATA && SSL_USE_EXPLICIT_IV(s) diff --git a/test/recipes/70-test_sslrecords.t b/test/recipes/70-test_sslrecords.t index 0ae018a..d1c8d3a 100644 --- a/test/recipes/70-test_sslrecords.t +++ b/test/recipes/70-test_sslrecords.t @@ -38,7 +38,7 @@ my $proxy = TLSProxy::Proxy->new( my $content_type = TLSProxy::Record::RT_APPLICATION_DATA; my $inject_recs_num = 1; $proxy->start() or plan skip_all => "Unable to start up Proxy for tests"; -plan tests => 4; +plan tests => 9; ok(TLSProxy::Message->fail(), "Out of context empty records test"); #Test 2: Injecting in context empty records should succeed @@ -62,6 +62,51 @@ $proxy->filter(\&add_frag_alert_filter); $proxy->start(); ok(!TLSProxy::Message->end(), "Fragmented alert records test"); +#Run some SSLv2 ClientHello tests + +use constant { + TLSV1_2_IN_SSLV2 => 0, + SSLV2_IN_SSLV2 => 1, + FRAGMENTED_IN_TLSV1_2 => 2, + FRAGMENTED_IN_SSLV2 => 3, + ALERT_BEFORE_SSLV2 => 4 +}; +#Test 5: Inject an SSLv2 style record format for a TLSv1.2 ClientHello +my $sslv2testtype = TLSV1_2_IN_SSLV2; +$proxy->clear(); +$proxy->filter(\&add_sslv2_filter); +$proxy->start(); +ok(TLSProxy::Message->success(), "TLSv1.2 in SSLv2 ClientHello test"); + +#Test 6: Inject an SSLv2 style record format for an SSLv2 ClientHello. We don't +# support this so it should fail. We actually treat it as an unknown +# protocol so we don't even send an alert in this case. +$sslv2testtype = SSLV2_IN_SSLV2; +$proxy->clear(); +$proxy->start(); +ok(!TLSProxy::Message->end(), "SSLv2 in SSLv2 ClientHello test"); + +#Test 7: Sanity check ClientHello fragmentation. This isn't really an SSLv2 test +# at all, but it gives us confidence that Test 8 fails for the right +# reasons +$sslv2testtype = FRAGMENTED_IN_TLSV1_2; +$proxy->clear(); +$proxy->start(); +ok(TLSProxy::Message->success(), "Fragmented ClientHello in TLSv1.2 test"); + +#Test 8: Fragment a TLSv1.2 ClientHello across a TLS1.2 record; an SSLv2 +# record; and another TLS1.2 record. This isn't allowed so should fail +$sslv2testtype = FRAGMENTED_IN_SSLV2; +$proxy->clear(); +$proxy->start(); +ok(TLSProxy::Message->fail(), "Fragmented ClientHello in TLSv1.2/SSLv2 test"); + +#Test 9: Send a TLS warning alert before an SSLv2 ClientHello. This should +# fail because an SSLv2 ClientHello must be the first record. +$sslv2testtype = ALERT_BEFORE_SSLV2; +$proxy->clear(); +$proxy->start(); +ok(TLSProxy::Message->fail(), "Alert before SSLv2 ClientHello test"); sub add_empty_recs_filter { my $proxy = shift; @@ -79,6 +124,7 @@ sub add_empty_recs_filter 0, 0, 0, + 0, "", "" ); @@ -117,6 +163,7 @@ sub add_frag_alert_filter TLSProxy::Record::RT_ALERT, TLSProxy::Record::VERS_TLS_1_2, 1, + 0, 1, 1, $byte, @@ -131,6 +178,7 @@ sub add_frag_alert_filter TLSProxy::Record::RT_ALERT, TLSProxy::Record::VERS_TLS_1_2, 1, + 0, 1, 1, $byte, @@ -138,3 +186,150 @@ sub add_frag_alert_filter ); push @{$proxy->record_list}, $record; } + +sub add_sslv2_filter +{ + my $proxy = shift; + my $clienthello; + my $record; + + # We're only interested in the initial ClientHello + if ($proxy->flight != 0) { + return; + } + + # Ditch the real ClientHello - we're going to replace it with our own + shift @{$proxy->record_list}; + + if ($sslv2testtype == ALERT_BEFORE_SSLV2) { + my $alert = pack('CC', TLSProxy::Message::AL_LEVEL_FATAL, + TLSProxy::Message::AL_DESC_NO_RENEGOTIATION); + my $alertlen = length $alert; + $record = TLSProxy::Record->new( + 0, + TLSProxy::Record::RT_ALERT, + TLSProxy::Record::VERS_TLS_1_2, + $alertlen, + 0, + $alertlen, + $alertlen, + $alert, + $alert + ); + + push @{$proxy->record_list}, $record; + } + + if ($sslv2testtype == ALERT_BEFORE_SSLV2 + || $sslv2testtype == TLSV1_2_IN_SSLV2 + || $sslv2testtype == SSLV2_IN_SSLV2) { + # This is an SSLv2 format ClientHello + $clienthello = + pack "C44", + 0x01, # ClientHello + 0x03, 0x03, #TLSv1.2 + 0x00, 0x03, # Ciphersuites len + 0x00, 0x00, # Session id len + 0x00, 0x20, # Challenge len + 0x00, 0x00, 0x2f, #AES128-SHA + 0x01, 0x18, 0x9F, 0x76, 0xEC, 0x57, 0xCE, 0xE5, 0xB3, 0xAB, 0x79, 0x90, + 0xAD, 0xAC, 0x6E, 0xD1, 0x58, 0x35, 0x03, 0x97, 0x16, 0x10, 0x82, 0x56, + 0xD8, 0x55, 0xFF, 0xE1, 0x8A, 0xA3, 0x2E, 0xF6; # Challenge + + if ($sslv2testtype == SSLV2_IN_SSLV2) { + # Set the version to "real" SSLv2 + vec($clienthello, 1, 8) = 0x00; + vec($clienthello, 2, 8) = 0x02; + } + + my $chlen = length $clienthello; + + $record = TLSProxy::Record->new( + 0, + TLSProxy::Record::RT_HANDSHAKE, + TLSProxy::Record::VERS_TLS_1_2, + $chlen, + 1, #SSLv2 + $chlen, + $chlen, + $clienthello, + $clienthello + ); + + push @{$proxy->record_list}, $record; + } else { + # For this test we're using a real TLS ClientHello + $clienthello = + pack "C49", + 0x01, # ClientHello + 0x00, 0x00, 0x2D, # Message length + 0x03, 0x03, # TLSv1.2 + 0x01, 0x18, 0x9F, 0x76, 0xEC, 0x57, 0xCE, 0xE5, 0xB3, 0xAB, 0x79, 0x90, + 0xAD, 0xAC, 0x6E, 0xD1, 0x58, 0x35, 0x03, 0x97, 0x16, 0x10, 0x82, 0x56, + 0xD8, 0x55, 0xFF, 0xE1, 0x8A, 0xA3, 0x2E, 0xF6, # Random + 0x00, # Session id len + 0x00, 0x04, # Ciphersuites len + 0x00, 0x2f, # AES128-SHA + 0x00, 0xff, # Empty reneg info SCSV + 0x01, # Compression methods len + 0x00, # Null compression + 0x00, 0x00; # Extensions len + + # Split this into 3: A TLS record; a SSLv2 record and a TLS record. + # We deliberately split the second record prior to the Challenge/Random + # and set the first byte of the random to 1. This makes the second SSLv2 + # record look like an SSLv2 ClientHello + my $frag1 = substr $clienthello, 0, 6; + my $frag2 = substr $clienthello, 6, 32; + my $frag3 = substr $clienthello, 38; + + my $fraglen = length $frag1; + $record = TLSProxy::Record->new( + 0, + TLSProxy::Record::RT_HANDSHAKE, + TLSProxy::Record::VERS_TLS_1_2, + $fraglen, + 0, + $fraglen, + $fraglen, + $frag1, + $frag1 + ); + push @{$proxy->record_list}, $record; + + $fraglen = length $frag2; + my $recvers; + if ($sslv2testtype == FRAGMENTED_IN_SSLV2) { + $recvers = 1; + } else { + $recvers = 0; + } + $record = TLSProxy::Record->new( + 0, + TLSProxy::Record::RT_HANDSHAKE, + TLSProxy::Record::VERS_TLS_1_2, + $fraglen, + $recvers, + $fraglen, + $fraglen, + $frag2, + $frag2 + ); + push @{$proxy->record_list}, $record; + + $fraglen = length $frag3; + $record = TLSProxy::Record->new( + 0, + TLSProxy::Record::RT_HANDSHAKE, + TLSProxy::Record::VERS_TLS_1_2, + $fraglen, + 0, + $fraglen, + $fraglen, + $frag3, + $frag3 + ); + push @{$proxy->record_list}, $record; + } + +} diff --git a/util/TLSProxy/Message.pm b/util/TLSProxy/Message.pm index b8db22f..321e080 100644 --- a/util/TLSProxy/Message.pm +++ b/util/TLSProxy/Message.pm @@ -37,7 +37,8 @@ use constant { #Alert descriptions use constant { AL_DESC_CLOSE_NOTIFY => 0, - AL_DESC_UNEXPECTED_MESSAGE => 10 + AL_DESC_UNEXPECTED_MESSAGE => 10, + AL_DESC_NO_RENEGOTIATION => 100 }; my %message_type = ( diff --git a/util/TLSProxy/Record.pm b/util/TLSProxy/Record.pm index 2a605e3..423bad3 100644 --- a/util/TLSProxy/Record.pm +++ b/util/TLSProxy/Record.pm @@ -98,6 +98,7 @@ sub get_records $content_type, $version, $len, + 0, $len_real, $decrypt_len, substr($packet, TLS_RECORD_HEADER_LENGTH, $len_real), @@ -167,6 +168,7 @@ sub new $content_type, $version, $len, + $sslv2, $len_real, $decrypt_len, $data, @@ -177,6 +179,7 @@ sub new content_type => $content_type, version => $version, len => $len, + sslv2 => $sslv2, len_real => $len_real, decrypt_len => $decrypt_len, data => $data, @@ -247,7 +250,11 @@ sub reconstruct_record my $self = shift; my $data; - $data = pack('Cnn', $self->content_type, $self->version, $self->len); + if ($self->sslv2) { + $data = pack('n', $self->len | 0x8000); + } else { + $data = pack('Cnn', $self->content_type, $self->version, $self->len); + } $data .= $self->data; return $data; @@ -269,6 +276,11 @@ sub version my $self = shift; return $self->{version}; } +sub sslv2 +{ + my $self = shift; + return $self->{sslv2}; +} sub len_real { my $self = shift; diff --git a/util/TLSProxy/ServerHello.pm b/util/TLSProxy/ServerHello.pm index ee2fd72..79a8be9 100644 --- a/util/TLSProxy/ServerHello.pm +++ b/util/TLSProxy/ServerHello.pm @@ -56,13 +56,25 @@ sub parse my $comp_meth = unpack('C', substr($self->data, $ptr)); $ptr++; my $extensions_len = unpack('n', substr($self->data, $ptr)); - $ptr += 2; + if (!defined $extensions_len) { + $extensions_len = 0; + } else { + $ptr += 2; + } #For now we just deal with this as a block of data. In the future we will #want to parse this - my $extension_data = substr($self->data, $ptr); + my $extension_data; + if ($extensions_len != 0) { + $extension_data = substr($self->data, $ptr); - if (length($extension_data) != $extensions_len) { - die "Invalid extension length\n"; + if (length($extension_data) != $extensions_len) { + die "Invalid extension length\n"; + } + } else { + if (length($self->data) != $ptr) { + die "Invalid extension length\n"; + } + $extension_data = ""; } my %extensions = (); while (length($extension_data) >= 4) { _____ openssl-commits mailing list To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-commits