commit f4958e767817f2ba77cece3031ddcf87916d57cf
author David Benjamin <davidben@chromium.org> Sun Jun 07 11:00:53 2015 -0400
committer Adam Langley <agl@google.com> Mon Jun 08 22:14:53 2015 +0000
tree b2ad565dfe0ee45d131ee582075d14c48516941c
parent b31040d0d8bbfa563a39e2321131074816d9d21b

Slightly simplify ServerKeyExchange handling.

The current logic requires each key exchange extract the key. It also
leaves handling X509_get_pubkey failure to the anonymous cipher suite
case which has an escape hatch where it goes back to check
ssl3_check_cert_and_algorithm.

Instead, get the key iff we know we have a signature to check.

Change-Id: If7154c7156aad3b89489defe4c1d951eeebf0089
Reviewed-on: https://boringssl-review.googlesource.com/5045
Reviewed-by: Adam Langley <agl@google.com>

ssl/s3_clnt.c

diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index d7ed14f..e1b8eb0 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1191,12 +1191,6 @@
                         SSL_R_BAD_DH_P_LENGTH);
       goto err;
     }
-
-    if (alg_a & SSL_aRSA) {
-      pkey = X509_get_pubkey(s->session->sess_cert->peer_cert);
-    }
-    /* else anonymous DH, so no certificate or pkey. */
-
     s->session->sess_cert->peer_dh_tmp = dh;
     dh = NULL;
   } else if (alg_k & SSL_kECDHE) {
@@ -1251,16 +1245,6 @@
       OPENSSL_PUT_ERROR(SSL, ssl3_get_server_key_exchange, SSL_R_BAD_ECPOINT);
       goto f_err;
     }
-
-    /* The ECC/TLS specification does not mention the use of DSA to sign
-     * ECParameters in the server key exchange message. We do support RSA and
-     * ECDSA. */
-    if (alg_a & (SSL_aRSA|SSL_aECDSA)) {
-      pkey = X509_get_pubkey(s->session->sess_cert->peer_cert);
-    } else {
-      /* Otherwise this is ECDHE_PSK, so no public key. */
-      assert(alg_a == SSL_aPSK);
-    }
     EC_KEY_set_public_key(ecdh, srvr_ecpoint);
     s->session->sess_cert->peer_ecdh_tmp = ecdh;
     ecdh = NULL;
@@ -1281,9 +1265,12 @@
   CBS_init(&parameter, CBS_data(&server_key_exchange_orig),
            CBS_len(&server_key_exchange_orig) - CBS_len(&server_key_exchange));
 
-  /* if it was signed, check the signature */
-  if (pkey != NULL) {
-    CBS signature;
+  /* ServerKeyExchange should be signed by the server's public key. */
+  if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher)) {
+    pkey = X509_get_pubkey(s->session->sess_cert->peer_cert);
+    if (pkey == NULL) {
+      goto err;
+    }
 
     if (SSL_USE_SIGALGS(s)) {
       if (!tls12_check_peer_sigalg(&md, &al, s, &server_key_exchange, pkey)) {
@@ -1296,6 +1283,7 @@
     }
 
     /* The last field in |server_key_exchange| is the signature. */
+    CBS signature;
     if (!CBS_get_u16_length_prefixed(&server_key_exchange, &signature) ||
         CBS_len(&server_key_exchange) != 0) {
       al = SSL_AD_DECODE_ERROR;
@@ -1318,16 +1306,9 @@
       goto f_err;
     }
   } else {
-    if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher)) {
-      /* Might be wrong key type, check it */
-      if (ssl3_check_cert_and_algorithm(s)) {
-        /* Otherwise this shouldn't happen */
-        OPENSSL_PUT_ERROR(SSL, ssl3_get_server_key_exchange,
-                          ERR_R_INTERNAL_ERROR);
-      }
-      goto err;
-    }
-    /* still data left over */
+    /* PSK ciphers are the only supported certificate-less ciphers. */
+    assert(alg_a == SSL_aPSK);
+
     if (CBS_len(&server_key_exchange) > 0) {
       al = SSL_AD_DECODE_ERROR;
       OPENSSL_PUT_ERROR(SSL, ssl3_get_server_key_exchange,