diff options
author | David Benjamin <davidben@chromium.org> | 2014-07-12 09:10:19 +0400 |
---|---|---|
committer | Adam Langley <agl@google.com> | 2014-07-15 00:50:30 +0400 |
commit | e8f3d666ccf4495fffc5aa9d7ac8ee3356bb980d (patch) | |
tree | f16a469b1926c627db8beb1c23591ee0832019c6 | |
parent | 1c375dda8a254d762f1439e22489b505a2a051bd (diff) |
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>
-rw-r--r-- | ssl/s3_clnt.c | 32 | ||||
-rw-r--r-- | ssl/s3_srvr.c | 11 | ||||
-rw-r--r-- | ssl/ssl_ciph.c | 16 | ||||
-rw-r--r-- | ssl/ssl_locl.h | 2 | ||||
-rw-r--r-- | ssl/test/runner/runner.go | 2 |
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:", }, } |