Push the difference in chain semantics to the edge.
OpenSSL includes a leaf certificate in a certificate chain when it's a
client, but doesn't when it's a server. This is also reflected in the
serialisation of sessions.
This change makes the internal semantics consistent: the leaf is always
included in the chain in memory, and never duplicated when serialised.
To maintain the same API, SSL_get_peer_cert_chain will construct a copy
of the chain without the leaf if needed.
Since the serialised format of a client session has changed, an
|is_server| boolean is added to the ASN.1 that defaults to true. Thus
any old client sessions will be parsed as server sessions and (silently)
discarded by a client.
Change-Id: Ibcf72bc8a130cedb423bc0fd3417868e0af3ca3e
Reviewed-on: https://boringssl-review.googlesource.com/12704
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/handshake_client.c b/ssl/handshake_client.c
index bae5365..59fefc4 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -756,7 +756,8 @@
* version, drop it. */
if (ssl->session != NULL) {
uint16_t session_version;
- if (!ssl->method->version_from_wire(&session_version,
+ if (ssl->session->is_server ||
+ !ssl->method->version_from_wire(&session_version,
ssl->session->ssl_version) ||
(session_version < TLS1_3_VERSION &&
ssl->session->session_id_length == 0) ||
@@ -1059,10 +1060,10 @@
goto err;
}
- /* NOTE: Unlike the server half, the client's copy of |x509_chain| includes
- * the leaf. */
sk_X509_pop_free(ssl->s3->new_session->x509_chain, X509_free);
ssl->s3->new_session->x509_chain = chain;
+ sk_X509_pop_free(ssl->s3->new_session->x509_chain_without_leaf, X509_free);
+ ssl->s3->new_session->x509_chain_without_leaf = NULL;
X509_free(ssl->s3->new_session->x509_peer);
X509_up_ref(leaf);
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index 5121f99..cca9bd6 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -1483,6 +1483,8 @@
goto err;
}
+ X509 *leaf = NULL;
+
if (sk_X509_num(chain) == 0) {
/* No client certificate so the handshake buffer may be discarded. */
ssl3_free_handshake_buffer(ssl);
@@ -1506,6 +1508,7 @@
* classed by them as a bug, but it's assumed by at least NGINX. */
ssl->s3->new_session->verify_result = X509_V_OK;
} else {
+ leaf = sk_X509_value(chain, 0);
/* The hash would have been filled in. */
if (ssl->retain_only_sha256_of_client_certs) {
ssl->s3->new_session->peer_sha256_valid = 1;
@@ -1517,13 +1520,16 @@
}
}
- X509_free(ssl->s3->new_session->x509_peer);
- ssl->s3->new_session->x509_peer = sk_X509_shift(chain);
-
sk_X509_pop_free(ssl->s3->new_session->x509_chain, X509_free);
ssl->s3->new_session->x509_chain = chain;
- /* Inconsistency alert: x509_chain does *not* include the peer's own
- * certificate, while we do include it in s3_clnt.c */
+ sk_X509_pop_free(ssl->s3->new_session->x509_chain_without_leaf, X509_free);
+ ssl->s3->new_session->x509_chain_without_leaf = NULL;
+
+ X509_free(ssl->s3->new_session->x509_peer);
+ if (leaf) {
+ X509_up_ref(leaf);
+ }
+ ssl->s3->new_session->x509_peer = leaf;
return 1;
diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c
index aab6052..67d9df8 100644
--- a/ssl/ssl_asn1.c
+++ b/ssl/ssl_asn1.c
@@ -122,6 +122,7 @@
* keyExchangeInfo [18] INTEGER OPTIONAL,
* certChain [19] SEQUENCE OF Certificate OPTIONAL,
* ticketAgeAdd [21] OCTET STRING OPTIONAL,
+ * isServer [22] BOOLEAN DEFAULT TRUE,
* }
*
* Note: historically this serialization has included other optional
@@ -170,6 +171,8 @@
CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 19;
static const int kTicketAgeAddTag =
CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 21;
+static const int kIsServerTag =
+ CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 22;
static int SSL_SESSION_to_bytes_full(const SSL_SESSION *in, uint8_t **out_data,
size_t *out_len, int for_ticket) {
@@ -340,12 +343,13 @@
/* The certificate chain is only serialized if the leaf's SHA-256 isn't
* serialized instead. */
- if (in->x509_chain != NULL && !in->peer_sha256_valid) {
+ if (in->x509_chain != NULL && !in->peer_sha256_valid &&
+ sk_X509_num(in->x509_chain) >= 2) {
if (!CBB_add_asn1(&session, &child, kCertChainTag)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
goto err;
}
- for (size_t i = 0; i < sk_X509_num(in->x509_chain); i++) {
+ for (size_t i = 1; i < sk_X509_num(in->x509_chain); i++) {
if (!ssl_add_cert_to_cbb(&child, sk_X509_value(in->x509_chain, i))) {
goto err;
}
@@ -361,6 +365,15 @@
}
}
+ if (!in->is_server) {
+ if (!CBB_add_asn1(&session, &child, kIsServerTag) ||
+ !CBB_add_asn1(&child, &child2, CBS_ASN1_BOOLEAN) ||
+ !CBB_add_u8(&child2, 0x00)) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+ goto err;
+ }
+ }
+
if (!CBB_finish(&cbb, out_data, out_len)) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
goto err;
@@ -658,8 +671,20 @@
}
sk_X509_pop_free(ret->x509_chain, X509_free);
ret->x509_chain = NULL;
- if (has_cert_chain) {
+ if (ret->x509_peer != NULL) {
ret->x509_chain = sk_X509_new_null();
+ if (ret->x509_chain == NULL ||
+ !sk_X509_push(ret->x509_chain, ret->x509_peer)) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
+ goto err;
+ }
+ X509_up_ref(ret->x509_peer);
+ }
+ if (has_cert_chain) {
+ if (ret->x509_peer == NULL) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION);
+ goto err;
+ }
if (ret->x509_chain == NULL) {
OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
goto err;
@@ -688,6 +713,17 @@
}
ret->ticket_age_add_valid = age_add_present;
+ int is_server;
+ if (!CBS_get_optional_asn1_bool(&session, &is_server, kIsServerTag,
+ 1 /* default to true */)) {
+ OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION);
+ goto err;
+ }
+ /* TODO: in time we can include |is_server| for servers too, then we can
+ enforce that client and server sessions are never mixed up. */
+
+ ret->is_server = is_server;
+
if (CBS_len(&session) != 0) {
OPENSSL_PUT_ERROR(SSL, SSL_R_INVALID_SSL_SESSION);
goto err;
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index 336689a..8638396 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1087,10 +1087,35 @@
return NULL;
}
SSL_SESSION *session = SSL_get_session(ssl);
- if (session == NULL) {
+ if (session == NULL ||
+ session->x509_chain == NULL) {
return NULL;
}
- return session->x509_chain;
+
+ if (!ssl->server) {
+ return session->x509_chain;
+ }
+
+ /* OpenSSL historically didn't include the leaf certificate in the returned
+ * certificate chain, but only for servers. */
+ if (session->x509_chain_without_leaf == NULL) {
+ session->x509_chain_without_leaf = sk_X509_new_null();
+ if (session->x509_chain_without_leaf == NULL) {
+ return NULL;
+ }
+
+ for (size_t i = 1; i < sk_X509_num(session->x509_chain); i++) {
+ X509 *cert = sk_X509_value(session->x509_chain, i);
+ if (!sk_X509_push(session->x509_chain_without_leaf, cert)) {
+ sk_X509_pop_free(session->x509_chain_without_leaf, X509_free);
+ session->x509_chain_without_leaf = NULL;
+ return NULL;
+ }
+ X509_up_ref(cert);
+ }
+ }
+
+ return session->x509_chain_without_leaf;
}
int SSL_get_tls_unique(const SSL *ssl, uint8_t *out, size_t *out_len,
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index 98d101c..b0951ac 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -182,6 +182,7 @@
goto err;
}
+ new_session->is_server = session->is_server;
new_session->ssl_version = session->ssl_version;
new_session->sid_ctx_length = session->sid_ctx_length;
memcpy(new_session->sid_ctx, session->sid_ctx, session->sid_ctx_length);
@@ -327,6 +328,7 @@
OPENSSL_cleanse(session->session_id, sizeof(session->session_id));
X509_free(session->x509_peer);
sk_X509_pop_free(session->x509_chain, X509_free);
+ sk_X509_pop_free(session->x509_chain_without_leaf, X509_free);
OPENSSL_free(session->tlsext_hostname);
OPENSSL_free(session->tlsext_tick);
OPENSSL_free(session->tlsext_signed_cert_timestamp_list);
@@ -462,6 +464,9 @@
return 0;
}
+ session->is_server = is_server;
+ session->ssl_version = ssl->version;
+
/* Fill in the time from the |SSL_CTX|'s clock. */
struct timeval now;
ssl_get_current_time(ssl, &now);
@@ -469,8 +474,6 @@
session->timeout = ssl->session_timeout;
- session->ssl_version = ssl->version;
-
if (is_server) {
if (hs->ticket_expected) {
/* Don't set session IDs for sessions resumed with tickets. This will keep
@@ -634,6 +637,9 @@
int ssl_session_is_resumable(const SSL *ssl, const SSL_SESSION *session) {
return ssl_session_is_context_valid(ssl, session) &&
+ /* The session must have been created by the same type of end point as
+ * we're now using it with. */
+ session->is_server == ssl->server &&
/* The session must not be expired. */
ssl_session_is_time_valid(ssl, session) &&
/* Only resume if the session's version matches the negotiated
diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc
index 5e4485f..b74e51e 100644
--- a/ssl/ssl_test.cc
+++ b/ssl/ssl_test.cc
@@ -448,10 +448,9 @@
return true;
}
-// kOpenSSLSession is a serialized SSL_SESSION generated from openssl
-// s_client -sess_out.
+// kOpenSSLSession is a serialized SSL_SESSION.
static const char kOpenSSLSession[] =
- "MIIFpQIBAQICAwMEAsAvBCAG5Q1ndq4Yfmbeo1zwLkNRKmCXGdNgWvGT3cskV0yQ"
+ "MIIFqgIBAQICAwMEAsAvBCAG5Q1ndq4Yfmbeo1zwLkNRKmCXGdNgWvGT3cskV0yQ"
"kAQwJlrlzkAWBOWiLj/jJ76D7l+UXoizP2KI2C7I2FccqMmIfFmmkUy32nIJ0mZH"
"IWoJoQYCBFRDO46iBAICASyjggR6MIIEdjCCA16gAwIBAgIIK9dUvsPWSlUwDQYJ"
"KoZIhvcNAQEFBQAwSTELMAkGA1UEBhMCVVMxEzARBgNVBAoTCkdvb2dsZSBJbmMx"
@@ -481,7 +480,7 @@
"YTcLEkXqKwOBfF9vE4KX0NxeLwjcDTpsuh3qXEaZ992r1N38VDcyS6P7I6HBYN9B"
"sNHM362zZnY27GpTw+Kwd751CLoXFPoaMOe57dbBpXoro6Pd3BTbf/Tzr88K06yE"
"OTDKPNj3+inbMaVigtK4PLyPq+Topyzvx9USFgRvyuoxn0Hgb+R0A3j6SLRuyOdA"
- "i4gv7Y5oliyn";
+ "i4gv7Y5oliyntgMBAQA=";
// kCustomSession is a custom serialized SSL_SESSION generated by
// filling in missing fields from |kOpenSSLSession|. This includes
diff --git a/ssl/tls13_both.c b/ssl/tls13_both.c
index 1a1d8b9..939e530 100644
--- a/ssl/tls13_both.c
+++ b/ssl/tls13_both.c
@@ -322,6 +322,8 @@
sk_X509_pop_free(ssl->s3->new_session->x509_chain, X509_free);
ssl->s3->new_session->x509_chain = chain;
chain = NULL;
+ sk_X509_pop_free(ssl->s3->new_session->x509_chain_without_leaf, X509_free);
+ ssl->s3->new_session->x509_chain_without_leaf = NULL;
ret = 1;
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c
index 3ab9c86..367ea5a 100644
--- a/ssl/tls13_server.c
+++ b/ssl/tls13_server.c
@@ -552,12 +552,6 @@
return ssl_hs_error;
}
- /* For historical reasons, the server's copy of the chain does not include the
- * leaf while the client's does. */
- if (sk_X509_num(ssl->s3->new_session->x509_chain) > 0) {
- X509_free(sk_X509_shift(ssl->s3->new_session->x509_chain));
- }
-
hs->tls13_state = state_process_client_certificate_verify;
return ssl_hs_read_message;
}
