kris/boringssl
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Push the difference in chain semantics to the edge.

Browse Source 
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>
This commit is contained in:
Adam Langley 2016-12-12 10:51:00 -08:00 committed by CQ bot account: commit-bot@chromium.org
parent cb0c29ff75
commit 364f7a6d21
9 changed files with 104 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
include/openssl/ssl.h
View File

@ -3698,6 +3698,13 @@ struct ssl_session_st {
* |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 @@ struct ssl_session_st {
/* 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

7
ssl/handshake_client.c
View File

@ -756,7 +756,8 @@ static int ssl3_send_client_hello(SSL_HANDSHAKE *hs) {
* 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 @@ static int ssl3_get_server_certificate(SSL_HANDSHAKE *hs) {
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);

16
ssl/handshake_server.c
View File

@ -1483,6 +1483,8 @@ static int ssl3_get_client_certificate(SSL_HANDSHAKE *hs) {
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 @@ static int ssl3_get_client_certificate(SSL_HANDSHAKE *hs) {
* 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 @@ static int ssl3_get_client_certificate(SSL_HANDSHAKE *hs) {
}
}
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;

42
ssl/ssl_asn1.c
View File

@ -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 @@ static const int kCertChainTag =
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 @@ static int SSL_SESSION_to_bytes_full(const SSL_SESSION *in, uint8_t **out_data,
/* 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 @@ static int SSL_SESSION_to_bytes_full(const SSL_SESSION *in, uint8_t **out_data,
}
}
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 @@ static SSL_SESSION *SSL_SESSION_parse(CBS *cbs) {
}
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 @@ static SSL_SESSION *SSL_SESSION_parse(CBS *cbs) {
}
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;

29
ssl/ssl_lib.c
View File

@ -1087,10 +1087,35 @@ STACK_OF(X509) *SSL_get_peer_cert_chain(const SSL *ssl) {
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,

10
ssl/ssl_session.c
View File

@ -182,6 +182,7 @@ SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *session, int dup_flags) {
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 @@ void SSL_SESSION_free(SSL_SESSION *session) {
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 @@ int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) {
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 @@ int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) {
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_time_valid(const SSL *ssl, const SSL_SESSION *session) {
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

7
ssl/ssl_test.cc
View File

@ -448,10 +448,9 @@ static bool TestCurveRules() {
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 @@ static const char kOpenSSLSession[] =
"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

2
ssl/tls13_both.c
View File

@ -322,6 +322,8 @@ int tls13_process_certificate(SSL *ssl, int allow_anonymous) {
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;

6
ssl/tls13_server.c
View File

@ -552,12 +552,6 @@ static enum ssl_hs_wait_t do_process_client_certificate(SSL_HANDSHAKE *hs) {
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;
}