From 97ea1e7f42eea97b117af08b3c1d29f6443850ab Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Tue, 23 Jan 2018 12:23:23 +0000 Subject: [PATCH] Updates following review of SSL_stateless() code Reviewed-by: Ben Kaduk (Merged from https://github.com/openssl/openssl/pull/4435) --- doc/man3/DTLSv1_listen.pod | 2 +- ssl/statem/extensions.c | 1 + ssl/statem/extensions_srvr.c | 17 ++++++++--------- test/sslapitest.c | 12 ++++++------ 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/doc/man3/DTLSv1_listen.pod b/doc/man3/DTLSv1_listen.pod index 02c1200268..062215e7ac 100644 --- a/doc/man3/DTLSv1_listen.pod +++ b/doc/man3/DTLSv1_listen.pod @@ -39,7 +39,7 @@ If TCP is being used then there is no need to use SSL_stateless(). However some stream-based transport protocols (e.g. QUIC) may not validate the source address. In this case a TLSv1.3 application would be susceptible to this attack. -As a counter measure to this issue TLSv1.3 and DTLS include a stateless cookie +As a countermeasure to this issue TLSv1.3 and DTLS include a stateless cookie mechanism. The idea is that when a client attempts to connect to a server it sends a ClientHello message. The server responds with a HelloRetryRequest (in TLSv1.3) or a HelloVerifyRequest (in DTLS) which contains a unique cookie. The diff --git a/ssl/statem/extensions.c b/ssl/statem/extensions.c index 335c9452ff..5a0fa25571 100644 --- a/ssl/statem/extensions.c +++ b/ssl/statem/extensions.c @@ -321,6 +321,7 @@ static const EXTENSION_DEFINITION ext_defs[] = { }, #endif { + /* Must be after key_share */ TLSEXT_TYPE_cookie, SSL_EXT_CLIENT_HELLO | SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST | SSL_EXT_TLS_IMPLEMENTATION_ONLY | SSL_EXT_TLS1_3_ONLY, diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 60fa34201c..fadc6a70ea 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -26,12 +26,12 @@ /* * Message header + 2 bytes for protocol version + number of random bytes + - * + number of bytes in legacy session id + 2 bytes for ciphersuite - * + 1 byte for legacy compression + 2 bytes for extension block length - * + 6 bytes for key_share extension + 4 bytes for cookie extension header - * + the number of bytes in the cookie + * + 1 byte for legacy session id length + number of bytes in legacy session id + * + 2 bytes for ciphersuite + 1 byte for legacy compression + * + 2 bytes for extension block length + 6 bytes for key_share extension + * + 4 bytes for cookie extension header + the number of bytes in the cookie */ -#define MAX_HRR_SIZE (SSL3_HM_HEADER_LENGTH + 2 + SSL3_RANDOM_SIZE \ +#define MAX_HRR_SIZE (SSL3_HM_HEADER_LENGTH + 2 + SSL3_RANDOM_SIZE + 1 \ + SSL_MAX_SSL_SESSION_ID_LENGTH + 2 + 1 + 2 + 6 + 4 \ + MAX_COOKIE_SIZE) @@ -742,11 +742,10 @@ int tls_parse_ctos_cookie(SSL *s, PACKET *pkt, unsigned int context, X509 *x, return 0; } - hmaclen = sizeof(s->session_ctx->ext.cookie_hmac_key); + hmaclen = SHA256_DIGEST_LENGTH; if (EVP_DigestSignInit(hctx, NULL, EVP_sha256(), NULL, pkey) <= 0 - || EVP_DigestSignUpdate(hctx, data, - rawlen - SHA256_DIGEST_LENGTH) <= 0 - || EVP_DigestSignFinal(hctx, hmac, &hmaclen) <= 0 + || EVP_DigestSign(hctx, hmac, &hmaclen, data, + rawlen - SHA256_DIGEST_LENGTH) <= 0 || hmaclen != SHA256_DIGEST_LENGTH) { EVP_MD_CTX_free(hctx); EVP_PKEY_free(pkey); diff --git a/test/sslapitest.c b/test/sslapitest.c index 82f61bceab..a338578fcc 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -2561,8 +2561,8 @@ static int generate_cookie_callback(SSL *ssl, unsigned char *cookie, * Not suitable as a real cookie generation function but good enough for * testing! */ - memcpy(cookie, cookie_magic_value, sizeof(cookie_magic_value)); - *cookie_len = sizeof(cookie_magic_value); + memcpy(cookie, cookie_magic_value, sizeof(cookie_magic_value) - 1); + *cookie_len = sizeof(cookie_magic_value) - 1; return 1; } @@ -2570,7 +2570,7 @@ static int generate_cookie_callback(SSL *ssl, unsigned char *cookie, static int verify_cookie_callback(SSL *ssl, const unsigned char *cookie, unsigned int cookie_len) { - if (cookie_len == sizeof(cookie_magic_value) + if (cookie_len == sizeof(cookie_magic_value) - 1 && memcmp(cookie, cookie_magic_value, cookie_len) == 0) return 1; @@ -2601,7 +2601,7 @@ static int test_stateless(void) || !TEST_false(create_ssl_connection(serverssl, clientssl, SSL_ERROR_WANT_READ)) /* This should fail because there is no cookie */ - || !TEST_int_le(SSL_stateless(serverssl), 0)) + || !TEST_false(SSL_stateless(serverssl))) goto end; /* Abandon the connection from this client */ @@ -2618,12 +2618,12 @@ static int test_stateless(void) || !TEST_false(create_ssl_connection(serverssl, clientssl, SSL_ERROR_WANT_READ)) /* This should fail because there is no cookie */ - || !TEST_int_le(SSL_stateless(serverssl), 0) + || !TEST_false(SSL_stateless(serverssl)) /* Send the second ClientHello */ || !TEST_false(create_ssl_connection(serverssl, clientssl, SSL_ERROR_WANT_READ)) /* This should succeed because a cookie is now present */ - || !TEST_int_gt(SSL_stateless(serverssl), 0) + || !TEST_true(SSL_stateless(serverssl)) /* Complete the connection */ || !TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE))) -- 2.25.1