From 602f4669ab8e01cb02747e4fff1cd702a84c5f1d Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 7 Dec 2018 12:06:22 -0600 Subject: [PATCH] Forbid empty CertificateRequestsupported_signature_algorithms in TLS 1.2. See the IETF thread here: https://www.ietf.org/mail-archive/web/tls/current/msg27292.html In particular, although the original publication of RFC 5246 had a syntax error in the field (the minimum length was unspecified), there is an errata from 2012 to fix it to be non-empty. https://www.rfc-editor.org/errata/eid2864 Currently, when empty, we implicitly interpret it as SHA1/*, matching the server behavior in missing extension in ClientHellos. However that text does not support doing it for CertificateRequests, and there is not much reason to. That default (which is in itself confusing and caused problems such as older OpenSSL only signing SHA-1 given SNI) was because, at the time, there were concerns over making any ClientHello extensions mandatory. This isn't applicable for CertificateRequest, which can freely advertise their true preferences. Change-Id: I113494d8f66769fde1362795fb08ff2f471ef31d Reviewed-on: https://boringssl-review.googlesource.com/c/33524 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- ssl/t1_lib.cc | 6 +- ssl/test/runner/runner.go | 120 ++++++++++++++++++++------------------ ssl/tls13_client.cc | 1 - 3 files changed, 67 insertions(+), 60 deletions(-) diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index 678e4a3b..00c796ad 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc @@ -1038,7 +1038,6 @@ static bool ext_sigalgs_parse_clienthello(SSL_HANDSHAKE *hs, uint8_t *out_alert, CBS supported_signature_algorithms; if (!CBS_get_u16_length_prefixed(contents, &supported_signature_algorithms) || CBS_len(contents) != 0 || - CBS_len(&supported_signature_algorithms) == 0 || !tls1_parse_peer_sigalgs(hs, &supported_signature_algorithms)) { return false; } @@ -3556,7 +3555,10 @@ bool tls1_parse_peer_sigalgs(SSL_HANDSHAKE *hs, const CBS *in_sigalgs) { return true; } - return parse_u16_array(in_sigalgs, &hs->peer_sigalgs); + // In all contexts, the signature algorithms list may not be empty. (It may be + // omitted by clients in TLS 1.2, but then the entire extension is omitted.) + return CBS_len(in_sigalgs) != 0 && + parse_u16_array(in_sigalgs, &hs->peer_sigalgs); } bool tls1_get_legacy_signature_algorithm(uint16_t *out, const EVP_PKEY *pkey) { diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 6b251a27..da81f236 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -9306,26 +9306,8 @@ func addSignatureAlgorithmTests() { expectedError: ":WRONG_SIGNATURE_TYPE:", }) - // Test that, if the list is missing, the peer falls back to SHA-1 in - // TLS 1.2, but not TLS 1.3. - testCases = append(testCases, testCase{ - name: "ClientAuth-SHA1-Fallback-RSA", - config: Config{ - MaxVersion: VersionTLS12, - ClientAuth: RequireAnyClientCert, - VerifySignatureAlgorithms: []signatureAlgorithm{ - signatureRSAPKCS1WithSHA1, - }, - Bugs: ProtocolBugs{ - NoSignatureAlgorithms: true, - }, - }, - flags: []string{ - "-cert-file", path.Join(*resourceDir, rsaCertificateFile), - "-key-file", path.Join(*resourceDir, rsaKeyFile), - }, - }) - + // Test that, if the ClientHello list is missing, the server falls back + // to SHA-1 in TLS 1.2, but not TLS 1.3. testCases = append(testCases, testCase{ testType: serverTest, name: "ServerAuth-SHA1-Fallback-RSA", @@ -9344,24 +9326,6 @@ func addSignatureAlgorithmTests() { }, }) - testCases = append(testCases, testCase{ - name: "ClientAuth-SHA1-Fallback-ECDSA", - config: Config{ - MaxVersion: VersionTLS12, - ClientAuth: RequireAnyClientCert, - VerifySignatureAlgorithms: []signatureAlgorithm{ - signatureECDSAWithSHA1, - }, - Bugs: ProtocolBugs{ - NoSignatureAlgorithms: true, - }, - }, - flags: []string{ - "-cert-file", path.Join(*resourceDir, ecdsaP256CertificateFile), - "-key-file", path.Join(*resourceDir, ecdsaP256KeyFile), - }, - }) - testCases = append(testCases, testCase{ testType: serverTest, name: "ServerAuth-SHA1-Fallback-ECDSA", @@ -9380,6 +9344,66 @@ func addSignatureAlgorithmTests() { }, }) + testCases = append(testCases, testCase{ + testType: serverTest, + name: "ServerAuth-NoFallback-TLS13", + config: Config{ + MaxVersion: VersionTLS13, + VerifySignatureAlgorithms: []signatureAlgorithm{ + signatureRSAPKCS1WithSHA1, + }, + Bugs: ProtocolBugs{ + NoSignatureAlgorithms: true, + }, + }, + shouldFail: true, + expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:", + }) + + // The CertificateRequest list, however, may never be omitted. It is a + // syntax error for it to be empty. + testCases = append(testCases, testCase{ + name: "ClientAuth-NoFallback-RSA", + config: Config{ + MaxVersion: VersionTLS12, + ClientAuth: RequireAnyClientCert, + VerifySignatureAlgorithms: []signatureAlgorithm{ + signatureRSAPKCS1WithSHA1, + }, + Bugs: ProtocolBugs{ + NoSignatureAlgorithms: true, + }, + }, + flags: []string{ + "-cert-file", path.Join(*resourceDir, rsaCertificateFile), + "-key-file", path.Join(*resourceDir, rsaKeyFile), + }, + shouldFail: true, + expectedError: ":DECODE_ERROR:", + expectedLocalError: "remote error: error decoding message", + }) + + testCases = append(testCases, testCase{ + name: "ClientAuth-NoFallback-ECDSA", + config: Config{ + MaxVersion: VersionTLS12, + ClientAuth: RequireAnyClientCert, + VerifySignatureAlgorithms: []signatureAlgorithm{ + signatureECDSAWithSHA1, + }, + Bugs: ProtocolBugs{ + NoSignatureAlgorithms: true, + }, + }, + flags: []string{ + "-cert-file", path.Join(*resourceDir, ecdsaP256CertificateFile), + "-key-file", path.Join(*resourceDir, ecdsaP256KeyFile), + }, + shouldFail: true, + expectedError: ":DECODE_ERROR:", + expectedLocalError: "remote error: error decoding message", + }) + testCases = append(testCases, testCase{ name: "ClientAuth-NoFallback-TLS13", config: Config{ @@ -9396,29 +9420,11 @@ func addSignatureAlgorithmTests() { "-cert-file", path.Join(*resourceDir, rsaCertificateFile), "-key-file", path.Join(*resourceDir, rsaKeyFile), }, - shouldFail: true, - // An empty CertificateRequest signature algorithm list is a - // syntax error in TLS 1.3. + shouldFail: true, expectedError: ":DECODE_ERROR:", expectedLocalError: "remote error: error decoding message", }) - testCases = append(testCases, testCase{ - testType: serverTest, - name: "ServerAuth-NoFallback-TLS13", - config: Config{ - MaxVersion: VersionTLS13, - VerifySignatureAlgorithms: []signatureAlgorithm{ - signatureRSAPKCS1WithSHA1, - }, - Bugs: ProtocolBugs{ - NoSignatureAlgorithms: true, - }, - }, - shouldFail: true, - expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:", - }) - // Test that signature preferences are enforced. BoringSSL does not // implement MD5 signatures. testCases = append(testCases, testCase{ diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index 0d3e8771..0d778962 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -506,7 +506,6 @@ static enum ssl_hs_wait_t do_read_certificate_request(SSL_HANDSHAKE *hs) { !have_sigalgs || !CBS_get_u16_length_prefixed(&sigalgs, &supported_signature_algorithms) || - CBS_len(&supported_signature_algorithms) == 0 || !tls1_parse_peer_sigalgs(hs, &supported_signature_algorithms)) { ssl_send_alert(ssl, SSL3_AL_FATAL, alert); OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);