Reject legacy_cookie in DTLS 1.3 There's no practical consequence to this, but RFC 9147 says: > legacy_cookie: A DTLS 1.3-only client MUST set the legacy_cookie field > to zero length. If a DTLS 1.3 ClientHello is received with any other > value in this field, the server MUST abort the handshake with an > "illegal_parameter" alert. Fixed: 516205743 Change-Id: I9c3ee0f2dcb7f68275a3713fee95cc0942441c09 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/95867 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com> Auto-Submit: David Benjamin <davidben@google.com> Commit-Queue: Adam Langley <agl@google.com>
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index aaa6565..0ffc246 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc
@@ -655,6 +655,14 @@ return ssl_hs_error; } + // The legacy cookie should never be sent in DTLS 1.3. + if (ssl_protocol_version(ssl) >= TLS1_3_VERSION && + client_hello.dtls_cookie_len != 0) { + ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); + OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); + return ssl_hs_error; + } + // TLS extensions. if (!ssl_parse_clienthello_tlsext(hs, &client_hello)) { OPENSSL_PUT_ERROR(SSL, SSL_R_PARSE_TLSEXT);
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index a4d6413..c241c41 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -884,6 +884,10 @@ // an empty cookie in HelloVerifyRequest. EmptyHelloVerifyRequestCookie bool + // SendLegacyDTLSCookie, if not nil, contains the legacy DTLS 1.2 cookie + // to be sent in the ClientHello (not the TLS 1.3 cookie extension). + SendLegacyDTLSCookie []byte + // 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/curve_tests.go b/ssl/test/runner/curve_tests.go index b0e75ca..956a732 100644 --- a/ssl/test/runner/curve_tests.go +++ b/ssl/test/runner/curve_tests.go
@@ -271,8 +271,8 @@ NoSupportedCurves: true, }, }, - shouldFail: true, - expectedError: ":NO_SHARED_GROUP:", + shouldFail: true, + expectedError: ":NO_SHARED_GROUP:", expectedLocalError: "remote error: missing extension", })
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 338ab58..c514a80 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -529,6 +529,7 @@ isDTLS: c.isDTLS, compressionMethods: []uint8{compressionNone}, random: make([]byte, 32), + cookie: c.config.Bugs.SendLegacyDTLSCookie, ocspStapling: !c.config.Bugs.NoOCSPStapling, sctListSupported: !c.config.Bugs.NoSignedCertificateTimestamps, supportedCurves: c.config.curvePreferences(),
diff --git a/ssl/test/runner/tls13_tests.go b/ssl/test/runner/tls13_tests.go index f66ebbf..44af4e7 100644 --- a/ssl/test/runner/tls13_tests.go +++ b/ssl/test/runner/tls13_tests.go
@@ -114,8 +114,8 @@ MissingKeyShare: true, }, }, - shouldFail: true, - expectedError: ":MISSING_KEY_SHARE:", + shouldFail: true, + expectedError: ":MISSING_KEY_SHARE:", expectedLocalError: "remote error: missing extension", }) @@ -2313,6 +2313,22 @@ strconv.Itoa(int(typeEncryptedExtensions)), }, }) + + testCases = append(testCases, testCase{ + protocol: dtls, + testType: serverTest, + name: "DTLS13-RejectLegacyCookie", + config: Config{ + MinVersion: VersionTLS13, + MaxVersion: VersionTLS13, + Bugs: ProtocolBugs{ + SendLegacyDTLSCookie: []byte("cookie"), + }, + }, + shouldFail: true, + expectedError: ":DECODE_ERROR:", + expectedLocalError: "remote error: illegal parameter", + }) } func addTLS13CipherPreferenceTests() {