diff --git a/ssl/internal.h b/ssl/internal.h index 585aaf28..3c47a20e 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -339,6 +339,15 @@ class Array { return true; } + // Shrink shrinks the stored size of the array to |new_size|. It crashes if + // the new size is larger. Note this does not shrink the allocation itself. + void Shrink(size_t new_size) { + if (new_size > size_) { + abort(); + } + size_ = new_size; + } + private: T *data_ = nullptr; size_t size_ = 0; @@ -2701,8 +2710,8 @@ bool ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, CBS *cbs); // |ssl_ticket_aead_error|: an error occured that is fatal to the connection. enum ssl_ticket_aead_result_t ssl_process_ticket( SSL_HANDSHAKE *hs, UniquePtr *out_session, - bool *out_renew_ticket, const uint8_t *ticket, size_t ticket_len, - const uint8_t *session_id, size_t session_id_len); + bool *out_renew_ticket, Span ticket, + Span session_id); // tls1_verify_channel_id processes |msg| as a Channel ID message, and verifies // the signature. If the key is valid, it saves the Channel ID and returns true. diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 70be17e6..1b0b68a4 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc @@ -718,16 +718,15 @@ enum ssl_hs_wait_t ssl_get_prev_session(SSL_HANDSHAKE *hs, bool renew_ticket = false; // If tickets are disabled, always behave as if no tickets are present. - const uint8_t *ticket = NULL; - size_t ticket_len = 0; + CBS ticket; const bool tickets_supported = !(SSL_get_options(hs->ssl) & SSL_OP_NO_TICKET) && - SSL_early_callback_ctx_extension_get( - client_hello, TLSEXT_TYPE_session_ticket, &ticket, &ticket_len); - if (tickets_supported && ticket_len > 0) { - switch (ssl_process_ticket(hs, &session, &renew_ticket, ticket, ticket_len, - client_hello->session_id, - client_hello->session_id_len)) { + ssl_client_hello_get_extension(client_hello, &ticket, + TLSEXT_TYPE_session_ticket); + if (tickets_supported && CBS_len(&ticket) != 0) { + switch (ssl_process_ticket(hs, &session, &renew_ticket, ticket, + MakeConstSpan(client_hello->session_id, + client_hello->session_id_len))) { case ssl_ticket_aead_success: break; case ssl_ticket_aead_ignore_ticket: diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc index dde767e9..4de563d8 100644 --- a/ssl/t1_lib.cc +++ b/ssl/t1_lib.cc @@ -3412,21 +3412,24 @@ bool ssl_parse_serverhello_tlsext(SSL_HANDSHAKE *hs, CBS *cbs) { } static enum ssl_ticket_aead_result_t decrypt_ticket_with_cipher_ctx( - uint8_t **out, size_t *out_len, EVP_CIPHER_CTX *cipher_ctx, - HMAC_CTX *hmac_ctx, const uint8_t *ticket, size_t ticket_len) { + Array *out, EVP_CIPHER_CTX *cipher_ctx, HMAC_CTX *hmac_ctx, + Span ticket) { size_t iv_len = EVP_CIPHER_CTX_iv_length(cipher_ctx); // Check the MAC at the end of the ticket. uint8_t mac[EVP_MAX_MD_SIZE]; size_t mac_len = HMAC_size(hmac_ctx); - if (ticket_len < SSL_TICKET_KEY_NAME_LEN + iv_len + 1 + mac_len) { + if (ticket.size() < SSL_TICKET_KEY_NAME_LEN + iv_len + 1 + mac_len) { // The ticket must be large enough for key name, IV, data, and MAC. return ssl_ticket_aead_ignore_ticket; } - HMAC_Update(hmac_ctx, ticket, ticket_len - mac_len); + // Split the ticket into the ticket and the MAC. + auto ticket_mac = ticket.subspan(ticket.size() - mac_len); + ticket = ticket.subspan(0, ticket.size() - mac_len); + HMAC_Update(hmac_ctx, ticket.data(), ticket.size()); HMAC_Final(hmac_ctx, mac, NULL); - bool mac_ok = - CRYPTO_memcmp(mac, ticket + (ticket_len - mac_len), mac_len) == 0; + assert(mac_len == ticket_mac.size()); + bool mac_ok = CRYPTO_memcmp(mac, ticket_mac.data(), mac_len) == 0; #if defined(BORINGSSL_UNSAFE_FUZZER_MODE) mac_ok = true; #endif @@ -3435,46 +3438,48 @@ static enum ssl_ticket_aead_result_t decrypt_ticket_with_cipher_ctx( } // Decrypt the session data. - const uint8_t *ciphertext = ticket + SSL_TICKET_KEY_NAME_LEN + iv_len; - size_t ciphertext_len = ticket_len - SSL_TICKET_KEY_NAME_LEN - iv_len - - mac_len; - UniquePtr plaintext((uint8_t *)OPENSSL_malloc(ciphertext_len)); - if (!plaintext) { + auto ciphertext = ticket.subspan(SSL_TICKET_KEY_NAME_LEN + iv_len); + Array plaintext; +#if defined(BORINGSSL_UNSAFE_FUZZER_MODE) + if (!plaintext.CopyFrom(ciphertext)) { return ssl_ticket_aead_error; } - size_t plaintext_len; -#if defined(BORINGSSL_UNSAFE_FUZZER_MODE) - OPENSSL_memcpy(plaintext.get(), ciphertext, ciphertext_len); - plaintext_len = ciphertext_len; #else - if (ciphertext_len >= INT_MAX) { + if (ciphertext.size() >= INT_MAX) { return ssl_ticket_aead_ignore_ticket; } + if (!plaintext.Init(ciphertext.size())) { + return ssl_ticket_aead_error; + } int len1, len2; - if (!EVP_DecryptUpdate(cipher_ctx, plaintext.get(), &len1, ciphertext, - (int)ciphertext_len) || - !EVP_DecryptFinal_ex(cipher_ctx, plaintext.get() + len1, &len2)) { + if (!EVP_DecryptUpdate(cipher_ctx, plaintext.data(), &len1, ciphertext.data(), + (int)ciphertext.size()) || + !EVP_DecryptFinal_ex(cipher_ctx, plaintext.data() + len1, &len2)) { ERR_clear_error(); return ssl_ticket_aead_ignore_ticket; } - plaintext_len = (size_t)(len1) + len2; + plaintext.Shrink(static_cast(len1) + len2); #endif - *out = plaintext.release(); - *out_len = plaintext_len; + *out = std::move(plaintext); return ssl_ticket_aead_success; } static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_cb( - SSL_HANDSHAKE *hs, uint8_t **out, size_t *out_len, bool *out_renew_ticket, - const uint8_t *ticket, size_t ticket_len) { - assert(ticket_len >= SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH); + SSL_HANDSHAKE *hs, Array *out, bool *out_renew_ticket, + Span ticket) { + assert(ticket.size() >= SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH); ScopedEVP_CIPHER_CTX cipher_ctx; ScopedHMAC_CTX hmac_ctx; - const uint8_t *iv = ticket + SSL_TICKET_KEY_NAME_LEN; + auto name = ticket.subspan(0, SSL_TICKET_KEY_NAME_LEN); + // The actual IV is shorter, but the length is determined by the callback's + // chosen cipher. Instead we pass in |EVP_MAX_IV_LENGTH| worth of IV to ensure + // the callback has enough. + auto iv = ticket.subspan(SSL_TICKET_KEY_NAME_LEN, EVP_MAX_IV_LENGTH); int cb_ret = hs->ssl->session_ctx->ticket_key_cb( - hs->ssl, (uint8_t *)ticket /* name */, (uint8_t *)iv, cipher_ctx.get(), - hmac_ctx.get(), 0 /* decrypt */); + hs->ssl, const_cast(name.data()), + const_cast(iv.data()), cipher_ctx.get(), hmac_ctx.get(), + 0 /* decrypt */); if (cb_ret < 0) { return ssl_ticket_aead_error; } else if (cb_ret == 0) { @@ -3484,14 +3489,13 @@ static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_cb( } else { assert(cb_ret == 1); } - return decrypt_ticket_with_cipher_ctx(out, out_len, cipher_ctx.get(), - hmac_ctx.get(), ticket, ticket_len); + return decrypt_ticket_with_cipher_ctx(out, cipher_ctx.get(), hmac_ctx.get(), + ticket); } static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_ticket_keys( - SSL_HANDSHAKE *hs, uint8_t **out, size_t *out_len, const uint8_t *ticket, - size_t ticket_len) { - assert(ticket_len >= SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH); + SSL_HANDSHAKE *hs, Array *out, Span ticket) { + assert(ticket.size() >= SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH); SSL_CTX *ctx = hs->ssl->session_ctx.get(); // Rotate the ticket key if necessary. @@ -3499,40 +3503,40 @@ static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_ticket_keys( return ssl_ticket_aead_error; } + const EVP_CIPHER *cipher = EVP_aes_128_cbc(); + auto name = ticket.subspan(0, SSL_TICKET_KEY_NAME_LEN); + auto iv = + ticket.subspan(SSL_TICKET_KEY_NAME_LEN, EVP_CIPHER_iv_length(cipher)); + // Pick the matching ticket key and decrypt. ScopedEVP_CIPHER_CTX cipher_ctx; ScopedHMAC_CTX hmac_ctx; { MutexReadLock lock(&ctx->lock); const TicketKey *key; - if (ctx->ticket_key_current && - !OPENSSL_memcmp(ctx->ticket_key_current->name, ticket, - SSL_TICKET_KEY_NAME_LEN)) { + if (ctx->ticket_key_current && name == ctx->ticket_key_current->name) { key = ctx->ticket_key_current.get(); - } else if (ctx->ticket_key_prev && - !OPENSSL_memcmp(ctx->ticket_key_prev->name, ticket, - SSL_TICKET_KEY_NAME_LEN)) { + } else if (ctx->ticket_key_prev && name == ctx->ticket_key_prev->name) { key = ctx->ticket_key_prev.get(); } else { return ssl_ticket_aead_ignore_ticket; } - const uint8_t *iv = ticket + SSL_TICKET_KEY_NAME_LEN; if (!HMAC_Init_ex(hmac_ctx.get(), key->hmac_key, sizeof(key->hmac_key), tlsext_tick_md(), NULL) || - !EVP_DecryptInit_ex(cipher_ctx.get(), EVP_aes_128_cbc(), NULL, - key->aes_key, iv)) { + !EVP_DecryptInit_ex(cipher_ctx.get(), cipher, NULL, + key->aes_key, iv.data())) { return ssl_ticket_aead_error; } } - return decrypt_ticket_with_cipher_ctx(out, out_len, cipher_ctx.get(), - hmac_ctx.get(), ticket, ticket_len); + return decrypt_ticket_with_cipher_ctx(out, cipher_ctx.get(), hmac_ctx.get(), + ticket); } static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_method( - SSL_HANDSHAKE *hs, uint8_t **out, size_t *out_len, bool *out_renew_ticket, - const uint8_t *ticket, size_t ticket_len) { - uint8_t *plaintext = (uint8_t *)OPENSSL_malloc(ticket_len); - if (plaintext == NULL) { + SSL_HANDSHAKE *hs, Array *out, bool *out_renew_ticket, + Span ticket) { + Array plaintext; + if (!plaintext.Init(ticket.size())) { OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE); return ssl_ticket_aead_error; } @@ -3540,50 +3544,47 @@ static enum ssl_ticket_aead_result_t ssl_decrypt_ticket_with_method( size_t plaintext_len; const enum ssl_ticket_aead_result_t result = hs->ssl->session_ctx->ticket_aead_method->open( - hs->ssl, plaintext, &plaintext_len, ticket_len, ticket, ticket_len); - - if (result == ssl_ticket_aead_success) { - *out = plaintext; - plaintext = NULL; - *out_len = plaintext_len; + hs->ssl, plaintext.data(), &plaintext_len, ticket.size(), + ticket.data(), ticket.size()); + if (result != ssl_ticket_aead_success) { + return result; } - OPENSSL_free(plaintext); - return result; + plaintext.Shrink(plaintext_len); + *out = std::move(plaintext); + return ssl_ticket_aead_success; } enum ssl_ticket_aead_result_t ssl_process_ticket( SSL_HANDSHAKE *hs, UniquePtr *out_session, - bool *out_renew_ticket, const uint8_t *ticket, size_t ticket_len, - const uint8_t *session_id, size_t session_id_len) { + bool *out_renew_ticket, Span ticket, + Span session_id) { *out_renew_ticket = false; out_session->reset(); if ((SSL_get_options(hs->ssl) & SSL_OP_NO_TICKET) || - session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH) { + session_id.size() > SSL_MAX_SSL_SESSION_ID_LENGTH) { return ssl_ticket_aead_ignore_ticket; } - uint8_t *plaintext = NULL; - size_t plaintext_len; + Array plaintext; enum ssl_ticket_aead_result_t result; if (hs->ssl->session_ctx->ticket_aead_method != NULL) { - result = ssl_decrypt_ticket_with_method( - hs, &plaintext, &plaintext_len, out_renew_ticket, ticket, ticket_len); + result = ssl_decrypt_ticket_with_method(hs, &plaintext, out_renew_ticket, + ticket); } else { // Ensure there is room for the key name and the largest IV |ticket_key_cb| // may try to consume. The real limit may be lower, but the maximum IV // length should be well under the minimum size for the session material and // HMAC. - if (ticket_len < SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH) { + if (ticket.size() < SSL_TICKET_KEY_NAME_LEN + EVP_MAX_IV_LENGTH) { return ssl_ticket_aead_ignore_ticket; } if (hs->ssl->session_ctx->ticket_key_cb != NULL) { - result = ssl_decrypt_ticket_with_cb(hs, &plaintext, &plaintext_len, - out_renew_ticket, ticket, ticket_len); + result = + ssl_decrypt_ticket_with_cb(hs, &plaintext, out_renew_ticket, ticket); } else { - result = ssl_decrypt_ticket_with_ticket_keys( - hs, &plaintext, &plaintext_len, ticket, ticket_len); + result = ssl_decrypt_ticket_with_ticket_keys(hs, &plaintext, ticket); } } @@ -3592,10 +3593,8 @@ enum ssl_ticket_aead_result_t ssl_process_ticket( } // Decode the session. - UniquePtr session( - SSL_SESSION_from_bytes(plaintext, plaintext_len, hs->ssl->ctx.get())); - OPENSSL_free(plaintext); - + UniquePtr session(SSL_SESSION_from_bytes( + plaintext.data(), plaintext.size(), hs->ssl->ctx.get())); if (!session) { ERR_clear_error(); // Don't leave an error on the queue. return ssl_ticket_aead_ignore_ticket; @@ -3603,8 +3602,8 @@ enum ssl_ticket_aead_result_t ssl_process_ticket( // Copy the client's session ID into the new session, to denote the ticket has // been accepted. - OPENSSL_memcpy(session->session_id, session_id, session_id_len); - session->session_id_length = session_id_len; + OPENSSL_memcpy(session->session_id, session_id.data(), session_id.size()); + session->session_id_length = session_id.size(); *out_session = std::move(session); return ssl_ticket_aead_success; diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 203e704d..a3940b64 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc @@ -313,8 +313,7 @@ static enum ssl_ticket_aead_result_t select_session( bool unused_renew; UniquePtr session; enum ssl_ticket_aead_result_t ret = - ssl_process_ticket(hs, &session, &unused_renew, CBS_data(&ticket), - CBS_len(&ticket), NULL, 0); + ssl_process_ticket(hs, &session, &unused_renew, ticket, {}); switch (ret) { case ssl_ticket_aead_success: break;