Give SSL_SESSION a destructor.
Previously we'd partially attempted the ssl_st / bssl::SSLConnection
subclassing split, but that gets messy when we actually try to add a
destructor, because CRYPTO_EX_DATA's cleanup function needs an ssl_st*,
not a bssl::SSLConnection*. Downcasting is technically undefined at this
point and will likely offend some CFI-like check.
Moreover, it appears that even with today's subclassing split,
New<SSL>() emits symbols like:
W ssl_st*& std::forward<ssl_st*&>(std::remove_reference<ssl_st*&>::type&)
The compiler does not bother emitting them in optimized builds, but it
does suggest we can't really avoid claiming the ssl_st type name at the
symbol level, short of doing reinterpret_casts at all API boundaries.
And, of course, we've already long claimed it at the #include level.
So I've just left this defining directly on ssl_session_st. The cost is
we need to write some silly "bssl::" prefixes in the headers, but so it
goes. In the likely event we change our minds again, we can always
revise this.
Change-Id: Ieb429e8eaabe7c2961ef7f8d9234fb71f19a5e2a
Reviewed-on: https://boringssl-review.googlesource.com/29587
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index 65a6299..edbe617 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -965,17 +965,14 @@
CBS certificate_msg = msg.body;
uint8_t alert = SSL_AD_DECODE_ERROR;
- UniquePtr<STACK_OF(CRYPTO_BUFFER)> chain;
- if (!ssl_parse_cert_chain(&alert, &chain, &hs->peer_pubkey,
+ if (!ssl_parse_cert_chain(&alert, &hs->new_session->certs, &hs->peer_pubkey,
hs->config->retain_only_sha256_of_client_certs
? hs->new_session->peer_sha256
- : NULL,
+ : nullptr,
&certificate_msg, ssl->ctx->pool)) {
ssl_send_alert(ssl, SSL3_AL_FATAL, alert);
return ssl_hs_error;
}
- 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())) {
@@ -984,7 +981,7 @@
return ssl_hs_error;
}
- if (sk_CRYPTO_BUFFER_num(hs->new_session->certs) == 0) {
+ if (sk_CRYPTO_BUFFER_num(hs->new_session->certs.get()) == 0) {
// No client certificate so the handshake buffer may be discarded.
hs->transcript.FreeBuffer();
@@ -1009,7 +1006,7 @@
}
static enum ssl_hs_wait_t do_verify_client_certificate(SSL_HANDSHAKE *hs) {
- if (sk_CRYPTO_BUFFER_num(hs->new_session->certs) > 0) {
+ if (sk_CRYPTO_BUFFER_num(hs->new_session->certs.get()) > 0) {
switch (ssl_verify_peer_cert(hs)) {
case ssl_verify_ok:
break;
@@ -1058,12 +1055,13 @@
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER);
return ssl_hs_error;
}
-
- if (!CBS_strdup(&psk_identity, &hs->new_session->psk_identity)) {
+ char *raw = nullptr;
+ if (!CBS_strdup(&psk_identity, &raw)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
return ssl_hs_error;
}
+ hs->new_session->psk_identity.reset(raw);
}
// Depending on the key exchange method, compute |premaster_secret|.
@@ -1178,7 +1176,7 @@
// Look up the key for the identity.
uint8_t psk[PSK_MAX_PSK_LEN];
unsigned psk_len = hs->config->psk_server_callback(
- ssl, hs->new_session->psk_identity, psk, sizeof(psk));
+ ssl, hs->new_session->psk_identity.get(), psk, sizeof(psk));
if (psk_len > PSK_MAX_PSK_LEN) {
OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
ssl_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR);
@@ -1468,8 +1466,7 @@
// If we aren't retaining peer certificates then we can discard it now.
if (hs->new_session != NULL &&
hs->config->retain_only_sha256_of_client_certs) {
- sk_CRYPTO_BUFFER_pop_free(hs->new_session->certs, CRYPTO_BUFFER_free);
- hs->new_session->certs = NULL;
+ hs->new_session->certs.reset();
ssl->ctx->x509_method->session_clear(hs->new_session.get());
}
