From 5960a909648b9be1469f9f6b9c481d1cb8f4f759 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 14 Feb 2017 20:07:11 -0500 Subject: [PATCH] Move sid_ctx from SSL/SSL_CTX to CERT. This reduces us from seven different configuration patterns to six (see comment #2 of linked bug). I do not believe there is any behavior change here as SSL_set_SSL_CTX already manually copied the field. It now gives us a nice invariant: SSL_set_SSL_CTX overrides all and only the dual-SSL/SSL_CTX options hanging off of CERT. BUG=123 Change-Id: I1ae06b791fb869917a6503cee41afb2d9be53d89 Reviewed-on: https://boringssl-review.googlesource.com/13865 Reviewed-by: David Benjamin Commit-Queue: David Benjamin CQ-Verified: CQ bot account: commit-bot@chromium.org --- include/openssl/ssl.h | 2 -- ssl/internal.h | 10 +++++----- ssl/ssl_cert.c | 3 +++ ssl/ssl_lib.c | 38 +++++++++++++------------------------- ssl/ssl_session.c | 13 +++++++------ 5 files changed, 28 insertions(+), 38 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 56a03078..8d1c4c1f 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -3971,8 +3971,6 @@ struct ssl_ctx_st { void *msg_callback_arg; int verify_mode; - uint8_t sid_ctx_length; - uint8_t sid_ctx[SSL_MAX_SID_CTX_LENGTH]; int (*default_verify_callback)( int ok, X509_STORE_CTX *ctx); /* called 'verify_callback' in the SSL */ diff --git a/ssl/internal.h b/ssl/internal.h index d671308d..7fe5dee2 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1329,6 +1329,11 @@ typedef struct cert_st { /* OCSP response to be sent to the client, if requested. */ CRYPTO_BUFFER *ocsp_response; + + /* sid_ctx partitions the session space within a shared session cache or + * ticket key. Only sessions with a matching value will be accepted. */ + uint8_t sid_ctx_length; + uint8_t sid_ctx[SSL_MAX_SID_CTX_LENGTH]; } CERT; /* SSL_METHOD is a compatibility structure to support the legacy version-locked @@ -1804,11 +1809,6 @@ struct ssl_st { * milliseconds. It's used to initialize the timer any time it's restarted. */ unsigned initial_timeout_duration_ms; - /* the session_id_context is used to ensure sessions are only reused - * in the appropriate context */ - uint8_t sid_ctx_length; - uint8_t sid_ctx[SSL_MAX_SID_CTX_LENGTH]; - /* session is the configured session to be offered by the client. This session * is immutable. */ SSL_SESSION *session; diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c index c94ca497..f4301a88 100644 --- a/ssl/ssl_cert.c +++ b/ssl/ssl_cert.c @@ -213,6 +213,9 @@ CERT *ssl_cert_dup(CERT *cert) { ret->ocsp_response = cert->ocsp_response; } + ret->sid_ctx_length = cert->sid_ctx_length; + OPENSSL_memcpy(ret->sid_ctx, cert->sid_ctx, sizeof(ret->sid_ctx)); + return ret; err: diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 91959054..9bf09df1 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -403,9 +403,6 @@ SSL *SSL_new(SSL_CTX *ctx) { ssl->msg_callback = ctx->msg_callback; ssl->msg_callback_arg = ctx->msg_callback_arg; ssl->verify_mode = ctx->verify_mode; - ssl->sid_ctx_length = ctx->sid_ctx_length; - assert(ssl->sid_ctx_length <= sizeof ssl->sid_ctx); - OPENSSL_memcpy(&ssl->sid_ctx, &ctx->sid_ctx, sizeof(ssl->sid_ctx)); ssl->verify_callback = ctx->default_verify_callback; ssl->retain_only_sha256_of_client_certs = ctx->retain_only_sha256_of_client_certs; @@ -1072,37 +1069,32 @@ err: return 0; } -int SSL_CTX_set_session_id_context(SSL_CTX *ctx, const uint8_t *sid_ctx, +static int set_session_id_context(CERT *cert, const uint8_t *sid_ctx, size_t sid_ctx_len) { - if (sid_ctx_len > sizeof(ctx->sid_ctx)) { + if (sid_ctx_len > sizeof(cert->sid_ctx)) { OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_SESSION_ID_CONTEXT_TOO_LONG); return 0; } - assert(sizeof(ctx->sid_ctx) < 256); - ctx->sid_ctx_length = (uint8_t)sid_ctx_len; - OPENSSL_memcpy(ctx->sid_ctx, sid_ctx, sid_ctx_len); - + OPENSSL_COMPILE_ASSERT(sizeof(cert->sid_ctx) < 256, sid_ctx_too_large); + cert->sid_ctx_length = (uint8_t)sid_ctx_len; + OPENSSL_memcpy(cert->sid_ctx, sid_ctx, sid_ctx_len); return 1; } +int SSL_CTX_set_session_id_context(SSL_CTX *ctx, const uint8_t *sid_ctx, + size_t sid_ctx_len) { + return set_session_id_context(ctx->cert, sid_ctx, sid_ctx_len); +} + int SSL_set_session_id_context(SSL *ssl, const uint8_t *sid_ctx, size_t sid_ctx_len) { - if (sid_ctx_len > sizeof(ssl->sid_ctx)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_SSL_SESSION_ID_CONTEXT_TOO_LONG); - return 0; - } - - assert(sizeof(ssl->sid_ctx) < 256); - ssl->sid_ctx_length = (uint8_t)sid_ctx_len; - OPENSSL_memcpy(ssl->sid_ctx, sid_ctx, sid_ctx_len); - - return 1; + return set_session_id_context(ssl->cert, sid_ctx, sid_ctx_len); } const uint8_t *SSL_get0_session_id_context(const SSL *ssl, size_t *out_len) { - *out_len = ssl->sid_ctx_length; - return ssl->sid_ctx; + *out_len = ssl->cert->sid_ctx_length; + return ssl->cert->sid_ctx; } void ssl_cipher_preference_list_free( @@ -2012,10 +2004,6 @@ SSL_CTX *SSL_set_SSL_CTX(SSL *ssl, SSL_CTX *ctx) { SSL_CTX_free(ssl->ctx); ssl->ctx = ctx; - ssl->sid_ctx_length = ctx->sid_ctx_length; - assert(ssl->sid_ctx_length <= sizeof(ssl->sid_ctx)); - OPENSSL_memcpy(ssl->sid_ctx, ctx->sid_ctx, sizeof(ssl->sid_ctx)); - return ssl->ctx; } diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c index 60f20f48..47f3bcd7 100644 --- a/ssl/ssl_session.c +++ b/ssl/ssl_session.c @@ -560,12 +560,13 @@ int ssl_get_new_session(SSL_HANDSHAKE *hs, int is_server) { session->session_id_length = 0; } - if (ssl->sid_ctx_length > sizeof(session->sid_ctx)) { + if (ssl->cert->sid_ctx_length > sizeof(session->sid_ctx)) { OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR); goto err; } - OPENSSL_memcpy(session->sid_ctx, ssl->sid_ctx, ssl->sid_ctx_length); - session->sid_ctx_length = ssl->sid_ctx_length; + OPENSSL_memcpy(session->sid_ctx, ssl->cert->sid_ctx, + ssl->cert->sid_ctx_length); + session->sid_ctx_length = ssl->cert->sid_ctx_length; /* The session is marked not resumable until it is completely filled in. */ session->not_resumable = 1; @@ -678,9 +679,9 @@ int ssl_session_is_context_valid(const SSL *ssl, const SSL_SESSION *session) { return 0; } - return session->sid_ctx_length == ssl->sid_ctx_length && - OPENSSL_memcmp(session->sid_ctx, ssl->sid_ctx, ssl->sid_ctx_length) == - 0; + return session->sid_ctx_length == ssl->cert->sid_ctx_length && + OPENSSL_memcmp(session->sid_ctx, ssl->cert->sid_ctx, + ssl->cert->sid_ctx_length) == 0; } int ssl_session_is_time_valid(const SSL *ssl, const SSL_SESSION *session) {