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>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 157d19f..53a3e3c 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -240,8 +240,7 @@
// 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;
}
}
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 636d166..0a41ffe 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -313,7 +313,8 @@
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 @@
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);
diff --git a/ssl/internal.h b/ssl/internal.h
index 153c9ca..d8df2ab 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1837,9 +1837,15 @@
// 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 @@
};
// lengths of messages
-#define DTLS1_COOKIE_LENGTH 256
-
#define DTLS1_RT_HEADER_LENGTH 13
#define DTLS1_HM_HEADER_LENGTH 12
@@ -2921,9 +2925,6 @@
// 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;
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index bf6a3d1..07562df 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -639,6 +639,14 @@
// 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.
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go
index f2ef2fc..fbfbdd9 100644
--- a/ssl/test/runner/handshake_messages.go
+++ b/ssl/test/runner/handshake_messages.go
@@ -872,11 +872,8 @@
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) ||
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 4f41184..5ae7d93 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -235,9 +235,16 @@
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)
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 2b5d0e0..5c6ef4f 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -3546,6 +3546,31 @@
},
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...)
