Check for invalid certificate_authorities extensions Our parsers were neither rejecting trailing data, nor an empty list. (Empty lists aren't allowed because the sender should have skipped the extension.) Update-Note: Our TLS 1.3 parsers are slightly more spec-compliant and may reject messages from buggy peers. This isn't expected to have any impact. Confirmed that OpenSSL correctly skips the extension when empty. Any stack tested against our SSL tests also would have sent it correctly. Additionally, as a server, clients that send certificate_authorities are rare. As a client, servers that send CertificateRequest are rare. Change-Id: I12c9821c12f0c075fc32635d447417b19e95263f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/93727 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Lily Chen <chlily@google.com> Auto-Submit: David Benjamin <davidben@google.com>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc index 50ce434..640001a 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc
@@ -2812,7 +2812,8 @@ } hs->ca_names = SSL_parse_CA_list(hs->ssl, out_alert, contents); - if (!hs->ca_names) { + if (!hs->ca_names || sk_CRYPTO_BUFFER_num(hs->ca_names.get()) == 0 || + CBS_len(contents) != 0) { return false; }
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index ad8e3f7..b98052c 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -2274,6 +2274,17 @@ // send a server_certificate_type extension containing the given values. // For a server, this may not contain more than 1 value. SendServerCertificateTypes []CertificateType + + // SendEmptyCertificateAuthorities, if true, causes a TLS 1.3 client or + // server to send an empty certificate_authorities extension, instead of + // omitting the extension. + SendEmptyCertificateAuthorities bool + + // ExtensionsWithTrailingData specifies a list of extensions to include + // trailing data in. + // TODO(crbug.com/505803427): Currently only implemented for ClientHello and + // CertificateRequest. + ExtensionsWithTrailingData []uint16 } func (c *Config) serverInit() {
diff --git a/ssl/test/runner/extension_tests.go b/ssl/test/runner/extension_tests.go index 3087efe..9334f45 100644 --- a/ssl/test/runner/extension_tests.go +++ b/ssl/test/runner/extension_tests.go
@@ -1880,6 +1880,40 @@ expectedLocalError: "remote error: error decoding message", }) + // The certificate authorities extension cannot be empty. The sender should + // have omitted if empty. + if ver.version >= VersionTLS13 { + testCases = append(testCases, testCase{ + protocol: protocol, + testType: clientTest, + name: "RejectEmptyCertificateAuthorities-Client-" + suffix, + config: Config{ + MaxVersion: ver.version, + ClientAuth: RequireAnyClientCert, + Bugs: ProtocolBugs{ + SendEmptyCertificateAuthorities: true, + }, + }, + shouldFail: true, + expectedError: ":ERROR_PARSING_EXTENSION:", + expectedLocalError: "remote error: error decoding message", + }) + testCases = append(testCases, testCase{ + protocol: protocol, + testType: serverTest, + name: "RejectEmptyCertificateAuthorities-Server-" + suffix, + config: Config{ + MaxVersion: ver.version, + Bugs: ProtocolBugs{ + SendEmptyCertificateAuthorities: true, + }, + }, + shouldFail: true, + expectedError: ":ERROR_PARSING_EXTENSION:", + expectedLocalError: "remote error: error decoding message", + }) + } + // Extension permutation should interact correctly with other extensions, // HelloVerifyRequest, HelloRetryRequest, and ECH. SSLTest.PermuteExtensions // in ssl_test.cc tests that the extensions are actually permuted. This @@ -2371,3 +2405,38 @@ }) } } + +// Test that our extension parsers consistently reject trailing data. +// TODO(crbug.com/505803427): Test other extensions. +func addExtensionTrailingDataTests() { + testCases = append(testCases, testCase{ + testType: clientTest, + name: "ExtensionTrailingData-CertificateAuthorities-Client-TLS13", + config: Config{ + MaxVersion: VersionTLS13, + ClientAuth: RequireAnyClientCert, + ClientCAs: makeCertPoolFromRoots(&rsaChainCertificate, &ecdsaP384Certificate), + Bugs: ProtocolBugs{ + ExtensionsWithTrailingData: []uint16{extensionCertificateAuthorities}, + }, + }, + shouldFail: true, + expectedError: ":ERROR_PARSING_EXTENSION:", + expectedLocalError: "remote error: error decoding message", + }) + testCases = append(testCases, testCase{ + testType: serverTest, + name: "ExtensionTrailingData-CertificateAuthorities-Server-TLS13", + config: Config{ + MaxVersion: VersionTLS13, + RootCAs: makeCertPoolFromRoots(&rsaChainCertificate, &ecdsaP384Certificate), + SendRootCAs: true, + Bugs: ProtocolBugs{ + ExtensionsWithTrailingData: []uint16{extensionCertificateAuthorities}, + }, + }, + shouldFail: true, + expectedError: ":ERROR_PARSING_EXTENSION:", + expectedLocalError: "remote error: error decoding message", + }) +}
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index b5d7084..f064c38 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -526,30 +526,31 @@ } hello := &clientHelloMsg{ - isDTLS: c.isDTLS, - compressionMethods: []uint8{compressionNone}, - random: make([]byte, 32), - ocspStapling: !c.config.Bugs.NoOCSPStapling, - sctListSupported: !c.config.Bugs.NoSignedCertificateTimestamps, - supportedCurves: c.config.curvePreferences(), - supportedPoints: []uint8{pointFormatUncompressed}, - nextProtoNeg: len(c.config.NextProtos) > 0, - secureRenegotiation: []byte{}, - alpnProtocols: c.config.NextProtos, - quicTransportParams: quicTransportParams, - quicTransportParamsLegacy: quicTransportParamsLegacy, - duplicateExtension: c.config.Bugs.DuplicateExtension, - channelIDSupported: c.config.ChannelID != nil, - extendedMasterSecret: maxVersion >= VersionTLS10, - srtpProtectionProfiles: c.config.SRTPProtectionProfiles, - srtpMasterKeyIdentifier: c.config.Bugs.SRTPMasterKeyIdentifier, - customExtension: c.config.Bugs.CustomExtension, - omitExtensions: c.config.Bugs.OmitExtensions, - emptyExtensions: c.config.Bugs.EmptyExtensions, - delegatedCredential: c.config.DelegatedCredentialAlgorithms, - trustAnchors: c.config.RequestTrustAnchors, - clientCertificateTypes: c.config.Bugs.SendClientCertificateTypes, - serverCertificateTypes: c.config.Bugs.SendServerCertificateTypes, + isDTLS: c.isDTLS, + compressionMethods: []uint8{compressionNone}, + random: make([]byte, 32), + ocspStapling: !c.config.Bugs.NoOCSPStapling, + sctListSupported: !c.config.Bugs.NoSignedCertificateTimestamps, + supportedCurves: c.config.curvePreferences(), + supportedPoints: []uint8{pointFormatUncompressed}, + nextProtoNeg: len(c.config.NextProtos) > 0, + secureRenegotiation: []byte{}, + alpnProtocols: c.config.NextProtos, + quicTransportParams: quicTransportParams, + quicTransportParamsLegacy: quicTransportParamsLegacy, + duplicateExtension: c.config.Bugs.DuplicateExtension, + channelIDSupported: c.config.ChannelID != nil, + extendedMasterSecret: maxVersion >= VersionTLS10, + srtpProtectionProfiles: c.config.SRTPProtectionProfiles, + srtpMasterKeyIdentifier: c.config.Bugs.SRTPMasterKeyIdentifier, + customExtension: c.config.Bugs.CustomExtension, + omitExtensions: c.config.Bugs.OmitExtensions, + emptyExtensions: c.config.Bugs.EmptyExtensions, + delegatedCredential: c.config.DelegatedCredentialAlgorithms, + trustAnchors: c.config.RequestTrustAnchors, + clientCertificateTypes: c.config.Bugs.SendClientCertificateTypes, + serverCertificateTypes: c.config.Bugs.SendServerCertificateTypes, + extensionsWithTrailingData: c.config.Bugs.ExtensionsWithTrailingData, } // Translate the bugs that modify ClientHello extension order into a @@ -672,6 +673,9 @@ if c.config.SendRootCAs && c.config.RootCAs != nil { hello.certificateAuthorities = c.config.RootCAs.Subjects() } + if c.config.Bugs.SendEmptyCertificateAuthorities { + hello.certificateAuthorities = [][]byte{} + } if maxVersion >= VersionTLS13 { // Use the same key shares between ClientHelloInner and ClientHelloOuter.
diff --git a/ssl/test/runner/handshake_messages.go b/ssl/test/runner/handshake_messages.go index 2efccfd..7d09d84 100644 --- a/ssl/test/runner/handshake_messages.go +++ b/ssl/test/runner/handshake_messages.go
@@ -7,6 +7,7 @@ import ( "errors" "fmt" + "slices" "golang.org/x/crypto/cryptobyte" ) @@ -259,6 +260,7 @@ outerExtensions []uint16 reorderOuterExtensionsWithoutCompressing bool prefixExtensions []uint16 + extensionsWithTrailingData []uint16 // The following fields are only filled in by |unmarshal| and ignored when // marshaling a new ClientHello. echPayloadStart int @@ -611,7 +613,8 @@ body: body.BytesOrPanic(), }) } - if len(m.certificateAuthorities) > 0 { + // Check against nil to allow sending an (invalid) empty list. + if m.certificateAuthorities != nil { body := cryptobyte.NewBuilder(nil) body.AddUint16LengthPrefixed(func(certificateAuthorities *cryptobyte.Builder) { for _, ca := range m.certificateAuthorities { @@ -680,6 +683,13 @@ }) } + // Append trailing data to extensions if requested. + for i := range extensions { + if slices.Contains(m.extensionsWithTrailingData, extensions[i].id) { + extensions[i].body = append(slices.Clip(extensions[i].body), 0) + } + } + if m.omitExtensions { return } @@ -2625,12 +2635,13 @@ // TLS 1.3. hasRequestContext bool - certificateTypes []byte - requestContext []byte - signatureAlgorithms []signatureAlgorithm - signatureAlgorithmsCert []signatureAlgorithm - certificateAuthorities [][]byte - customExtension uint16 + certificateTypes []byte + requestContext []byte + signatureAlgorithms []signatureAlgorithm + signatureAlgorithmsCert []signatureAlgorithm + certificateAuthorities [][]byte + customExtension uint16 + extensionsWithTrailingData []uint16 } func (m *certificateRequestMsg) marshal() []byte { @@ -2645,9 +2656,18 @@ if m.hasRequestContext { addUint8LengthPrefixedBytes(body, m.requestContext) body.AddUint16LengthPrefixed(func(extensions *cryptobyte.Builder) { - if m.hasSignatureAlgorithm { - extensions.AddUint16(extensionSignatureAlgorithms) + writeExt := func(id uint16, cb func(*cryptobyte.Builder)) { + extensions.AddUint16(id) extensions.AddUint16LengthPrefixed(func(extension *cryptobyte.Builder) { + cb(extension) + if slices.Contains(m.extensionsWithTrailingData, id) { + extension.AddUint8(0) + } + }) + } + + if m.hasSignatureAlgorithm { + writeExt(extensionSignatureAlgorithms, func(extension *cryptobyte.Builder) { extension.AddUint16LengthPrefixed(func(signatureAlgorithms *cryptobyte.Builder) { for _, sigAlg := range m.signatureAlgorithms { signatureAlgorithms.AddUint16(uint16(sigAlg)) @@ -2656,8 +2676,7 @@ }) } if len(m.signatureAlgorithmsCert) > 0 { - extensions.AddUint16(extensionSignatureAlgorithmsCert) - extensions.AddUint16LengthPrefixed(func(extension *cryptobyte.Builder) { + writeExt(extensionSignatureAlgorithmsCert, func(extension *cryptobyte.Builder) { extension.AddUint16LengthPrefixed(func(signatureAlgorithmsCert *cryptobyte.Builder) { for _, sigAlg := range m.signatureAlgorithmsCert { signatureAlgorithmsCert.AddUint16(uint16(sigAlg)) @@ -2665,9 +2684,9 @@ }) }) } - if len(m.certificateAuthorities) > 0 { - extensions.AddUint16(extensionCertificateAuthorities) - extensions.AddUint16LengthPrefixed(func(extension *cryptobyte.Builder) { + // Check against nil to allow sending an (invalid) empty list. + if m.certificateAuthorities != nil { + writeExt(extensionCertificateAuthorities, func(extension *cryptobyte.Builder) { extension.AddUint16LengthPrefixed(func(certificateAuthorities *cryptobyte.Builder) { for _, ca := range m.certificateAuthorities { addUint16LengthPrefixedBytes(certificateAuthorities, ca) @@ -2676,8 +2695,7 @@ }) } if m.customExtension > 0 { - extensions.AddUint16(m.customExtension) - extensions.AddUint16(0) // Empty extension + writeExt(m.customExtension, func(extension *cryptobyte.Builder) {}) } }) } else {
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 8425f74..fcb9c0a 100644 --- a/ssl/test/runner/handshake_server.go +++ b/ssl/test/runner/handshake_server.go
@@ -1140,10 +1140,11 @@ if requestClientCert { // Request a client certificate certReq := &certificateRequestMsg{ - hasSignatureAlgorithm: !config.Bugs.OmitCertificateRequestAlgorithms, - hasRequestContext: true, - requestContext: config.Bugs.SendRequestContext, - customExtension: config.Bugs.SendCustomCertificateRequest, + hasSignatureAlgorithm: !config.Bugs.OmitCertificateRequestAlgorithms, + hasRequestContext: true, + requestContext: config.Bugs.SendRequestContext, + customExtension: config.Bugs.SendCustomCertificateRequest, + extensionsWithTrailingData: config.Bugs.ExtensionsWithTrailingData, } if !config.Bugs.NoSignatureAlgorithms { certReq.signatureAlgorithms = config.verifySignatureAlgorithms() @@ -1157,6 +1158,9 @@ if config.ClientCAs != nil { certReq.certificateAuthorities = config.ClientCAs.Subjects() } + if config.Bugs.SendEmptyCertificateAuthorities { + certReq.certificateAuthorities = [][]byte{} + } hs.writeServerHash(certReq.marshal()) c.writeRecord(recordTypeHandshake, certReq.marshal()) }
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index f092873..43b3227 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -2382,6 +2382,7 @@ addRSAKeyUsageTests() addExtraHandshakeTests() addOmitExtensionsTests() + addExtensionTrailingDataTests() addCertCompressionTests() addJDK11WorkaroundTests() addDelegatedCredentialTests()
diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 67b6321..c92bb27 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc
@@ -789,7 +789,9 @@ if (ca.present) { hs->ca_names = SSL_parse_CA_list(ssl, &alert, &ca.data); - if (!hs->ca_names) { + if (!hs->ca_names || sk_CRYPTO_BUFFER_num(hs->ca_names.get()) == 0 || + CBS_len(&ca.data) != 0) { + OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_PARSING_EXTENSION); ssl_send_alert(ssl, SSL3_AL_FATAL, alert); return ssl_hs_error; }