Don't accept {sha1, ecdsa} and {sha512, ecdsa}.
{sha1, ecdsa} is virtually nonexistent. {sha512, ecdsa} is pointless
when we only accept P-256 and P-384. See Chromium Intent thread here:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/kWwLfeIQIBM/9chGZ40TCQAJ
This tweaks the signature algorithm logic slightly so that sign and
verify preferences are separate.
BUG=chromium:655318
Change-Id: I1097332600dcaa38e62e4dffa0194fb734c6df3f
Reviewed-on: https://boringssl-review.googlesource.com/11621
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index 83e09d3..e018680 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -1194,8 +1194,8 @@
int have_rsa_sign = 0;
int have_ecdsa_sign = 0;
const uint16_t *sig_algs;
- size_t sig_algs_len = tls12_get_psigalgs(ssl, &sig_algs);
- for (size_t i = 0; i < sig_algs_len; i++) {
+ size_t num_sig_algs = tls12_get_verify_sigalgs(ssl, &sig_algs);
+ for (size_t i = 0; i < num_sig_algs; i++) {
switch (sig_algs[i]) {
case SSL_SIGN_RSA_PKCS1_SHA512:
case SSL_SIGN_RSA_PKCS1_SHA384:
@@ -1242,7 +1242,7 @@
if (ssl3_protocol_version(ssl) >= TLS1_2_VERSION) {
const uint16_t *sigalgs;
- size_t num_sigalgs = tls12_get_psigalgs(ssl, &sigalgs);
+ size_t num_sigalgs = tls12_get_verify_sigalgs(ssl, &sigalgs);
if (!CBB_add_u16_length_prefixed(&body, &sigalgs_cbb)) {
goto err;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index debc5d4..7e9954d 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1111,6 +1111,27 @@
uint16_t ssl_get_grease_value(const SSL *ssl, enum ssl_grease_index_t index);
+/* Signature algorithms. */
+
+/* tls1_parse_peer_sigalgs parses |sigalgs| as the list of peer signature
+ * algorithms and them on |ssl|. It returns one on success and zero on error. */
+int tls1_parse_peer_sigalgs(SSL *ssl, const CBS *sigalgs);
+
+/* tls1_choose_signature_algorithm sets |*out| to a signature algorithm for use
+ * with |ssl|'s private key based on the peer's preferences and the algorithms
+ * supported. It returns one on success and zero on error. */
+int tls1_choose_signature_algorithm(SSL *ssl, uint16_t *out);
+
+/* tls12_get_verify_sigalgs sets |*out| to the signature algorithms acceptable
+ * for the peer signature and returns the length of the list. */
+size_t tls12_get_verify_sigalgs(const SSL *ssl, const uint16_t **out);
+
+/* tls12_check_peer_sigalg checks if |sigalg| is acceptable for the peer
+ * signature. It returns one on success and zero on error, setting |*out_alert|
+ * to an alert to send. */
+int tls12_check_peer_sigalg(SSL *ssl, int *out_alert, uint16_t sigalg);
+
+
/* Underdocumented functions.
*
* Functions below here haven't been touched up and may be underdocumented. */
@@ -1812,20 +1833,7 @@
uint16_t ssl3_protocol_version(const SSL *ssl);
uint32_t ssl_get_algorithm_prf(const SSL *ssl);
-int tls1_parse_peer_sigalgs(SSL *ssl, const CBS *sigalgs);
-/* tls1_choose_signature_algorithm sets |*out| to a signature algorithm for use
- * with |ssl|'s private key based on the peer's preferences and the digests
- * supported. It returns one on success and zero on error. */
-int tls1_choose_signature_algorithm(SSL *ssl, uint16_t *out);
-
-size_t tls12_get_psigalgs(SSL *ssl, const uint16_t **psigs);
-
-/* tls12_check_peer_sigalg checks that |signature_algorithm| is consistent with
- * |ssl|'s sent, supported signature algorithms and returns 1. Otherwise it
- * returns 0 and writes an alert into |*out_alert|. */
-int tls12_check_peer_sigalg(SSL *ssl, int *out_alert,
- uint16_t signature_algorithm);
void ssl_set_client_disabled(SSL *ssl);
void ssl_get_current_time(const SSL *ssl, struct timeval *out_clock);
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 92967de..cfce2d0 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1890,7 +1890,7 @@
}
static const uint8_t kTLS12ClientHello[] = {
- 0x16, 0x03, 0x01, 0x00, 0xa2, 0x01, 0x00, 0x00, 0x9e, 0x03, 0x03, 0x00,
+ 0x16, 0x03, 0x01, 0x00, 0x9e, 0x01, 0x00, 0x00, 0x9a, 0x03, 0x03, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x3a, 0xcc, 0xa9,
@@ -1898,12 +1898,12 @@
0xc0, 0x2c, 0xc0, 0x30, 0x00, 0x9f, 0xc0, 0x09, 0xc0, 0x23, 0xc0, 0x13,
0xc0, 0x27, 0x00, 0x33, 0x00, 0x67, 0xc0, 0x0a, 0xc0, 0x24, 0xc0, 0x14,
0xc0, 0x28, 0x00, 0x39, 0x00, 0x6b, 0x00, 0x9c, 0x00, 0x9d, 0x00, 0x2f,
- 0x00, 0x3c, 0x00, 0x35, 0x00, 0x3d, 0x00, 0x0a, 0x01, 0x00, 0x00, 0x3b,
+ 0x00, 0x3c, 0x00, 0x35, 0x00, 0x3d, 0x00, 0x0a, 0x01, 0x00, 0x00, 0x37,
0xff, 0x01, 0x00, 0x01, 0x00, 0x00, 0x17, 0x00, 0x00, 0x00, 0x23, 0x00,
- 0x00, 0x00, 0x0d, 0x00, 0x18, 0x00, 0x16, 0x08, 0x06, 0x06, 0x01, 0x06,
- 0x03, 0x08, 0x05, 0x05, 0x01, 0x05, 0x03, 0x08, 0x04, 0x04, 0x01, 0x04,
- 0x03, 0x02, 0x01, 0x02, 0x03, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00,
- 0x0a, 0x00, 0x08, 0x00, 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18,
+ 0x00, 0x00, 0x0d, 0x00, 0x14, 0x00, 0x12, 0x08, 0x06, 0x06, 0x01, 0x08,
+ 0x05, 0x05, 0x01, 0x05, 0x03, 0x08, 0x04, 0x04, 0x01, 0x04, 0x03, 0x02,
+ 0x01, 0x00, 0x0b, 0x00, 0x02, 0x01, 0x00, 0x00, 0x0a, 0x00, 0x08, 0x00,
+ 0x06, 0x00, 0x1d, 0x00, 0x17, 0x00, 0x18,
};
if (!ClientHelloMatches(TLS1_2_VERSION, kTLS12ClientHello,
sizeof(kTLS12ClientHello))) {
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 3c9f5ba..9655b83 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -441,11 +441,41 @@
return 0;
}
-/* List of supported signature algorithms and hashes. Should make this
- * customisable at some point, for now include everything we support. */
+/* kVerifySignatureAlgorithms is the default list of accepted signature
+ * algorithms for verifying. */
+static const uint16_t kVerifySignatureAlgorithms[] = {
+ /* For now, do not enable RSA-PSS signature algorithms on Android's system
+ * BoringSSL. Once TLS 1.3 is finalized and the change in Chrome has stuck,
+ * restore them. */
+#if !defined(BORINGSSL_ANDROID_SYSTEM)
+ SSL_SIGN_RSA_PSS_SHA512,
+#endif
+ SSL_SIGN_RSA_PKCS1_SHA512,
+ /* TODO(davidben): Remove this entry and SSL_CURVE_SECP521R1 from
+ * kDefaultGroups. */
+#if defined(BORINGSSL_ANDROID_SYSTEM)
+ SSL_SIGN_ECDSA_SECP521R1_SHA512,
+#endif
-static const uint16_t kDefaultSignatureAlgorithms[] = {
- /* For now, do not ship RSA-PSS signature algorithms on Android's system
+#if !defined(BORINGSSL_ANDROID_SYSTEM)
+ SSL_SIGN_RSA_PSS_SHA384,
+#endif
+ SSL_SIGN_RSA_PKCS1_SHA384,
+ SSL_SIGN_ECDSA_SECP384R1_SHA384,
+
+#if !defined(BORINGSSL_ANDROID_SYSTEM)
+ SSL_SIGN_RSA_PSS_SHA256,
+#endif
+ SSL_SIGN_RSA_PKCS1_SHA256,
+ SSL_SIGN_ECDSA_SECP256R1_SHA256,
+
+ SSL_SIGN_RSA_PKCS1_SHA1,
+};
+
+/* kSignSignatureAlgorithms is the default list of supported signature
+ * algorithms for signing. */
+static const uint16_t kSignSignatureAlgorithms[] = {
+ /* For now, do not enable RSA-PSS signature algorithms on Android's system
* BoringSSL. Once TLS 1.3 is finalized and the change in Chrome has stuck,
* restore them. */
#if !defined(BORINGSSL_ANDROID_SYSTEM)
@@ -470,30 +500,23 @@
SSL_SIGN_ECDSA_SHA1,
};
-size_t tls12_get_psigalgs(SSL *ssl, const uint16_t **psigs) {
- *psigs = kDefaultSignatureAlgorithms;
- return OPENSSL_ARRAY_SIZE(kDefaultSignatureAlgorithms);
+size_t tls12_get_verify_sigalgs(const SSL *ssl, const uint16_t **out) {
+ *out = kVerifySignatureAlgorithms;
+ return OPENSSL_ARRAY_SIZE(kVerifySignatureAlgorithms);
}
int tls12_check_peer_sigalg(SSL *ssl, int *out_alert, uint16_t sigalg) {
- const uint16_t *sent_sigs;
- size_t sent_sigslen, i;
-
- /* Check signature matches a type we sent */
- sent_sigslen = tls12_get_psigalgs(ssl, &sent_sigs);
- for (i = 0; i < sent_sigslen; i++) {
- if (sigalg == sent_sigs[i]) {
- break;
+ const uint16_t *verify_sigalgs;
+ size_t num_verify_sigalgs = tls12_get_verify_sigalgs(ssl, &verify_sigalgs);
+ for (size_t i = 0; i < num_verify_sigalgs; i++) {
+ if (sigalg == verify_sigalgs[i]) {
+ return 1;
}
}
- if (i == sent_sigslen) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_SIGNATURE_TYPE);
- *out_alert = SSL_AD_ILLEGAL_PARAMETER;
- return 0;
- }
-
- return 1;
+ OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_SIGNATURE_TYPE);
+ *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+ return 0;
}
/* Get a mask of disabled algorithms: an algorithm is disabled if it isn't
@@ -506,10 +529,10 @@
c->mask_a = 0;
c->mask_k = 0;
- /* Now go through all signature algorithms seeing if we support any for RSA,
- * DSA, ECDSA. Do this for all versions not just TLS 1.2. */
+ /* Now go through all signature algorithms seeing if we support any for RSA or
+ * ECDSA. Do this for all versions not just TLS 1.2. */
const uint16_t *sigalgs;
- size_t num_sigalgs = tls12_get_psigalgs(ssl, &sigalgs);
+ size_t num_sigalgs = tls12_get_verify_sigalgs(ssl, &sigalgs);
for (size_t i = 0; i < num_sigalgs; i++) {
switch (sigalgs[i]) {
case SSL_SIGN_RSA_PSS_SHA512:
@@ -1052,7 +1075,7 @@
}
const uint16_t *sigalgs;
- const size_t num_sigalgs = tls12_get_psigalgs(ssl, &sigalgs);
+ const size_t num_sigalgs = tls12_get_verify_sigalgs(ssl, &sigalgs);
CBB contents, sigalgs_cbb;
if (!CBB_add_u16(out, TLSEXT_TYPE_signature_algorithms) ||
@@ -3144,11 +3167,11 @@
return 0;
}
- const uint16_t *sigalgs;
- size_t num_sigalgs = tls12_get_psigalgs(ssl, &sigalgs);
- if (cert->sigalgs != NULL) {
- sigalgs = cert->sigalgs;
- num_sigalgs = cert->num_sigalgs;
+ const uint16_t *sigalgs = cert->sigalgs;
+ size_t num_sigalgs = cert->num_sigalgs;
+ if (sigalgs == NULL) {
+ sigalgs = kSignSignatureAlgorithms;
+ num_sigalgs = OPENSSL_ARRAY_SIZE(kSignSignatureAlgorithms);
}
const uint16_t *peer_sigalgs = hs->peer_sigalgs;
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 77f9a0d..367fef1 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -5884,19 +5884,28 @@
continue
}
- var shouldFail bool
+ var shouldSignFail, shouldVerifyFail bool
// ecdsa_sha1 does not exist in TLS 1.3.
if ver.version >= VersionTLS13 && alg.id == signatureECDSAWithSHA1 {
- shouldFail = true
+ shouldSignFail = true
+ shouldVerifyFail = true
}
// RSA-PKCS1 does not exist in TLS 1.3.
if ver.version == VersionTLS13 && hasComponent(alg.name, "PKCS1") {
- shouldFail = true
+ shouldSignFail = true
+ shouldVerifyFail = true
+ }
+
+ // BoringSSL will sign SHA-1 and SHA-512 with ECDSA but not accept them.
+ if alg.id == signatureECDSAWithSHA1 || alg.id == signatureECDSAWithP521AndSHA512 {
+ shouldVerifyFail = true
}
var signError, verifyError string
- if shouldFail {
+ if shouldSignFail {
signError = ":NO_COMMON_SIGNATURE_ALGORITHMS:"
+ }
+ if shouldVerifyFail {
verifyError = ":WRONG_SIGNATURE_TYPE:"
}
@@ -5918,7 +5927,7 @@
"-key-file", path.Join(*resourceDir, getShimKey(alg.cert)),
"-enable-all-curves",
},
- shouldFail: shouldFail,
+ shouldFail: shouldSignFail,
expectedError: signError,
expectedPeerSignatureAlgorithm: alg.id,
})
@@ -5933,11 +5942,10 @@
alg.id,
},
Bugs: ProtocolBugs{
- SkipECDSACurveCheck: shouldFail,
- IgnoreSignatureVersionChecks: shouldFail,
- // The client won't advertise 1.3-only algorithms after
- // version negotiation.
- IgnorePeerSignatureAlgorithmPreferences: shouldFail,
+ SkipECDSACurveCheck: shouldVerifyFail,
+ IgnoreSignatureVersionChecks: shouldVerifyFail,
+ // Some signature algorithms may not be advertised.
+ IgnorePeerSignatureAlgorithmPreferences: shouldVerifyFail,
},
},
flags: []string{
@@ -5945,7 +5953,7 @@
"-expect-peer-signature-algorithm", strconv.Itoa(int(alg.id)),
"-enable-all-curves",
},
- shouldFail: shouldFail,
+ shouldFail: shouldVerifyFail,
expectedError: verifyError,
})
@@ -5966,7 +5974,7 @@
"-key-file", path.Join(*resourceDir, getShimKey(alg.cert)),
"-enable-all-curves",
},
- shouldFail: shouldFail,
+ shouldFail: shouldSignFail,
expectedError: signError,
expectedPeerSignatureAlgorithm: alg.id,
})
@@ -5981,19 +5989,21 @@
alg.id,
},
Bugs: ProtocolBugs{
- SkipECDSACurveCheck: shouldFail,
- IgnoreSignatureVersionChecks: shouldFail,
+ SkipECDSACurveCheck: shouldVerifyFail,
+ IgnoreSignatureVersionChecks: shouldVerifyFail,
+ // Some signature algorithms may not be advertised.
+ IgnorePeerSignatureAlgorithmPreferences: shouldVerifyFail,
},
},
flags: []string{
"-expect-peer-signature-algorithm", strconv.Itoa(int(alg.id)),
"-enable-all-curves",
},
- shouldFail: shouldFail,
+ shouldFail: shouldVerifyFail,
expectedError: verifyError,
})
- if !shouldFail {
+ if !shouldVerifyFail {
testCases = append(testCases, testCase{
testType: serverTest,
name: "ClientAuth-InvalidSignature" + suffix,
@@ -6034,7 +6044,7 @@
})
}
- if ver.version >= VersionTLS12 && !shouldFail {
+ if ver.version >= VersionTLS12 && !shouldSignFail {
testCases = append(testCases, testCase{
name: "ClientAuth-Sign-Negotiate" + suffix,
config: Config{
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index cf3c9f7..2570069 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -381,7 +381,7 @@
}
const uint16_t *sigalgs;
- size_t num_sigalgs = tls12_get_psigalgs(ssl, &sigalgs);
+ size_t num_sigalgs = tls12_get_verify_sigalgs(ssl, &sigalgs);
if (!CBB_add_u16_length_prefixed(&body, &sigalgs_cbb)) {
goto err;
}
