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 <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 678e4a3..00c796a 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -1038,7 +1038,6 @@
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 @@
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 6b251a2..da81f23 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -9306,26 +9306,8 @@
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",
@@ -9345,24 +9327,6 @@
})
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",
config: Config{
@@ -9381,6 +9345,66 @@
})
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{
MaxVersion: VersionTLS13,
@@ -9396,29 +9420,11 @@
"-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 0d3e877..0d77896 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -506,7 +506,6 @@
!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);
