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>

6 files changed, 20 insertions, 15 deletions

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_add_verify_sigalgs(const SSL_HANDSHAKE *hs, CBB *out) {
 }
 
 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 @@ static enum ssl_hs_wait_t do_read_server_key_exchange(SSL_HANDSHAKE *hs) {
         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 @@ static enum ssl_hs_wait_t do_read_client_certificate_verify(SSL_HANDSHAKE *hs) {
       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 tls1_choose_signature_algorithm(SSL_HANDSHAKE *hs,
 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 @@ func addSignatureAlgorithmTests() {
 				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 @@ bool tls13_process_certificate_verify(SSL_HANDSHAKE *hs, const SSLMessage &msg)
   }
 
   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;
   }