From 71728dd8aa3acc0bc9d621f8c4a4032aa3325fe4 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Mon, 7 Nov 2016 13:50:43 +0000 Subject: [PATCH] Send and Receive a TLSv1.3 format ServerHello There are some minor differences in the format of a ServerHello in TLSv1.3. Perl changes reviewed by Richard Levitte. Non-perl changes reviewed by Rich Salz Reviewed-by: Rich Salz Reviewed-by: Richard Levitte --- ssl/statem/statem_clnt.c | 44 +++++++---- ssl/statem/statem_srvr.c | 6 +- ssl/t1_trce.c | 33 +++++--- test/recipes/70-test_tls13messages.t | 13 +-- test/ssl-tests/09-alpn.conf | 8 ++ test/ssl-tests/09-alpn.conf.in | 19 ++++- test/ssl-tests/12-ct.conf | 3 + test/ssl-tests/12-ct.conf.in | 113 +++++++++++++++------------ test/ssl-tests/protocol_version.pm | 16 ++++ test/sslapitest.c | 6 ++ util/TLSProxy/ServerHello.pm | 44 +++++++---- 11 files changed, 201 insertions(+), 104 deletions(-) diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c index 287d8ab8a6..bd657aabef 100644 --- a/ssl/statem/statem_clnt.c +++ b/ssl/statem/statem_clnt.c @@ -1089,17 +1089,22 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) s->hit = 0; /* Get the session-id. */ - if (!PACKET_get_length_prefixed_1(pkt, &session_id)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_LENGTH_MISMATCH); - goto f_err; - } - session_id_len = PACKET_remaining(&session_id); - if (session_id_len > sizeof s->session->session_id - || session_id_len > SSL3_SESSION_ID_SIZE) { - al = SSL_AD_ILLEGAL_PARAMETER; - SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_SSL3_SESSION_ID_TOO_LONG); - goto f_err; + if (!SSL_IS_TLS13(s)) { + if (!PACKET_get_length_prefixed_1(pkt, &session_id)) { + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_LENGTH_MISMATCH); + goto f_err; + } + session_id_len = PACKET_remaining(&session_id); + if (session_id_len > sizeof s->session->session_id + || session_id_len > SSL3_SESSION_ID_SIZE) { + al = SSL_AD_ILLEGAL_PARAMETER; + SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, + SSL_R_SSL3_SESSION_ID_TOO_LONG); + goto f_err; + } + } else { + session_id_len = 0; } if (!PACKET_get_bytes(pkt, &cipherchars, TLS_CIPHER_LEN)) { @@ -1120,8 +1125,8 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) * we can resume, and later peek at the next handshake message to see if the * server wants to resume. */ - if (s->version >= TLS1_VERSION && s->tls_session_secret_cb && - s->session->tlsext_tick) { + if (s->version >= TLS1_VERSION && !SSL_IS_TLS13(s) + && s->tls_session_secret_cb && s->session->tlsext_tick) { const SSL_CIPHER *pref_cipher = NULL; /* * s->session->master_key_length is a size_t, but this is an int for @@ -1235,11 +1240,16 @@ MSG_PROCESS_RETURN tls_process_server_hello(SSL *s, PACKET *pkt) s->s3->tmp.new_cipher = c; /* lets get the compression algorithm */ /* COMPRESSION */ - if (!PACKET_get_1(pkt, &compression)) { - SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_LENGTH_MISMATCH); - al = SSL_AD_DECODE_ERROR; - goto f_err; + if (!SSL_IS_TLS13(s)) { + if (!PACKET_get_1(pkt, &compression)) { + SSLerr(SSL_F_TLS_PROCESS_SERVER_HELLO, SSL_R_LENGTH_MISMATCH); + al = SSL_AD_DECODE_ERROR; + goto f_err; + } + } else { + compression = 0; } + #ifdef OPENSSL_NO_COMP if (compression != 0) { al = SSL_AD_ILLEGAL_PARAMETER; diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c index 33808ed9f4..fa56af1764 100644 --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c @@ -1773,9 +1773,11 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt) compm = s->s3->tmp.new_compression->id; #endif - if (!WPACKET_sub_memcpy_u8(pkt, s->session->session_id, sl) + if ((!SSL_IS_TLS13(s) + && !WPACKET_sub_memcpy_u8(pkt, s->session->session_id, sl)) || !s->method->put_cipher_by_char(s->s3->tmp.new_cipher, pkt, &len) - || !WPACKET_put_bytes_u8(pkt, compm) + || (!SSL_IS_TLS13(s) + && !WPACKET_put_bytes_u8(pkt, compm)) || !ssl_prepare_serverhello_tlsext(s) || !ssl_add_serverhello_tlsext(s, pkt, &al)) { SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR); diff --git a/ssl/t1_trce.c b/ssl/t1_trce.c index 421d90d289..ee08d0eb87 100644 --- a/ssl/t1_trce.c +++ b/ssl/t1_trce.c @@ -588,12 +588,17 @@ static int ssl_print_hexbuf(BIO *bio, int indent, } static int ssl_print_version(BIO *bio, int indent, const char *name, - const unsigned char **pmsg, size_t *pmsglen) + const unsigned char **pmsg, size_t *pmsglen, + unsigned int *version) { int vers; if (*pmsglen < 2) return 0; vers = ((*pmsg)[0] << 8) | (*pmsg)[1]; + if (version != NULL) { + /* TODO(TLS1.3): Remove the draft conditional here before release */ + *version = (vers == TLS1_3_VERSION_DRAFT) ? TLS1_3_VERSION : vers; + } BIO_indent(bio, indent, 80); BIO_printf(bio, "%s=0x%x (%s)\n", name, vers, ssl_trace_str(vers, ssl_version_tbl)); @@ -796,7 +801,7 @@ static int ssl_print_client_hello(BIO *bio, SSL *ssl, int indent, { size_t len; unsigned int cs; - if (!ssl_print_version(bio, indent, "client_version", &msg, &msglen)) + if (!ssl_print_version(bio, indent, "client_version", &msg, &msglen, NULL)) return 0; if (!ssl_print_random(bio, indent, &msg, &msglen)) return 0; @@ -849,7 +854,7 @@ static int ssl_print_client_hello(BIO *bio, SSL *ssl, int indent, static int dtls_print_hello_vfyrequest(BIO *bio, int indent, const unsigned char *msg, size_t msglen) { - if (!ssl_print_version(bio, indent, "server_version", &msg, &msglen)) + if (!ssl_print_version(bio, indent, "server_version", &msg, &msglen, NULL)) return 0; if (!ssl_print_hexbuf(bio, indent, "cookie", 1, &msg, &msglen)) return 0; @@ -860,11 +865,13 @@ static int ssl_print_server_hello(BIO *bio, int indent, const unsigned char *msg, size_t msglen) { unsigned int cs; - if (!ssl_print_version(bio, indent, "server_version", &msg, &msglen)) + unsigned int vers; + if (!ssl_print_version(bio, indent, "server_version", &msg, &msglen, &vers)) return 0; if (!ssl_print_random(bio, indent, &msg, &msglen)) return 0; - if (!ssl_print_hexbuf(bio, indent, "session_id", 1, &msg, &msglen)) + if (vers != TLS1_3_VERSION + && !ssl_print_hexbuf(bio, indent, "session_id", 1, &msg, &msglen)) return 0; if (msglen < 2) return 0; @@ -874,13 +881,15 @@ static int ssl_print_server_hello(BIO *bio, int indent, msg[0], msg[1], ssl_trace_str(cs, ssl_ciphers_tbl)); msg += 2; msglen -= 2; - if (msglen < 1) - return 0; - BIO_indent(bio, indent, 80); - BIO_printf(bio, "compression_method: %s (0x%02X)\n", - ssl_trace_str(msg[0], ssl_comp_tbl), msg[0]); - msg++; - msglen--; + if (vers != TLS1_3_VERSION) { + if (msglen < 1) + return 0; + BIO_indent(bio, indent, 80); + BIO_printf(bio, "compression_method: %s (0x%02X)\n", + ssl_trace_str(msg[0], ssl_comp_tbl), msg[0]); + msg++; + msglen--; + } if (!ssl_print_extensions(bio, indent, 1, msg, msglen)) return 0; return 1; diff --git a/test/recipes/70-test_tls13messages.t b/test/recipes/70-test_tls13messages.t index 62c12c4ad3..50baf2e0fb 100755 --- a/test/recipes/70-test_tls13messages.t +++ b/test/recipes/70-test_tls13messages.t @@ -60,17 +60,18 @@ sub checkmessages($$); #Test 1: Check we get all the right messages for a default handshake (undef, my $session) = tempfile(); -$proxy->serverconnects(2); +#$proxy->serverconnects(2); $proxy->clientflags("-sess_out ".$session); $proxy->start() or plan skip_all => "Unable to start up Proxy for tests"; -plan tests => 4; +plan tests => 3; checkmessages(DEFAULT_HANDSHAKE, "Default handshake test"); +#TODO(TLS1.3): Test temporarily disabled until we implement TLS1.3 resumption #Test 2: Resumption handshake -$proxy->clearClient(); -$proxy->clientflags("-sess_in ".$session); -$proxy->clientstart(); -checkmessages(RESUME_HANDSHAKE, "Resumption handshake test"); +#$proxy->clearClient(); +#$proxy->clientflags("-sess_in ".$session); +#$proxy->clientstart(); +#checkmessages(RESUME_HANDSHAKE, "Resumption handshake test"); unlink $session; #Test 3: A default handshake, but with a CertificateStatus message diff --git a/test/ssl-tests/09-alpn.conf b/test/ssl-tests/09-alpn.conf index e7e6cb9534..fc3c8da154 100644 --- a/test/ssl-tests/09-alpn.conf +++ b/test/ssl-tests/09-alpn.conf @@ -383,6 +383,7 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [10-alpn-simple-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer @@ -425,6 +426,7 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [11-alpn-server-switch-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer @@ -465,11 +467,13 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [12-alpn-client-switch-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [12-alpn-client-switch-resumption-resume-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer @@ -515,6 +519,7 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [13-alpn-alert-on-mismatch-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer @@ -560,6 +565,7 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [14-alpn-no-server-support-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer @@ -595,11 +601,13 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [15-alpn-no-client-support-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [15-alpn-no-client-support-resumption-resume-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer diff --git a/test/ssl-tests/09-alpn.conf.in b/test/ssl-tests/09-alpn.conf.in index 18560e1801..ff931a9425 100644 --- a/test/ssl-tests/09-alpn.conf.in +++ b/test/ssl-tests/09-alpn.conf.in @@ -204,6 +204,8 @@ our @tests = ( }, }, client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2", extra => { "ALPNProtocols" => "foo", }, @@ -227,6 +229,8 @@ our @tests = ( }, }, client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2", extra => { "ALPNProtocols" => "foo,bar,baz", }, @@ -245,11 +249,15 @@ our @tests = ( }, }, client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2", extra => { "ALPNProtocols" => "foo,baz", }, }, resume_client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2", extra => { "ALPNProtocols" => "bar,baz", }, @@ -273,6 +281,8 @@ our @tests = ( }, }, client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2", extra => { "ALPNProtocols" => "foo,bar", }, @@ -292,6 +302,8 @@ our @tests = ( }, resume_server => { }, client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2", extra => { "ALPNProtocols" => "foo", }, @@ -310,11 +322,16 @@ our @tests = ( }, }, client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2", extra => { "ALPNProtocols" => "foo", }, }, - resume_client => { }, + resume_client => { + #TODO(TLS1.3): Temporary until we support TLSv1.3 resumption + MaxProtocol => "TLSv1.2" + }, test => { "HandshakeMode" => "Resume", "ResumptionExpected" => "Yes", diff --git a/test/ssl-tests/12-ct.conf b/test/ssl-tests/12-ct.conf index 22fa18dd45..14b8e938c0 100644 --- a/test/ssl-tests/12-ct.conf +++ b/test/ssl-tests/12-ct.conf @@ -79,6 +79,7 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [2-ct-permissive-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer @@ -111,11 +112,13 @@ PrivateKey = ${ENV::TEST_CERTS_DIR}/serverkey.pem [3-ct-strict-resumption-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer [3-ct-strict-resumption-resume-client] CipherString = DEFAULT +MaxProtocol = TLSv1.2 VerifyCAFile = ${ENV::TEST_CERTS_DIR}/rootcert.pem VerifyMode = Peer diff --git a/test/ssl-tests/12-ct.conf.in b/test/ssl-tests/12-ct.conf.in index 9964d013c2..e7fe1b93d2 100644 --- a/test/ssl-tests/12-ct.conf.in +++ b/test/ssl-tests/12-ct.conf.in @@ -18,63 +18,72 @@ package ssltests; our @tests = ( # Currently only have tests for certs without SCTs. { - name => "ct-permissive", - server => { }, - client => { - extra => { - "CTValidation" => "Permissive", - }, - }, - test => { - "ExpectedResult" => "Success", - }, + name => "ct-permissive", + server => { }, + client => { + extra => { + "CTValidation" => "Permissive", + }, + }, + test => { + "ExpectedResult" => "Success", + }, }, { - name => "ct-strict", - server => { }, - client => { - extra => { - "CTValidation" => "Strict", - }, - }, - test => { - "ExpectedResult" => "ClientFail", - "ExpectedClientAlert" => "HandshakeFailure", - }, + name => "ct-strict", + server => { }, + client => { + extra => { + "CTValidation" => "Strict", + }, + }, + test => { + "ExpectedResult" => "ClientFail", + "ExpectedClientAlert" => "HandshakeFailure", + }, }, { - name => "ct-permissive-resumption", - server => { }, - client => { - extra => { - "CTValidation" => "Permissive", - }, - }, - test => { - "HandshakeMode" => "Resume", - "ResumptionExpected" => "Yes", - "ExpectedResult" => "Success", - }, + name => "ct-permissive-resumption", + server => { }, + client => { + #TODO(TLS1.3): Temporarily set to TLSv1.2 until we implement TLS1.3 + # resumption + MaxProtocol => "TLSv1.2", + extra => { + "CTValidation" => "Permissive", + }, + }, + test => { + "HandshakeMode" => "Resume", + "ResumptionExpected" => "Yes", + "ExpectedResult" => "Success", + }, }, { - name => "ct-strict-resumption", - server => { }, - client => { - extra => { - "CTValidation" => "Permissive", - }, - }, - # SCTs are not present during resumption, so the resumption - # should succeed. - resume_client => { - extra => { - "CTValidation" => "Strict", - }, - }, - test => { - "HandshakeMode" => "Resume", - "ResumptionExpected" => "Yes", - "ExpectedResult" => "Success", - }, + name => "ct-strict-resumption", + server => { }, + client => { + #TODO(TLS1.3): Temporarily set to TLSv1.2 until we implement TLS1.3 + # resumption + MaxProtocol => "TLSv1.2", + extra => { + "CTValidation" => "Permissive", + }, + }, + # SCTs are not present during resumption, so the resumption + # should succeed. + resume_client => { + #TODO(TLS1.3): Temporarily set to TLSv1.2 until we implement TLS1.3 + # resumption + MaxProtocol => "TLSv1.2", + extra => { + "CTValidation" => "Strict", + }, + }, + test => { + "HandshakeMode" => "Resume", + "ResumptionExpected" => "Yes", + "ExpectedResult" => "Success", + }, }, ); diff --git a/test/ssl-tests/protocol_version.pm b/test/ssl-tests/protocol_version.pm index cc39c757c4..a41ffc4b7a 100644 --- a/test/ssl-tests/protocol_version.pm +++ b/test/ssl-tests/protocol_version.pm @@ -135,6 +135,22 @@ sub generate_resumption_tests { # Don't write the redundant "Method = TLS" into the configuration. undef $method if !$dtls; + + #TODO(TLS1.3): This is temporary code while we do not have support for + # TLS1.3 resumption. We recalculate min_tls_enabled and + # max_tls_enabled, ignoring TLS1.3 + foreach my $i (0..($#tls_protocols - 1)) { + if (!$is_tls_disabled[$i]) { + $min_tls_enabled = $i; + last; + } + } + foreach my $i (0..($#tls_protocols - 1)) { + if (!$is_tls_disabled[$i]) { + $max_tls_enabled = $i; + } + } + my @protocols = $dtls ? @dtls_protocols : @tls_protocols; my $min_enabled = $dtls ? $min_dtls_enabled : $min_tls_enabled; my $max_enabled = $dtls ? $max_dtls_enabled : $max_tls_enabled; diff --git a/test/sslapitest.c b/test/sslapitest.c index 1fa9a8df9b..add38cf622 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -430,6 +430,12 @@ static int execute_test_session(SSL_SESSION_TEST_FIXTURE fix) SSL_CTX_set_min_proto_version(cctx, TLS1_2_VERSION); #endif + /* + * TODO(TLS1.3): Test temporarily disabled for TLS1.3 until we've + * implemented session resumption. + */ + SSL_CTX_set_max_proto_version(cctx, TLS1_2_VERSION); + /* Set up session cache */ if (fix.use_ext_cache) { SSL_CTX_sess_set_new_cb(cctx, new_session_cb); diff --git a/util/TLSProxy/ServerHello.pm b/util/TLSProxy/ServerHello.pm index a1bc7b3d48..40f04c2313 100644 --- a/util/TLSProxy/ServerHello.pm +++ b/util/TLSProxy/ServerHello.pm @@ -45,16 +45,30 @@ sub parse my $self = shift; my $ptr = 2; my ($server_version) = unpack('n', $self->data); + + # TODO(TLS1.3): Replace this reference to draft version before release + if ($server_version == TLSProxy::Record::VERS_TLS_1_3_DRAFT) { + $server_version = TLSProxy::Record::VERS_TLS_1_3; + TLSProxy::Proxy->is_tls13(1); + } + my $random = substr($self->data, $ptr, 32); $ptr += 32; - my $session_id_len = unpack('C', substr($self->data, $ptr)); - $ptr++; - my $session = substr($self->data, $ptr, $session_id_len); - $ptr += $session_id_len; + my $session_id_len = 0; + my $session = ""; + if (!TLSProxy::Proxy->is_tls13()) { + $session_id_len = unpack('C', substr($self->data, $ptr)); + $ptr++; + $session = substr($self->data, $ptr, $session_id_len); + $ptr += $session_id_len; + } my $ciphersuite = unpack('n', substr($self->data, $ptr)); $ptr += 2; - my $comp_meth = unpack('C', substr($self->data, $ptr)); - $ptr++; + my $comp_meth = 0; + if (!TLSProxy::Proxy->is_tls13()) { + $comp_meth = unpack('C', substr($self->data, $ptr)); + $ptr++; + } my $extensions_len = unpack('n', substr($self->data, $ptr)); if (!defined $extensions_len) { $extensions_len = 0; @@ -94,11 +108,9 @@ sub parse $self->process_data(); - # TODO(TLS1.3): Replace this reference to draft version before release - if ($server_version == TLSProxy::Record::VERS_TLS_1_3_DRAFT) { + if (TLSProxy::Proxy->is_tls13()) { TLSProxy::Record->server_encrypting(1); TLSProxy::Record->client_encrypting(1); - TLSProxy::Proxy->is_tls13(1); } print " Server Version:".$server_version."\n"; @@ -125,10 +137,14 @@ sub set_message_contents $data = pack('n', $self->server_version); $data .= $self->random; - $data .= pack('C', $self->session_id_len); - $data .= $self->session; + if (!TLSProxy::Proxy->is_tls13()) { + $data .= pack('C', $self->session_id_len); + $data .= $self->session; + } $data .= pack('n', $self->ciphersuite); - $data .= pack('C', $self->comp_meth); + if (!TLSProxy::Proxy->is_tls13()) { + $data .= pack('C', $self->comp_meth); + } foreach my $key (keys %{$self->extension_data}) { my $extdata = ${$self->extension_data}{$key}; @@ -152,9 +168,9 @@ sub server_version { my $self = shift; if (@_) { - $self->{client_version} = shift; + $self->{server_version} = shift; } - return $self->{client_version}; + return $self->{server_version}; } sub random { -- 2.25.1