Updates following review of SSL_stateless() code

Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/4435)

diff --git a/doc/man3/DTLSv1_listen.pod b/doc/man3/DTLSv1_listen.pod
index 02c12002684ab27a91d03cd7beb93e406274df28..062215e7acde35dbd46b2e63218d90222521f60d 100644 (file)
--- 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 335c9452ffaee0c68b95dcc879a34d7d6e066b1a..5a0fa25571f393e421898357e8eaa3bb5f0f6de2 100644 (file)
--- 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 60fa34201cb48bee1f678e3042732c27b24df528..fadc6a70ea6b76ae0da5dede0a5effef4b2e0056 100644 (file)
--- 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 82f61bceabbb9d981b5f75bf3acd99a770fda3e0..a338578fccab9abdede96d00653b6b62f873f205 100644 (file)
--- 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)))