Be strict about expecting a server Certificate message.

Introduce a ssl_cipher_has_server_public_key to save the repeated
NULL/PSK/RSA_PSK[*] check. Don't allow skipping to ServerKeyExchange when
expecting Certificate; the messages expected are determined by the cipher
suite. The ssl3_get_server_public_key call is already guarded.

As the previous test demonstrates, this is safe because of the
ssl3_check_cert_and_algorithm call, but avoid the looseness in the parsing
there.

[*] NB: we don't implement RSA_PSK, and OpenSSL has never implemented it.

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

5 files changed, 28 insertions, 35 deletions

diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index 35c399ff..6c10a32b 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -313,11 +313,7 @@ int ssl3_connect(SSL *s)
 				s->init_num=0;
 				break;
 				}
-			/* Check if it is anon DH/ECDH */
-			/* or non-RSA PSK */
-			if (!(s->s3->tmp.new_cipher->algorithm_auth & SSL_aNULL) &&
-			    !((s->s3->tmp.new_cipher->algorithm_auth & SSL_aPSK) &&
-			      !(s->s3->tmp.new_cipher->algorithm_mkey & SSL_kRSA)))
+			if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher))
 				{
 				ret=ssl3_get_server_certificate(s);
 				if (ret <= 0) goto end;
@@ -1118,24 +1114,12 @@ int ssl3_get_server_certificate(SSL *s)
 	n=s->method->ssl_get_message(s,
 		SSL3_ST_CR_CERT_A,
 		SSL3_ST_CR_CERT_B,
-		-1,
+		SSL3_MT_CERTIFICATE,
 		s->max_cert_list,
 		&ok);
 
 	if (!ok) return((int)n);
 
-	if (s->s3->tmp.message_type == SSL3_MT_SERVER_KEY_EXCHANGE)
-		{
-		s->s3->tmp.reuse_message=1;
-		return(1);
-		}
-
-	if (s->s3->tmp.message_type != SSL3_MT_CERTIFICATE)
-		{
-		al=SSL_AD_UNEXPECTED_MESSAGE;
-		OPENSSL_PUT_ERROR(SSL, ssl3_get_server_certificate, SSL_R_BAD_MESSAGE_TYPE);
-		goto f_err;
-		}
 	CBS_init(&cbs, (uint8_t *)s->init_msg, n);
 
 	if ((sk=sk_X509_new_null()) == NULL)
@@ -1732,9 +1716,7 @@ fprintf(stderr, "USING TLSv1.2 HASH %s\n", EVP_MD_name(md));
 		}
 	else
 		{
-		if (!(alg_a & SSL_aNULL) &&
-			/* Among PSK ciphers only RSA_PSK needs a public key */
-			!((alg_a & SSL_aPSK) && !(alg_k & SSL_kRSA)))
+		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))
@@ -2916,13 +2898,13 @@ int ssl3_check_cert_and_algorithm(SSL *s)
 	DH *dh;
 #endif
 
+	/* we don't have a certificate */
+	if (!ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher))
+		return 1;
+
 	alg_k=s->s3->tmp.new_cipher->algorithm_mkey;
 	alg_a=s->s3->tmp.new_cipher->algorithm_auth;
 
-	/* we don't have a certificate */
-	if ((alg_a & SSL_aNULL) || ((alg_a & SSL_aPSK) && !(alg_k & SSL_kRSA)))
-		return(1);
-
 	sc=s->session->sess_cert;
 	if (sc == NULL)
 		{
diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c
index 2a57edf0..c8d5d2b5 100644
--- a/ssl/s3_srvr.c
+++ b/ssl/s3_srvr.c
@@ -351,12 +351,7 @@ int ssl3_accept(SSL *s)
 
 		case SSL3_ST_SW_CERT_A:
 		case SSL3_ST_SW_CERT_B:
-			/* Check if it is anon DH or anon ECDH, */
-			/* non-RSA PSK or SRP */
-			if (!(s->s3->tmp.new_cipher->algorithm_auth & SSL_aNULL)
-				/* Among PSK ciphersuites only RSA_PSK uses server certificate */
-				&& !(s->s3->tmp.new_cipher->algorithm_auth & SSL_aPSK &&
-					 !(s->s3->tmp.new_cipher->algorithm_mkey & SSL_kRSA)))
+			if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher))
 				{
 				ret=ssl3_send_server_certificate(s);
 				if (ret <= 0) goto end;
@@ -1776,9 +1771,7 @@ int ssl3_send_server_key_exchange(SSL *s)
 			n+=2+nr[i];
 			}
 
-		if (!(alg_a & SSL_aNULL)
-			/* Among PSK ciphersuites only RSA uses a certificate */
-			&& !((alg_a & SSL_aPSK) && !(alg_k & SSL_kRSA)))
+		if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher))
 			{
 			if ((pkey=ssl_get_sign_pkey(s,s->s3->tmp.new_cipher,&md))
 				== NULL)
diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c
index d7f3fbaf..4c4bb67c 100644
--- a/ssl/ssl_ciph.c
+++ b/ssl/ssl_ciph.c
@@ -1867,3 +1867,19 @@ const SSL_CIPHER *SSL_CIPHER_find(SSL *ssl, const unsigned char *ptr)
 	{
 	return ssl->method->get_cipher_by_char(ptr);
 	}
+
+/* ssl_cipher_has_server_public_key returns 1 if |cipher| involves a
+ * server public key in the key exchange, sent in a server Certificate
+ * message. Otherwise it returns 0. */
+int ssl_cipher_has_server_public_key(const SSL_CIPHER *cipher)
+	{
+	/* Anonymous ciphers do not include a server certificate. */
+	if (cipher->algorithm_auth & SSL_aNULL)
+		return 0;
+	/* Neither do PSK ciphers, except for RSA_PSK. */
+	if ((cipher->algorithm_auth & SSL_aPSK) &&
+		!(cipher->algorithm_mkey & SSL_kRSA))
+		return 0;
+	/* All other ciphers include it. */
+	return 1;
+	}
diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h
index 63e7dccc..ed3a0b5f 100644
--- a/ssl/ssl_locl.h
+++ b/ssl/ssl_locl.h
@@ -993,6 +993,8 @@ int ssl_get_handshake_digest(int i,long *mask,const EVP_MD **md);
 int ssl_get_handshake_digest(int i,long *mask,const EVP_MD **md);
 int ssl_cipher_get_cert_index(const SSL_CIPHER *c);
 const SSL_CIPHER *ssl_get_cipher_by_char(SSL *ssl, const unsigned char *ptr);
+int ssl_cipher_has_server_public_key(const SSL_CIPHER *cipher);
+
 int ssl_cert_set0_chain(CERT *c, STACK_OF(X509) *chain);
 int ssl_cert_set1_chain(CERT *c, STACK_OF(X509) *chain);
 int ssl_cert_add0_chain_cert(CERT *c, X509 *x);
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 0b210cd6..1207e9ad 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -198,7 +198,7 @@ var testCases = []testCase{
 			},
 		},
 		shouldFail:    true,
-		expectedError: ":MISSING_RSA_SIGNING_CERT:",
+		expectedError: ":UNEXPECTED_MESSAGE:",
 	},
 }