Move the DTLS cookie to SSL_HANDSHAKE.

The cookie is only needed in SSL_HANDSHAKE, so there's no need to retain
it for the lifetime of the connection. (SSL_HANDSHAKE is released after
the handshake completes.)

Back when DTLS1_COOKIE_LENGTH was 32, storing it inline made some sense.
Now that RFC 6347 increased the maximum to 255 bytes, just indirect it
with an Array<uint8_t>. Along the way, remove the DTLS1_COOKIE_LENGTH
checks. The new limit is the largest that fits in the length prefix, so
it's always redundant. In fact, the constant was one higher was allowed
anyway. Add some tests for the maximum length, as well as zero-length
cookies.

I considered just repurposing the plain cookie field, used in
HelloRetryRequest (as opposed to HelloVerifyRequest), as they're
mutually exclusive, even in DTLS 1.3. But, when we get to DTLS 1.3,
that'll get a little hairy because ssl_write_client_hello will need
extra checks to know whether hs->cookie is meant to go in the
ClientHello directly or in extensions.

Change-Id: I1afedc7ce31414879545701bf8fe4658657ba66f
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54466
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/extensions.cc

@@ -240,8 +240,7 @@ bool ssl_parse_client_hello_with_trailing_data(const SSL *ssl, CBS *cbs,
   // Skip past DTLS cookie
   if (SSL_is_dtls(out->ssl)) {
     CBS cookie;
-    if (!CBS_get_u8_length_prefixed(cbs, &cookie) ||
-        CBS_len(&cookie) > DTLS1_COOKIE_LENGTH) {
+    if (!CBS_get_u8_length_prefixed(cbs, &cookie)) {
       return false;
     }
   }

ssl/handshake_client.cc

@@ -313,7 +313,8 @@ bool ssl_write_client_hello_without_extensions(const SSL_HANDSHAKE *hs,
 
   if (SSL_is_dtls(ssl)) {
     if (!CBB_add_u8_length_prefixed(cbb, &child) ||
-        !CBB_add_bytes(&child, ssl->d1->cookie, ssl->d1->cookie_len)) {
+        !CBB_add_bytes(&child, hs->dtls_cookie.data(),
+                       hs->dtls_cookie.size())) {
       return false;
     }
   }
@@ -626,15 +627,16 @@ static enum ssl_hs_wait_t do_read_hello_verify_request(SSL_HANDSHAKE *hs) {
   uint16_t server_version;
   if (!CBS_get_u16(&hello_verify_request, &server_version) ||
       !CBS_get_u8_length_prefixed(&hello_verify_request, &cookie) ||
-      CBS_len(&cookie) > sizeof(ssl->d1->cookie) ||
       CBS_len(&hello_verify_request) != 0) {
     OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
     ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
     return ssl_hs_error;
   }
 
-  OPENSSL_memcpy(ssl->d1->cookie, CBS_data(&cookie), CBS_len(&cookie));
-  ssl->d1->cookie_len = CBS_len(&cookie);
+  if (!hs->dtls_cookie.CopyFrom(cookie)) {
+    ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
+    return ssl_hs_error;
+  }
 
   ssl->method->next_message(ssl);
 

ssl/internal.h

@@ -1837,9 +1837,15 @@ struct SSL_HANDSHAKE {
   // ClientHelloInner.
   uint8_t inner_client_random[SSL3_RANDOM_SIZE] = {0};
 
-  // cookie is the value of the cookie received from the server, if any.
+  // cookie is the value of the cookie in HelloRetryRequest, or empty if none
+  // was received.
   Array<uint8_t> cookie;
 
+  // dtls_cookie is the value of the cookie in DTLS HelloVerifyRequest. If
+  // empty, either none was received or HelloVerifyRequest contained an empty
+  // cookie.
+  Array<uint8_t> dtls_cookie;
+
   // ech_client_outer contains the outer ECH extension to send in the
   // ClientHello, excluding the header and type byte.
   Array<uint8_t> ech_client_outer;
@@ -2854,8 +2860,6 @@ struct SSL3_STATE {
 };
 
 // lengths of messages
-#define DTLS1_COOKIE_LENGTH 256
-
 #define DTLS1_RT_HEADER_LENGTH 13
 
 #define DTLS1_HM_HEADER_LENGTH 12
@@ -2921,9 +2925,6 @@ struct DTLS1_STATE {
   // peer sent the final flight.
   bool flight_has_reply : 1;
 
-  uint8_t cookie[DTLS1_COOKIE_LENGTH] = {0};
-  size_t cookie_len = 0;
-
   // The current data and handshake epoch.  This is initially undefined, and
   // starts at zero once the initial handshake is completed.
   uint16_t r_epoch = 0;

ssl/test/runner/common.go

@@ -639,6 +639,14 @@ type ProtocolBugs struct {
 	// HelloVerifyRequest message.
 	SkipHelloVerifyRequest bool
 
+	// HelloVerifyRequestCookieLength, if non-zero, is the length of the cookie
+	// to request in HelloVerifyRequest.
+	HelloVerifyRequestCookieLength int
+
+	// EmptyHelloVerifyRequestCookie, if true, causes a DTLS server to request
+	// an empty cookie in HelloVerifyRequest.
+	EmptyHelloVerifyRequestCookie bool
+
 	// SkipCertificateStatus, if true, causes the server to skip the
 	// CertificateStatus message. This is legal because CertificateStatus is
 	// optional, even with a status_request in ServerHello.

ssl/test/runner/handshake_messages.go

@@ -872,11 +872,8 @@ func (m *clientHelloMsg) unmarshal(data []byte) bool {
 		len(m.sessionID) > 32 {
 		return false
 	}
-	if m.isDTLS {
-		if !reader.readU8LengthPrefixedBytes(&m.cookie) ||
-			len(m.cookie) > 32 {
-			return false
-		}
+	if m.isDTLS && !reader.readU8LengthPrefixedBytes(&m.cookie) {
+		return false
 	}
 	var cipherSuites byteReader
 	if !reader.readU16LengthPrefixed(&cipherSuites) ||

ssl/test/runner/handshake_server.go

@@ -235,9 +235,16 @@ func (hs *serverHandshakeState) readClientHello() error {
 	if c.isDTLS && !config.Bugs.SkipHelloVerifyRequest {
 		// Per RFC 6347, the version field in HelloVerifyRequest SHOULD
 		// be always DTLS 1.0
+		cookieLen := c.config.Bugs.HelloVerifyRequestCookieLength
+		if cookieLen == 0 {
+			cookieLen = 32
+		}
+		if c.config.Bugs.EmptyHelloVerifyRequestCookie {
+			cookieLen = 0
+		}
 		helloVerifyRequest := &helloVerifyRequestMsg{
 			vers:   VersionDTLS10,
-			cookie: make([]byte, 32),
+			cookie: make([]byte, cookieLen),
 		}
 		if _, err := io.ReadFull(c.config.rand(), helloVerifyRequest.cookie); err != nil {
 			c.sendAlert(alertInternalError)

ssl/test/runner/runner.go

@@ -3546,6 +3546,31 @@ read alert 1 0
 			},
 			flags: []string{"-async"},
 		},
+		{
+			// DTLS 1.2 allows up to a 255-byte HelloVerifyRequest cookie, which
+			// is the largest encodable value.
+			protocol: dtls,
+			name:     "DTLS-HelloVerifyRequest-255",
+			config: Config{
+				MaxVersion: VersionTLS12,
+				Bugs: ProtocolBugs{
+					HelloVerifyRequestCookieLength: 255,
+				},
+			},
+		},
+		{
+			// DTLS 1.2 allows up to a 0-byte HelloVerifyRequest cookie, which
+			// was probably a mistake in the spec but test that it works
+			// nonetheless.
+			protocol: dtls,
+			name:     "DTLS-HelloVerifyRequest-0",
+			config: Config{
+				MaxVersion: VersionTLS12,
+				Bugs: ProtocolBugs{
+					EmptyHelloVerifyRequestCookie: true,
+				},
+			},
+		},
 	}
 	testCases = append(testCases, basicTests...)