Return null from SSL_get0_peer_certificates if unauthenticated.
SSL_get0_peer_certificates is documented to return NULL if the peer was
anonymous, but it actually returns a non-NULL empty list (except in SSL
3.0 where the Certificate message and thus ssl_parse_cert_chain is
skipped).
Make the implementation match the documentation.
Change-Id: Ib3e25d2155f316cc5e9eb3ab7f74b78e08b8a86b
Reviewed-on: https://boringssl-review.googlesource.com/18226
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 260d3cd0..32714d1 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1087,15 +1087,14 @@
CBS_init(&cbs, ssl->init_msg, ssl->init_num);
uint8_t alert = SSL_AD_DECODE_ERROR;
- sk_CRYPTO_BUFFER_pop_free(hs->new_session->certs, CRYPTO_BUFFER_free);
- hs->peer_pubkey.reset();
- hs->new_session->certs =
- ssl_parse_cert_chain(&alert, &hs->peer_pubkey, NULL, &cbs, ssl->ctx->pool)
- .release();
- if (hs->new_session->certs == NULL) {
+ UniquePtr<STACK_OF(CRYPTO_BUFFER)> chain;
+ if (!ssl_parse_cert_chain(&alert, &chain, &hs->peer_pubkey, NULL, &cbs,
+ ssl->ctx->pool)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
return -1;
}
+ sk_CRYPTO_BUFFER_pop_free(hs->new_session->certs, CRYPTO_BUFFER_free);
+ hs->new_session->certs = chain.release();
if (sk_CRYPTO_BUFFER_num(hs->new_session->certs) == 0 ||
CBS_len(&cbs) != 0 ||
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 38fbef4..44c3b01 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -1228,20 +1228,18 @@
CBS certificate_msg;
CBS_init(&certificate_msg, ssl->init_msg, ssl->init_num);
- sk_CRYPTO_BUFFER_pop_free(hs->new_session->certs, CRYPTO_BUFFER_free);
- hs->peer_pubkey.reset();
uint8_t alert = SSL_AD_DECODE_ERROR;
- hs->new_session->certs =
- ssl_parse_cert_chain(&alert, &hs->peer_pubkey,
- ssl->retain_only_sha256_of_client_certs
- ? hs->new_session->peer_sha256
- : NULL,
- &certificate_msg, ssl->ctx->pool)
- .release();
- if (hs->new_session->certs == NULL) {
+ UniquePtr<STACK_OF(CRYPTO_BUFFER)> chain;
+ if (!ssl_parse_cert_chain(&alert, &chain, &hs->peer_pubkey,
+ ssl->retain_only_sha256_of_client_certs
+ ? hs->new_session->peer_sha256
+ : NULL,
+ &certificate_msg, ssl->ctx->pool)) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, alert);
return -1;
}
+ sk_CRYPTO_BUFFER_pop_free(hs->new_session->certs, CRYPTO_BUFFER_free);
+ hs->new_session->certs = chain.release();
if (CBS_len(&certificate_msg) != 0 ||
!ssl->ctx->x509_method->session_cache_objects(hs->new_session.get())) {
diff --git a/ssl/internal.h b/ssl/internal.h
index 5a2e560..085beef 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -920,18 +920,21 @@
int ssl_has_certificate(const SSL *ssl);
/* ssl_parse_cert_chain parses a certificate list from |cbs| in the format used
- * by a TLS Certificate message. On success, it returns a newly-allocated
- * |CRYPTO_BUFFER| list and advances |cbs|. Otherwise, it returns nullptr and
- * sets |*out_alert| to an alert to send to the peer.
+ * by a TLS Certificate message. On success, it advances |cbs| and returns
+ * true. Otherwise, it returns false and sets |*out_alert| to an alert to send
+ * to the peer.
*
- * If the list is non-empty then |*out_pubkey| will be set to a freshly
- * allocated public-key from the leaf certificate.
+ * If the list is non-empty then |*out_chain| and |*out_pubkey| will be set to
+ * the certificate chain and the leaf certificate's public key
+ * respectively. Otherwise, both will be set to nullptr.
*
* If the list is non-empty and |out_leaf_sha256| is non-NULL, it writes the
* SHA-256 hash of the leaf to |out_leaf_sha256|. */
-UniquePtr<STACK_OF(CRYPTO_BUFFER)> ssl_parse_cert_chain(
- uint8_t *out_alert, UniquePtr<EVP_PKEY> *out_pubkey,
- uint8_t *out_leaf_sha256, CBS *cbs, CRYPTO_BUFFER_POOL *pool);
+bool ssl_parse_cert_chain(uint8_t *out_alert,
+ UniquePtr<STACK_OF(CRYPTO_BUFFER)> *out_chain,
+ UniquePtr<EVP_PKEY> *out_pubkey,
+ uint8_t *out_leaf_sha256, CBS *cbs,
+ CRYPTO_BUFFER_POOL *pool);
/* ssl_add_cert_chain adds |ssl|'s certificate chain to |cbb| in the format used
* by a TLS Certificate message. If there is no certificate chain, it emits an
diff --git a/ssl/ssl_cert.cc b/ssl/ssl_cert.cc
index 4f457f6..0fcf48d 100644
--- a/ssl/ssl_cert.cc
+++ b/ssl/ssl_cert.cc
@@ -382,39 +382,47 @@
ssl_has_private_key(ssl);
}
-UniquePtr<STACK_OF(CRYPTO_BUFFER)>
- ssl_parse_cert_chain(uint8_t *out_alert, UniquePtr<EVP_PKEY> *out_pubkey,
- uint8_t *out_leaf_sha256, CBS *cbs,
- CRYPTO_BUFFER_POOL *pool) {
- UniquePtr<EVP_PKEY> pubkey;
- UniquePtr<STACK_OF(CRYPTO_BUFFER)> ret(sk_CRYPTO_BUFFER_new_null());
- if (!ret) {
- *out_alert = SSL_AD_INTERNAL_ERROR;
- OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- return nullptr;
- }
+bool ssl_parse_cert_chain(uint8_t *out_alert,
+ UniquePtr<STACK_OF(CRYPTO_BUFFER)> *out_chain,
+ UniquePtr<EVP_PKEY> *out_pubkey,
+ uint8_t *out_leaf_sha256, CBS *cbs,
+ CRYPTO_BUFFER_POOL *pool) {
+ out_chain->reset();
+ out_pubkey->reset();
CBS certificate_list;
if (!CBS_get_u24_length_prefixed(cbs, &certificate_list)) {
*out_alert = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- return nullptr;
+ return false;
}
+ if (CBS_len(&certificate_list) == 0) {
+ return true;
+ }
+
+ UniquePtr<STACK_OF(CRYPTO_BUFFER)> chain(sk_CRYPTO_BUFFER_new_null());
+ if (!chain) {
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+ return false;
+ }
+
+ UniquePtr<EVP_PKEY> pubkey;
while (CBS_len(&certificate_list) > 0) {
CBS certificate;
if (!CBS_get_u24_length_prefixed(&certificate_list, &certificate) ||
CBS_len(&certificate) == 0) {
*out_alert = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_CERT_LENGTH_MISMATCH);
- return nullptr;
+ return false;
}
- if (sk_CRYPTO_BUFFER_num(ret.get()) == 0) {
+ if (sk_CRYPTO_BUFFER_num(chain.get()) == 0) {
pubkey = ssl_cert_parse_pubkey(&certificate);
if (!pubkey) {
*out_alert = SSL_AD_DECODE_ERROR;
- return nullptr;
+ return false;
}
/* Retain the hash of the leaf certificate if requested. */
@@ -426,16 +434,17 @@
CRYPTO_BUFFER *buf =
CRYPTO_BUFFER_new_from_CBS(&certificate, pool);
if (buf == NULL ||
- !sk_CRYPTO_BUFFER_push(ret.get(), buf)) {
+ !sk_CRYPTO_BUFFER_push(chain.get(), buf)) {
*out_alert = SSL_AD_INTERNAL_ERROR;
CRYPTO_BUFFER_free(buf);
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
- return nullptr;
+ return false;
}
}
+ *out_chain = std::move(chain);
*out_pubkey = std::move(pubkey);
- return ret;
+ return true;
}
int ssl_add_cert_chain(SSL *ssl, CBB *cbb) {
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index b410175..48e2181 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -1716,14 +1716,16 @@
return false;
}
- // However, for historical reasons, the chain includes the leaf on the
+ // However, for historical reasons, the X509 chain includes the leaf on the
// client, but does not on the server.
- if (sk_X509_num(SSL_get_peer_cert_chain(client.get())) != 1) {
+ if (sk_X509_num(SSL_get_peer_cert_chain(client.get())) != 1 ||
+ sk_CRYPTO_BUFFER_num(SSL_get0_peer_certificates(client.get())) != 1) {
fprintf(stderr, "Client peer chain was incorrect.\n");
return false;
}
- if (sk_X509_num(SSL_get_peer_cert_chain(server.get())) != 0) {
+ if (sk_X509_num(SSL_get_peer_cert_chain(server.get())) != 0 ||
+ sk_CRYPTO_BUFFER_num(SSL_get0_peer_certificates(server.get())) != 1) {
fprintf(stderr, "Server peer chain was incorrect.\n");
return false;
}
@@ -1731,6 +1733,48 @@
return true;
}
+static bool TestNoPeerCertificate(bool is_dtls, const SSL_METHOD *method,
+ uint16_t version) {
+ bssl::UniquePtr<X509> cert = GetTestCertificate();
+ bssl::UniquePtr<EVP_PKEY> key = GetTestKey();
+ if (!cert || !key) {
+ return false;
+ }
+
+ // Configure an anonymous client.
+ bssl::UniquePtr<SSL_CTX> server_ctx(SSL_CTX_new(method)),
+ client_ctx(SSL_CTX_new(method));
+ if (!server_ctx || !client_ctx ||
+ !SSL_CTX_use_certificate(server_ctx.get(), cert.get()) ||
+ !SSL_CTX_use_PrivateKey(server_ctx.get(), key.get()) ||
+ !SSL_CTX_set_min_proto_version(server_ctx.get(), version) ||
+ !SSL_CTX_set_max_proto_version(server_ctx.get(), version) ||
+ !SSL_CTX_set_min_proto_version(client_ctx.get(), version) ||
+ !SSL_CTX_set_max_proto_version(client_ctx.get(), version)) {
+ return false;
+ }
+ SSL_CTX_set_verify(
+ server_ctx.get(), SSL_VERIFY_PEER, nullptr);
+ SSL_CTX_set_cert_verify_callback(server_ctx.get(), VerifySucceed, NULL);
+ SSL_CTX_set_cert_verify_callback(client_ctx.get(), VerifySucceed, NULL);
+
+ bssl::UniquePtr<SSL> client, server;
+ if (!ConnectClientAndServer(&client, &server, client_ctx.get(),
+ server_ctx.get(), nullptr /* no session */)) {
+ return false;
+ }
+
+ // Client and server should both see the leaf certificate.
+ bssl::UniquePtr<X509> peer(SSL_get_peer_certificate(server.get()));
+ if (peer ||
+ SSL_get0_peer_certificates(server.get()) != nullptr) {
+ fprintf(stderr, "Server peer certificate was non-null.\n");
+ return false;
+ }
+
+ return true;
+}
+
static bool TestRetainOnlySHA256OfCerts(bool is_dtls, const SSL_METHOD *method,
uint16_t version) {
bssl::UniquePtr<X509> cert = GetTestCertificate();
@@ -3900,6 +3944,7 @@
!ForEachVersion(TestSequenceNumber) ||
!ForEachVersion(TestOneSidedShutdown) ||
!ForEachVersion(TestGetPeerCertificate) ||
+ !ForEachVersion(TestNoPeerCertificate) ||
!ForEachVersion(TestRetainOnlySHA256OfCerts) ||
!TestClientHello() ||
!ForEachVersion(TestSessionIDContext) ||
diff --git a/ssl/tls13_both.cc b/ssl/tls13_both.cc
index 338975b..627f038 100644
--- a/ssl/tls13_both.cc
+++ b/ssl/tls13_both.cc
@@ -196,7 +196,9 @@
CBS cbs, context, certificate_list;
CBS_init(&cbs, ssl->init_msg, ssl->init_num);
if (!CBS_get_u8_length_prefixed(&cbs, &context) ||
- CBS_len(&context) != 0) {
+ CBS_len(&context) != 0 ||
+ !CBS_get_u24_length_prefixed(&cbs, &certificate_list) ||
+ CBS_len(&cbs) != 0) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
return 0;
@@ -209,12 +211,6 @@
return 0;
}
- if (!CBS_get_u24_length_prefixed(&cbs, &certificate_list)) {
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- return 0;
- }
-
const bool retain_sha256 =
ssl->server && ssl->retain_only_sha256_of_client_certs;
UniquePtr<EVP_PKEY> pkey;
@@ -326,10 +322,10 @@
}
}
- if (CBS_len(&cbs) != 0) {
- OPENSSL_PUT_ERROR(SSL, SSL_R_DECODE_ERROR);
- ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
- return 0;
+ /* Store a null certificate list rather than an empty one if the peer didn't
+ * send certificates. */
+ if (sk_CRYPTO_BUFFER_num(certs.get()) == 0) {
+ certs.reset();
}
hs->peer_pubkey = std::move(pkey);
