[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Tue May 9 16:29:23 UTC 2017
The branch master has been updated
via 5e3766e2f15b3a8ea696b194c32a141cbe668d4e (commit)
via 66d4bf6b20d8769a3c2bf1a0c4cb3155365601e7 (commit)
via c96ec6f8048c7c2210c5d45276000190051c229f (commit)
via 7b1ec1cfb76dfd71519d4a1482be0355817b06fc (commit)
via 07d447a6fcd02bbccca9f7bd139cf0554fedf48c (commit)
from ad448b21f8dcb0f2c60f7edcec6f00f0857c474f (commit)
- Log -----------------------------------------------------------------
commit 5e3766e2f15b3a8ea696b194c32a141cbe668d4e
Author: Matt Caswell <matt at openssl.org>
Date: Mon May 8 16:05:49 2017 +0100
Add test for no change following an HRR
Verify that we fail if we receive an HRR but no change will result in
ClientHello2.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3414)
commit 66d4bf6b20d8769a3c2bf1a0c4cb3155365601e7
Author: Matt Caswell <matt at openssl.org>
Date: Mon May 8 16:05:16 2017 +0100
Verify that if we have an HRR then something will change
It is invalid if we receive an HRR but no change will result in
ClientHello2.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3414)
commit c96ec6f8048c7c2210c5d45276000190051c229f
Author: Matt Caswell <matt at openssl.org>
Date: Tue May 9 08:52:48 2017 +0100
More TLSv1.3 cookie tests
Test sending a cookie without a key_share
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3414)
commit 7b1ec1cfb76dfd71519d4a1482be0355817b06fc
Author: Matt Caswell <matt at openssl.org>
Date: Tue May 9 08:52:04 2017 +0100
Fix HRR bug
If an HRR gets sent without a key_share (e.g. cookie only) then the code
fails when it should not.
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3414)
commit 07d447a6fcd02bbccca9f7bd139cf0554fedf48c
Author: Matt Caswell <matt at openssl.org>
Date: Mon May 8 16:51:47 2017 +0100
Don't do the final key_share checks if we are in an HRR
Reviewed-by: Rich Salz <rsalz at openssl.org>
(Merged from https://github.com/openssl/openssl/pull/3414)
-----------------------------------------------------------------------
Summary of changes:
include/openssl/ssl.h | 1 +
ssl/ssl_err.c | 1 +
ssl/statem/extensions.c | 4 +++
ssl/statem/extensions_clnt.c | 42 ++++++++++++++---------
ssl/statem/statem_clnt.c | 18 +++++++++-
test/recipes/70-test_key_share.t | 34 +++++++++++++++---
test/recipes/70-test_tls13cookie.t | 70 +++++++++++++++++++++++++++-----------
util/TLSProxy/Message.pm | 1 +
8 files changed, 128 insertions(+), 43 deletions(-)
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index e89e97f..54028f6 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -2680,6 +2680,7 @@ int ERR_load_SSL_strings(void);
# define SSL_R_NO_CERTIFICATES_RETURNED 176
# define SSL_R_NO_CERTIFICATE_ASSIGNED 177
# define SSL_R_NO_CERTIFICATE_SET 179
+# define SSL_R_NO_CHANGE_FOLLOWING_HRR 205
# define SSL_R_NO_CIPHERS_AVAILABLE 181
# define SSL_R_NO_CIPHERS_SPECIFIED 183
# define SSL_R_NO_CIPHER_MATCH 185
diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
index 18a38df..06cd852 100644
--- a/ssl/ssl_err.c
+++ b/ssl/ssl_err.c
@@ -646,6 +646,7 @@ static ERR_STRING_DATA SSL_str_reasons[] = {
{ERR_REASON(SSL_R_NO_CERTIFICATES_RETURNED), "no certificates returned"},
{ERR_REASON(SSL_R_NO_CERTIFICATE_ASSIGNED), "no certificate assigned"},
{ERR_REASON(SSL_R_NO_CERTIFICATE_SET), "no certificate set"},
+ {ERR_REASON(SSL_R_NO_CHANGE_FOLLOWING_HRR), "no change following hrr"},
{ERR_REASON(SSL_R_NO_CIPHERS_AVAILABLE), "no ciphers available"},
{ERR_REASON(SSL_R_NO_CIPHERS_SPECIFIED), "no ciphers specified"},
{ERR_REASON(SSL_R_NO_CIPHER_MATCH), "no cipher match"},
diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c
index 8984577..9b16014 100644
--- a/ssl/statem/extensions.c
+++ b/ssl/statem/extensions.c
@@ -1058,6 +1058,10 @@ static int final_key_share(SSL *s, unsigned int context, int sent, int *al)
if (!SSL_IS_TLS13(s))
return 1;
+ /* Nothing to do for key_share in an HRR */
+ if ((context & SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST) != 0)
+ return 1;
+
/*
* If
* we are a client
diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c
index 898992d..3f7fce0 100644
--- a/ssl/statem/extensions_clnt.c
+++ b/ssl/statem/extensions_clnt.c
@@ -530,14 +530,26 @@ int tls_construct_ctos_psk_kex_modes(SSL *s, WPACKET *pkt, unsigned int context,
#ifndef OPENSSL_NO_TLS1_3
static int add_key_share(SSL *s, WPACKET *pkt, unsigned int curve_id)
{
- unsigned char *encoded_point;
- EVP_PKEY *key_share_key;
+ unsigned char *encoded_point = NULL;
+ EVP_PKEY *key_share_key = NULL;
size_t encodedlen;
- key_share_key = ssl_generate_pkey_curve(curve_id);
- if (key_share_key == NULL) {
- SSLerr(SSL_F_ADD_KEY_SHARE, ERR_R_EVP_LIB);
- return 0;
+ if (s->s3->tmp.pkey != NULL) {
+ assert(s->hello_retry_request);
+ if (!s->hello_retry_request) {
+ SSLerr(SSL_F_ADD_KEY_SHARE, ERR_R_INTERNAL_ERROR);
+ return 0;
+ }
+ /*
+ * Could happen if we got an HRR that wasn't requesting a new key_share
+ */
+ key_share_key = s->s3->tmp.pkey;
+ } else {
+ key_share_key = ssl_generate_pkey_curve(curve_id);
+ if (key_share_key == NULL) {
+ SSLerr(SSL_F_ADD_KEY_SHARE, ERR_R_EVP_LIB);
+ return 0;
+ }
}
/* Encode the public key. */
@@ -545,17 +557,14 @@ static int add_key_share(SSL *s, WPACKET *pkt, unsigned int curve_id)
&encoded_point);
if (encodedlen == 0) {
SSLerr(SSL_F_ADD_KEY_SHARE, ERR_R_EC_LIB);
- EVP_PKEY_free(key_share_key);
- return 0;
+ goto err;
}
/* Create KeyShareEntry */
if (!WPACKET_put_bytes_u16(pkt, curve_id)
|| !WPACKET_sub_memcpy_u16(pkt, encoded_point, encodedlen)) {
SSLerr(SSL_F_ADD_KEY_SHARE, ERR_R_INTERNAL_ERROR);
- EVP_PKEY_free(key_share_key);
- OPENSSL_free(encoded_point);
- return 0;
+ goto err;
}
/*
@@ -568,6 +577,11 @@ static int add_key_share(SSL *s, WPACKET *pkt, unsigned int curve_id)
OPENSSL_free(encoded_point);
return 1;
+ err:
+ if (s->s3->tmp.pkey == NULL)
+ EVP_PKEY_free(key_share_key);
+ OPENSSL_free(encoded_point);
+ return 0;
}
#endif
@@ -594,12 +608,6 @@ int tls_construct_ctos_key_share(SSL *s, WPACKET *pkt, unsigned int context,
return 0;
}
- if (s->s3->tmp.pkey != NULL) {
- /* Shouldn't happen! */
- SSLerr(SSL_F_TLS_CONSTRUCT_CTOS_KEY_SHARE, ERR_R_INTERNAL_ERROR);
- return 0;
- }
-
/*
* TODO(TLS1.3): Make the number of key_shares sent configurable. For
* now, just send one
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index a66dd40..6bff9d4 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -1609,7 +1609,11 @@ static MSG_PROCESS_RETURN tls_process_hello_retry_request(SSL *s, PACKET *pkt)
goto f_err;
}
- if (!PACKET_as_length_prefixed_2(pkt, &extpkt)) {
+ if (!PACKET_as_length_prefixed_2(pkt, &extpkt)
+ /* Must have a non-empty extensions block */
+ || PACKET_remaining(&extpkt) == 0
+ /* Must be no trailing data after extensions */
+ || PACKET_remaining(pkt) != 0) {
al = SSL_AD_DECODE_ERROR;
SSLerr(SSL_F_TLS_PROCESS_HELLO_RETRY_REQUEST, SSL_R_BAD_LENGTH);
goto f_err;
@@ -1622,6 +1626,18 @@ static MSG_PROCESS_RETURN tls_process_hello_retry_request(SSL *s, PACKET *pkt)
goto f_err;
OPENSSL_free(extensions);
+ extensions = NULL;
+
+ if (s->ext.tls13_cookie_len == 0 && s->s3->tmp.pkey != NULL) {
+ /*
+ * We didn't receive a cookie or a new key_share so the next
+ * ClientHello will not change
+ */
+ al = SSL_AD_ILLEGAL_PARAMETER;
+ SSLerr(SSL_F_TLS_PROCESS_HELLO_RETRY_REQUEST,
+ SSL_R_NO_CHANGE_FOLLOWING_HRR);
+ goto f_err;
+ }
/*
* Re-initialise the Transcript Hash. We're going to prepopulate it with
diff --git a/test/recipes/70-test_key_share.t b/test/recipes/70-test_key_share.t
index e5212d4..62ab66c 100644
--- a/test/recipes/70-test_key_share.t
+++ b/test/recipes/70-test_key_share.t
@@ -24,7 +24,8 @@ use constant {
KEX_LEN_MISMATCH => 8,
ZERO_LEN_KEX_DATA => 9,
TRAILING_DATA => 10,
- SELECT_X25519 => 11
+ SELECT_X25519 => 11,
+ NO_KEY_SHARES_IN_HRR => 12
};
use constant {
@@ -75,7 +76,7 @@ $direction = CLIENT_TO_SERVER;
$proxy->filter(\&modify_key_shares_filter);
$proxy->serverflags("-curves P-256");
$proxy->start() or plan skip_all => "Unable to start up Proxy for tests";
-plan tests => 21;
+plan tests => 22;
ok(TLSProxy::Message->success(), "Success after HRR");
#Test 2: The server sending an HRR requesting a group the client already sent
@@ -219,12 +220,21 @@ $proxy->serverflags("-no_tls1_3");
$proxy->start();
ok(TLSProxy::Message->success(), "Ignore key_share for TLS<=1.2 server");
+#Test 22: The server sending an HRR but not requesting a new key_share should
+# fail
+$proxy->clear();
+$testtype = NO_KEY_SHARES_IN_HRR;
+$proxy->serverflags("-curves X25519");
+$proxy->start();
+ok(TLSProxy::Message->fail(), "Server sends HRR with no key_shares");
+
sub modify_key_shares_filter
{
my $proxy = shift;
# We're only interested in the initial ClientHello
- if (($direction == CLIENT_TO_SERVER && $proxy->flight != 0)
+ if (($direction == CLIENT_TO_SERVER && $proxy->flight != 0
+ && ($proxy->flight != 1 || $testtype != NO_KEY_SHARES_IN_HRR))
|| ($direction == SERVER_TO_CLIENT && $proxy->flight != 1)) {
return;
}
@@ -296,9 +306,18 @@ sub modify_key_shares_filter
"155155B95269ED5C87EAA99C2EF5A593".
"EDF83495E80380089F831B94D14B1421", #key_exchange data
0x00; #Trailing garbage
+ } elsif ($testtype == NO_KEY_SHARES_IN_HRR) {
+ #We trick the server into thinking we sent a P-256 key_share -
+ #but the client actually sent X25519
+ $ext = pack "C7",
+ 0x00, 0x05, #List Length
+ 0x00, 0x17, #P-256
+ 0x00, 0x01, #key_exchange data length
+ 0xff; #Dummy key_share data
}
- if ($testtype != EMPTY_EXTENSION) {
+ if ($testtype != EMPTY_EXTENSION
+ && $testtype != NO_KEY_SHARES_IN_HRR) {
$message->set_extension(
TLSProxy::Message::EXT_SUPPORTED_GROUPS, $suppgroups);
}
@@ -351,7 +370,12 @@ sub modify_key_shares_filter
$message->set_extension(TLSProxy::Message::EXT_KEY_SHARE, $ext);
$message->repack();
- }
+ } elsif ($message->mt == TLSProxy::Message::MT_HELLO_RETRY_REQUEST
+ && $testtype == NO_KEY_SHARES_IN_HRR) {
+ $message->delete_extension(TLSProxy::Message::EXT_KEY_SHARE);
+ $message->set_extension(TLSProxy::Message::EXT_UNKNOWN, "");
+ $message->repack();
+ }
}
}
diff --git a/test/recipes/70-test_tls13cookie.t b/test/recipes/70-test_tls13cookie.t
index c263e0e..3d3a10f 100644
--- a/test/recipes/70-test_tls13cookie.t
+++ b/test/recipes/70-test_tls13cookie.t
@@ -28,6 +28,11 @@ plan skip_all => "$test_name needs TLS1.3 enabled"
$ENV{OPENSSL_ia32cap} = '~0x200000200000000';
+use constant {
+ COOKIE_ONLY => 0,
+ COOKIE_AND_KEY_SHARE => 1
+};
+
my $proxy = TLSProxy::Proxy->new(
undef,
cmdstr(app(["openssl"]), display => 1),
@@ -36,22 +41,31 @@ my $proxy = TLSProxy::Proxy->new(
);
my $cookieseen = 0;
+my $testtype;
#Test 1: Inserting a cookie into an HRR should see it echoed in the ClientHello
+$testtype = COOKIE_ONLY;
$proxy->filter(\&cookie_filter);
-$proxy->serverflags("-curves P-256");
+$proxy->serverflags("-curves X25519");
$proxy->start() or plan skip_all => "Unable to start up Proxy for tests";
-plan tests => 1;
+plan tests => 2;
+ok(TLSProxy::Message->success() && $cookieseen == 1, "Cookie seen");
+
+#Test 2: Same as test 1 but should also work where a new key_share is also
+# required
+$testtype = COOKIE_AND_KEY_SHARE;
+$proxy->clear();
+$proxy->clientflags("-curves P-256:X25519");
+$proxy->serverflags("-curves X25519");
+$proxy->start();
ok(TLSProxy::Message->success() && $cookieseen == 1, "Cookie seen");
sub cookie_filter
{
my $proxy = shift;
- # We're only interested in the HRR and subsequent ClientHello
- if ($proxy->flight != 1 && $proxy->flight != 2) {
- return;
- }
+ # We're only interested in the HRR and both ClientHellos
+ return if ($proxy->flight > 2);
my $ext = pack "C8",
0x00, 0x06, #Cookie Length
@@ -60,22 +74,38 @@ sub cookie_filter
0x04, 0x05;
foreach my $message (@{$proxy->message_list}) {
- if ($message->mt == TLSProxy::Message::MT_HELLO_RETRY_REQUEST) {
-
+ if ($message->mt == TLSProxy::Message::MT_HELLO_RETRY_REQUEST
+ && ${$message->records}[0]->flight == 1) {
+ $message->delete_extension(TLSProxy::Message::EXT_KEY_SHARE)
+ if ($testtype == COOKIE_ONLY);
$message->set_extension(TLSProxy::Message::EXT_COOKIE, $ext);
$message->repack();
- } elsif ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO
- && ${$message->records}[0]->flight == 2) {
- #cmp can behave differently dependent on locale
- no locale;
- my $cookie =
- $message->extension_data->{TLSProxy::Message::EXT_COOKIE};
-
- return if !defined($cookie);
-
- return if ($cookie cmp $ext) != 0;
-
- $cookieseen = 1;
+ } elsif ($message->mt == TLSProxy::Message::MT_CLIENT_HELLO) {
+ if (${$message->records}[0]->flight == 0) {
+ if ($testtype == COOKIE_ONLY) {
+ my $ext = pack "C7",
+ 0x00, 0x05, #List Length
+ 0x00, 0x17, #P-256
+ 0x00, 0x01, #key_exchange data length
+ 0xff; #Dummy key_share data
+ # Trick the server into thinking we got an unacceptable
+ # key_share
+ $message->set_extension(
+ TLSProxy::Message::EXT_KEY_SHARE, $ext);
+ $message->repack();
+ }
+ } else {
+ #cmp can behave differently dependent on locale
+ no locale;
+ my $cookie =
+ $message->extension_data->{TLSProxy::Message::EXT_COOKIE};
+
+ return if !defined($cookie);
+
+ return if ($cookie cmp $ext) != 0;
+
+ $cookieseen = 1;
+ }
}
}
}
diff --git a/util/TLSProxy/Message.pm b/util/TLSProxy/Message.pm
index 3c19845..4cb594c 100644
--- a/util/TLSProxy/Message.pm
+++ b/util/TLSProxy/Message.pm
@@ -86,6 +86,7 @@ use constant {
# (i.e. not read), and even then only when enabled. We use it to test
# handling of duplicate extensions.
EXT_DUPLICATE_EXTENSION => 0xfde8,
+ EXT_UNKNOWN => 0xfffe,
#Unknown extension that should appear last
EXT_FORCE_LAST => 0xffff
};
More information about the openssl-commits
mailing list