From 4087df92f489e3f8bb09439d85aa8edad92ad7fa Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 1 Aug 2016 20:16:31 -0400 Subject: [PATCH] Move more side-specific code out of tls13_process_certificate. tls13_process_certificate can take a boolean for whether anonymous is allowed. This does change the error on the client slightly, but I think this is correct anyway. It is not a syntax error for the server to send no certificates in so far as the Certificate message allows it. It's just illegal. Change-Id: I1af80dacf23f50aad0b1fbd884bc068a40714399 Reviewed-on: https://boringssl-review.googlesource.com/9072 Reviewed-by: Adam Langley Commit-Queue: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org --- ssl/internal.h | 2 +- ssl/test/runner/runner.go | 2 +- ssl/tls13_both.c | 23 ++++++----------------- ssl/tls13_client.c | 2 +- ssl/tls13_server.c | 5 ++++- 5 files changed, 13 insertions(+), 21 deletions(-) diff --git a/ssl/internal.h b/ssl/internal.h index c69bc5fa..f64dc626 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -919,7 +919,7 @@ int tls13_post_handshake(SSL *ssl); * it returns one. Otherwise, it sends an alert and returns zero. */ int tls13_check_message_type(SSL *ssl, int type); -int tls13_process_certificate(SSL *ssl); +int tls13_process_certificate(SSL *ssl, int allow_anonymous); int tls13_process_certificate_verify(SSL *ssl); int tls13_process_finished(SSL *ssl); diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 085d7e19..ee4694bc 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -1430,7 +1430,7 @@ func addBasicTests() { }, }, shouldFail: true, - expectedError: ":DECODE_ERROR:", + expectedError: ":PEER_DID_NOT_RETURN_A_CERTIFICATE:", }, { name: "TLSFatalBadPackets", diff --git a/ssl/tls13_both.c b/ssl/tls13_both.c index dddc08e9..35fea53f 100644 --- a/ssl/tls13_both.c +++ b/ssl/tls13_both.c @@ -181,7 +181,7 @@ err: return 0; } -int tls13_process_certificate(SSL *ssl) { +int tls13_process_certificate(SSL *ssl, int allow_anonymous) { CBS cbs, context; CBS_init(&cbs, ssl->init_msg, ssl->init_num); if (!CBS_get_u8_length_prefixed(&cbs, &context) || @@ -191,12 +191,12 @@ int tls13_process_certificate(SSL *ssl) { return 0; } + const int retain_sha256 = + ssl->server && ssl->ctx->retain_only_sha256_of_client_certs; int ret = 0; uint8_t alert; STACK_OF(X509) *chain = ssl_parse_cert_chain( - ssl, &alert, ssl->ctx->retain_only_sha256_of_client_certs - ? ssl->s3->new_session->peer_sha256 - : NULL, + ssl, &alert, retain_sha256 ? ssl->s3->new_session->peer_sha256 : NULL, &cbs); if (chain == NULL) { ssl3_send_alert(ssl, SSL3_AL_FATAL, alert); @@ -210,15 +210,7 @@ int tls13_process_certificate(SSL *ssl) { } if (sk_X509_num(chain) == 0) { - /* Clients must receive a certificate from the server. */ - if (!ssl->server) { - OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR); - ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); - goto err; - } - - /* Servers may be configured to accept anonymous clients. */ - if (ssl->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) { + if (!allow_anonymous) { OPENSSL_PUT_ERROR(SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE); ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); goto err; @@ -229,10 +221,7 @@ int tls13_process_certificate(SSL *ssl) { goto err; } - if (ssl->server && ssl->ctx->retain_only_sha256_of_client_certs) { - /* The hash was filled in by |ssl_parse_cert_chain|. */ - ssl->s3->new_session->peer_sha256_valid = 1; - } + ssl->s3->new_session->peer_sha256_valid = retain_sha256; if (!ssl_verify_cert_chain(ssl, chain)) { goto err; diff --git a/ssl/tls13_client.c b/ssl/tls13_client.c index 376e0ac3..9dc49232 100644 --- a/ssl/tls13_client.c +++ b/ssl/tls13_client.c @@ -361,7 +361,7 @@ static enum ssl_hs_wait_t do_process_certificate_request(SSL *ssl, static enum ssl_hs_wait_t do_process_server_certificate(SSL *ssl, SSL_HANDSHAKE *hs) { if (!tls13_check_message_type(ssl, SSL3_MT_CERTIFICATE) || - !tls13_process_certificate(ssl) || + !tls13_process_certificate(ssl, 0 /* certificate required */) || !ssl->method->hash_current_message(ssl)) { return ssl_hs_error; } diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c index 1b99ff6e..71c716d3 100644 --- a/ssl/tls13_server.c +++ b/ssl/tls13_server.c @@ -458,8 +458,11 @@ static enum ssl_hs_wait_t do_process_client_certificate(SSL *ssl, return ssl_hs_ok; } + const int allow_anonymous = + (ssl->verify_mode & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) == 0; + if (!tls13_check_message_type(ssl, SSL3_MT_CERTIFICATE) || - !tls13_process_certificate(ssl) || + !tls13_process_certificate(ssl, allow_anonymous) || !ssl->method->hash_current_message(ssl)) { return ssl_hs_error; }