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/include/openssl/ssl.h b/include/openssl/ssl.h index d4b17c9..041895c 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -3698,6 +3698,13 @@ * |peer|, but when a server it does not. */ STACK_OF(X509) *x509_chain; + /* x509_chain_without_leaf is a lazily constructed copy of |x509_chain| that + * omits the leaf certificate. This exists because OpenSSL, historically, + * didn't include the leaf certificate in the chain for a server, but did for + * a client. The |x509_chain| always includes it and, if an API call requires + * a chain without, it is stored here. */ + STACK_OF(X509) *x509_chain_without_leaf; + /* verify_result is the result of certificate verification in the case of * non-fatal certificate errors. */ long verify_result; @@ -3756,6 +3763,9 @@ /* ticket_age_add_valid is non-zero if |ticket_age_add| is valid. */ unsigned ticket_age_add_valid:1; + + /* is_server is true if this session was created by a server. */ + unsigned is_server:1; }; /* ssl_cipher_preference_list_st contains a list of SSL_CIPHERs with
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; }