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);