Send a consistent alert when the peer sends a bad signature algorithm
I noticed that runner tests had a very weird test expectation on the
alerts sent around sigalg failures. I think this was an (unimportant)
bug on our end.
If the peer picks a sigalg that we didn't advertise, we send
illegal_parameter. However, it if picks an advertised sigalg that is
invalid in context (protocol version, public key), we end up catching it
very late in ssl_public_key_verify (by way of setup_ctx) and sending
decrypt_error.
Instead, have tls12_check_peer_sigalg check this so we consistently send
illegal_parameter. (Probably this should all fold into
ssl_public_key_verify with an alert out_param, but so it goes.)
Change-Id: I09fb84e9c1ee39b2683fa0b67dd6135d31f51c97
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/69367
Commit-Queue: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Bob Beck <bbe@google.com>
diff --git a/ssl/extensions.cc b/ssl/extensions.cc
index 8b2de59..586edbd 100644
--- a/ssl/extensions.cc
+++ b/ssl/extensions.cc
@@ -441,16 +441,18 @@
}
bool tls12_check_peer_sigalg(const SSL_HANDSHAKE *hs, uint8_t *out_alert,
- uint16_t sigalg) {
- for (uint16_t verify_sigalg : tls12_get_verify_sigalgs(hs)) {
- if (verify_sigalg == sigalg) {
- return true;
- }
+ uint16_t sigalg, EVP_PKEY *pkey) {
+ // The peer must have selected an algorithm that is consistent with its public
+ // key, the TLS version, and what we advertised.
+ Span<const uint16_t> sigalgs = tls12_get_verify_sigalgs(hs);
+ if (std::find(sigalgs.begin(), sigalgs.end(), sigalg) == sigalgs.end() ||
+ !ssl_pkey_supports_algorithm(hs->ssl, pkey, sigalg)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_SIGNATURE_TYPE);
+ *out_alert = SSL_AD_ILLEGAL_PARAMETER;
+ return false;
}
- OPENSSL_PUT_ERROR(SSL, SSL_R_WRONG_SIGNATURE_TYPE);
- *out_alert = SSL_AD_ILLEGAL_PARAMETER;
- return false;
+ return true;
}
// tls_extension represents a TLS extension that is handled internally.
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index b958dce..f674515 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1169,7 +1169,8 @@
return ssl_hs_error;
}
uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!tls12_check_peer_sigalg(hs, &alert, signature_algorithm)) {
+ if (!tls12_check_peer_sigalg(hs, &alert, signature_algorithm,
+ hs->peer_pubkey.get())) {
ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
}
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 1a25ea7..afa0927 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -1650,7 +1650,8 @@
return ssl_hs_error;
}
uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!tls12_check_peer_sigalg(hs, &alert, signature_algorithm)) {
+ if (!tls12_check_peer_sigalg(hs, &alert, signature_algorithm,
+ hs->peer_pubkey.get())) {
ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
}
diff --git a/ssl/internal.h b/ssl/internal.h
index 5744dfe..7145d13 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -2454,10 +2454,10 @@
bool tls12_add_verify_sigalgs(const SSL_HANDSHAKE *hs, CBB *out);
// tls12_check_peer_sigalg checks if |sigalg| is acceptable for the peer
-// signature. It returns true on success and false on error, setting
+// signature from |pkey|. It returns true on success and false on error, setting
// |*out_alert| to an alert to send.
bool tls12_check_peer_sigalg(const SSL_HANDSHAKE *hs, uint8_t *out_alert,
- uint16_t sigalg);
+ uint16_t sigalg, EVP_PKEY *pkey);
// Underdocumented functions.
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 39b7611..2e1b407 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -10105,12 +10105,12 @@
signError = ":NO_COMMON_SIGNATURE_ALGORITHMS:"
signLocalError = "remote error: handshake failure"
verifyError = ":WRONG_SIGNATURE_TYPE:"
- verifyLocalError = "remote error"
+ verifyLocalError = "remote error: illegal parameter"
rejectByDefault = true
}
if rejectByDefault {
defaultError = ":WRONG_SIGNATURE_TYPE:"
- defaultLocalError = "remote error"
+ defaultLocalError = "remote error: illegal parameter"
}
suffix := "-" + alg.name + "-" + ver.name
diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc
index 4a9b78e..f6d9547 100644
--- a/ssl/tls13_both.cc
+++ b/ssl/tls13_both.cc
@@ -335,7 +335,8 @@
}
uint8_t alert = SSL_AD_DECODE_ERROR;
- if (!tls12_check_peer_sigalg(hs, &alert, signature_algorithm)) {
+ if (!tls12_check_peer_sigalg(hs, &alert, signature_algorithm,
+ hs->peer_pubkey.get())) {
ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return false;
}
