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 <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/internal.h b/ssl/internal.h
index c69bc5f..f64dc62 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -919,7 +919,7 @@
* 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 085d7e1..ee4694b 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -1430,7 +1430,7 @@
},
},
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 dddc08e..35fea53 100644
--- a/ssl/tls13_both.c
+++ b/ssl/tls13_both.c
@@ -181,7 +181,7 @@
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 @@
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 @@
}
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 @@
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 376e0ac..9dc49232 100644
--- a/ssl/tls13_client.c
+++ b/ssl/tls13_client.c
@@ -361,7 +361,7 @@
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 1b99ff6..71c716d 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -458,8 +458,11 @@
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;
}
