Use the actual record header, rather than reassembling it. The last-minute TLS 1.3 change was done partly for consistency with DTLS 1.3, where authenticating the record header is less obviously pointless than in TLS. There, reconstructing it would be messy. Instead, pass in the record header and let SSLAEADContext decide whether or not to assemble its own. (While I'm here, reorder all the flags so the AD and nonce ones are grouped together.) Change-Id: I06e65d526b21a08019e5ca6f1b7c7e0e579e7760 Reviewed-on: https://boringssl-review.googlesource.com/27024 Commit-Queue: Steven Valdez <svaldez@google.com> Reviewed-by: Steven Valdez <svaldez@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/dtls_record.cc b/ssl/dtls_record.cc index 5e795fa..d348601 100644 --- a/ssl/dtls_record.cc +++ b/ssl/dtls_record.cc
@@ -219,8 +219,8 @@ return ssl_open_record_discard; } - ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HEADER, - in.subspan(0, DTLS1_RT_HEADER_LENGTH)); + Span<const uint8_t> header = in.subspan(0, DTLS1_RT_HEADER_LENGTH); + ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HEADER, header); uint16_t epoch = (((uint16_t)sequence[0]) << 8) | sequence[1]; if (epoch != ssl->d1->r_epoch || @@ -235,7 +235,7 @@ // discard the body in-place. if (!ssl->s3->aead_read_ctx->Open( - out, type, version, sequence, + out, type, version, sequence, header, MakeSpan(const_cast<uint8_t *>(CBS_data(&body)), CBS_len(&body)))) { // Bad packets are silently dropped in DTLS. See section 4.2.1 of RFC 6347. // Clear the error queue of any errors decryption may have added. Drop the @@ -328,25 +328,25 @@ OPENSSL_memcpy(&out[5], &seq[2], 6); size_t ciphertext_len; - if (!aead->Seal(out + DTLS1_RT_HEADER_LENGTH, &ciphertext_len, - max_out - DTLS1_RT_HEADER_LENGTH, type, record_version, - &out[3] /* seq */, in, in_len) || - !ssl_record_sequence_update(&seq[2], 6)) { - return 0; - } - - if (ciphertext_len >= 1 << 16) { - OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW); + if (!aead->CiphertextLen(&ciphertext_len, in_len, 0)) { + OPENSSL_PUT_ERROR(SSL, SSL_R_RECORD_TOO_LARGE); return 0; } out[11] = ciphertext_len >> 8; out[12] = ciphertext_len & 0xff; + Span<const uint8_t> header = MakeConstSpan(out, DTLS1_RT_HEADER_LENGTH); + + size_t len_copy; + if (!aead->Seal(out + DTLS1_RT_HEADER_LENGTH, &len_copy, + max_out - DTLS1_RT_HEADER_LENGTH, type, record_version, + &out[3] /* seq */, header, in, in_len) || + !ssl_record_sequence_update(&seq[2], 6)) { + return 0; + } + assert(ciphertext_len == len_copy); *out_len = DTLS1_RT_HEADER_LENGTH + ciphertext_len; - - ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, - MakeSpan(out, DTLS1_RT_HEADER_LENGTH)); - + ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, header); return 1; }
diff --git a/ssl/internal.h b/ssl/internal.h index e884b63..e69b382 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -657,19 +657,26 @@ bool SuffixLen(size_t *out_suffix_len, size_t in_len, size_t extra_in_len) const; + // CiphertextLen calculates the total ciphertext length written by + // |SealScatter| and writes it to |*out_len|. It returns true on success and + // false on error. |in_len| and |extra_in_len| should equal the argument of + // the same names passed to |SealScatter|. + bool CiphertextLen(size_t *out_len, size_t in_len, size_t extra_in_len) const; + // Open authenticates and decrypts |in| in-place. On success, it sets |*out| // to the plaintext in |in| and returns true. Otherwise, it returns // false. The output will always be |ExplicitNonceLen| bytes ahead of |in|. bool Open(Span<uint8_t> *out, uint8_t type, uint16_t record_version, - const uint8_t seqnum[8], Span<uint8_t> in); + const uint8_t seqnum[8], Span<const uint8_t> header, + Span<uint8_t> in); // Seal encrypts and authenticates |in_len| bytes from |in| and writes the // result to |out|. It returns true on success and false on error. // // If |in| and |out| alias then |out| + |ExplicitNonceLen| must be == |in|. bool Seal(uint8_t *out, size_t *out_len, size_t max_out, uint8_t type, - uint16_t record_version, const uint8_t seqnum[8], const uint8_t *in, - size_t in_len); + uint16_t record_version, const uint8_t seqnum[8], + Span<const uint8_t> header, const uint8_t *in, size_t in_len); // SealScatter encrypts and authenticates |in_len| bytes from |in| and splits // the result between |out_prefix|, |out| and |out_suffix|. It returns one on @@ -688,17 +695,20 @@ // alias anything. bool SealScatter(uint8_t *out_prefix, uint8_t *out, uint8_t *out_suffix, uint8_t type, uint16_t record_version, - const uint8_t seqnum[8], const uint8_t *in, size_t in_len, - const uint8_t *extra_in, size_t extra_in_len); + const uint8_t seqnum[8], Span<const uint8_t> header, + const uint8_t *in, size_t in_len, const uint8_t *extra_in, + size_t extra_in_len); bool GetIV(const uint8_t **out_iv, size_t *out_iv_len) const; private: - // GetAdditionalData writes the additional data into |out| and returns the - // number of bytes written. - size_t GetAdditionalData(uint8_t out[13], uint8_t type, - uint16_t record_version, const uint8_t seqnum[8], - size_t plaintext_len, size_t ciphertext_len); + // GetAdditionalData returns the additional data, writing into |storage| if + // necessary. + Span<const uint8_t> GetAdditionalData(uint8_t storage[13], uint8_t type, + uint16_t record_version, + const uint8_t seqnum[8], + size_t plaintext_len, + Span<const uint8_t> header); const SSL_CIPHER *cipher_; ScopedEVP_AEAD_CTX ctx_; @@ -717,6 +727,9 @@ // randomly generated, rather than derived from the sequence // number. bool random_variable_nonce_ : 1; + // xor_fixed_nonce_ is true if the fixed nonce should be XOR'd into the + // variable nonce rather than prepended. + bool xor_fixed_nonce_ : 1; // omit_length_in_ad_ is true if the length should be omitted in the // AEAD's ad parameter. bool omit_length_in_ad_ : 1; @@ -725,12 +738,8 @@ bool omit_version_in_ad_ : 1; // omit_ad_ is true if the AEAD's ad parameter should be omitted. bool omit_ad_ : 1; - // tls13_ad_ is true if the AEAD's ad parameter should be based on the - // TLS 1.3 format. - bool tls13_ad_ : 1; - // xor_fixed_nonce_ is true if the fixed nonce should be XOR'd into the - // variable nonce rather than prepended. - bool xor_fixed_nonce_ : 1; + // ad_is_header_ is true if the AEAD's ad parameter is the record header. + bool ad_is_header_ : 1; };
diff --git a/ssl/ssl_aead_ctx.cc b/ssl/ssl_aead_ctx.cc index e6b3ee9..363c959 100644 --- a/ssl/ssl_aead_ctx.cc +++ b/ssl/ssl_aead_ctx.cc
@@ -40,11 +40,11 @@ is_dtls_(is_dtls_arg), variable_nonce_included_in_record_(false), random_variable_nonce_(false), + xor_fixed_nonce_(false), omit_length_in_ad_(false), omit_version_in_ad_(false), omit_ad_(false), - tls13_ad_(false), - xor_fixed_nonce_(false) { + ad_is_header_(false) { OPENSSL_memset(fixed_nonce_, 0, sizeof(fixed_nonce_)); } @@ -136,7 +136,7 @@ aead_ctx->variable_nonce_len_ = 8; aead_ctx->variable_nonce_included_in_record_ = false; if (ssl_is_draft28(version)) { - aead_ctx->tls13_ad_ = true; + aead_ctx->ad_is_header_ = true; } else { aead_ctx->omit_ad_ = true; } @@ -198,6 +198,22 @@ extra_in_len); } +bool SSLAEADContext::CiphertextLen(size_t *out_len, const size_t in_len, + const size_t extra_in_len) const { + size_t len; + if (!SuffixLen(&len, in_len, extra_in_len)) { + return false; + } + len += ExplicitNonceLen(); + len += in_len; + if (len < in_len || len >= 0xffff) { + OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW); + return false; + } + *out_len = len; + return true; +} + size_t SSLAEADContext::MaxOverhead() const { return ExplicitNonceLen() + (is_null_cipher() || FUZZER_MODE @@ -205,38 +221,34 @@ : EVP_AEAD_max_overhead(EVP_AEAD_CTX_aead(ctx_.get()))); } -size_t SSLAEADContext::GetAdditionalData(uint8_t out[13], uint8_t type, - uint16_t record_version, - const uint8_t seqnum[8], - size_t plaintext_len, - size_t ciphertext_len) { - if (omit_ad_) { - return 0; +Span<const uint8_t> SSLAEADContext::GetAdditionalData( + uint8_t storage[13], uint8_t type, uint16_t record_version, + const uint8_t seqnum[8], size_t plaintext_len, Span<const uint8_t> header) { + if (ad_is_header_) { + return header; } - size_t len = 0; - if (!tls13_ad_) { - OPENSSL_memcpy(out, seqnum, 8); - len += 8; + if (omit_ad_) { + return {}; } - out[len++] = type; + + OPENSSL_memcpy(storage, seqnum, 8); + size_t len = 8; + storage[len++] = type; if (!omit_version_in_ad_) { - out[len++] = static_cast<uint8_t>((record_version >> 8)); - out[len++] = static_cast<uint8_t>(record_version); + storage[len++] = static_cast<uint8_t>((record_version >> 8)); + storage[len++] = static_cast<uint8_t>(record_version); } - if (tls13_ad_) { - out[len++] = static_cast<uint8_t>((ciphertext_len >> 8)); - out[len++] = static_cast<uint8_t>(ciphertext_len); - } else if (!omit_length_in_ad_) { - out[len++] = static_cast<uint8_t>((plaintext_len >> 8)); - out[len++] = static_cast<uint8_t>(plaintext_len); + if (!omit_length_in_ad_) { + storage[len++] = static_cast<uint8_t>((plaintext_len >> 8)); + storage[len++] = static_cast<uint8_t>(plaintext_len); } - return len; + return MakeConstSpan(storage, len); } bool SSLAEADContext::Open(Span<uint8_t> *out, uint8_t type, uint16_t record_version, const uint8_t seqnum[8], - Span<uint8_t> in) { + Span<const uint8_t> header, Span<uint8_t> in) { if (is_null_cipher() || FUZZER_MODE) { // Handle the initial NULL cipher. *out = in; @@ -255,9 +267,10 @@ } plaintext_len = in.size() - overhead; } - uint8_t ad[13]; - size_t ad_len = GetAdditionalData(ad, type, record_version, seqnum, - plaintext_len, in.size()); + + uint8_t ad_storage[13]; + Span<const uint8_t> ad = GetAdditionalData(ad_storage, type, record_version, + seqnum, plaintext_len, header); // Assemble the nonce. uint8_t nonce[EVP_AEAD_MAX_NONCE_LENGTH]; @@ -298,7 +311,8 @@ // Decrypt in-place. size_t len; if (!EVP_AEAD_CTX_open(ctx_.get(), in.data(), &len, in.size(), nonce, - nonce_len, in.data(), in.size(), ad, ad_len)) { + nonce_len, in.data(), in.size(), ad.data(), + ad.size())) { return false; } *out = in.subspan(0, len); @@ -308,7 +322,8 @@ bool SSLAEADContext::SealScatter(uint8_t *out_prefix, uint8_t *out, uint8_t *out_suffix, uint8_t type, uint16_t record_version, - const uint8_t seqnum[8], const uint8_t *in, + const uint8_t seqnum[8], + Span<const uint8_t> header, const uint8_t *in, size_t in_len, const uint8_t *extra_in, size_t extra_in_len) { const size_t prefix_len = ExplicitNonceLen(); @@ -331,9 +346,9 @@ return true; } - uint8_t ad[13]; - size_t ad_len = GetAdditionalData(ad, type, record_version, seqnum, in_len, - in_len + suffix_len); + uint8_t ad_storage[13]; + Span<const uint8_t> ad = GetAdditionalData(ad_storage, type, record_version, + seqnum, in_len, header); // Assemble the nonce. uint8_t nonce[EVP_AEAD_MAX_NONCE_LENGTH]; @@ -384,15 +399,15 @@ size_t written_suffix_len; bool result = !!EVP_AEAD_CTX_seal_scatter( ctx_.get(), out, out_suffix, &written_suffix_len, suffix_len, nonce, - nonce_len, in, in_len, extra_in, extra_in_len, ad, ad_len); + nonce_len, in, in_len, extra_in, extra_in_len, ad.data(), ad.size()); assert(!result || written_suffix_len == suffix_len); return result; } bool SSLAEADContext::Seal(uint8_t *out, size_t *out_len, size_t max_out_len, uint8_t type, uint16_t record_version, - const uint8_t seqnum[8], const uint8_t *in, - size_t in_len) { + const uint8_t seqnum[8], Span<const uint8_t> header, + const uint8_t *in, size_t in_len) { const size_t prefix_len = ExplicitNonceLen(); size_t suffix_len; if (!SuffixLen(&suffix_len, in_len, 0)) { @@ -410,7 +425,7 @@ } if (!SealScatter(out, out + prefix_len, out + prefix_len + in_len, type, - record_version, seqnum, in, in_len, 0, 0)) { + record_version, seqnum, header, in, in_len, 0, 0)) { return false; } *out_len = prefix_len + in_len + suffix_len;
diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc index 3152e7a..a2e4a20 100644 --- a/ssl/tls_record.cc +++ b/ssl/tls_record.cc
@@ -258,8 +258,8 @@ return ssl_open_record_partial; } - ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HEADER, - in.subspan(0, SSL3_RT_HEADER_LENGTH)); + Span<const uint8_t> header = in.subspan(0, SSL3_RT_HEADER_LENGTH); + ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HEADER, header); *out_consumed = in.size() - CBS_len(&cbs); @@ -288,7 +288,7 @@ // Decrypt the body in-place. if (!ssl->s3->aead_read_ctx->Open( - out, type, version, ssl->s3->read_sequence, + out, type, version, ssl->s3->read_sequence, header, MakeSpan(const_cast<uint8_t *>(CBS_data(&body)), CBS_len(&body)))) { if (ssl->s3->skip_early_data && !ssl->s3->aead_read_ctx->is_null_cipher()) { ERR_clear_error(); @@ -376,27 +376,22 @@ static int do_seal_record(SSL *ssl, uint8_t *out_prefix, uint8_t *out, uint8_t *out_suffix, uint8_t type, const uint8_t *in, const size_t in_len) { + SSLAEADContext *aead = ssl->s3->aead_write_ctx.get(); uint8_t *extra_in = NULL; size_t extra_in_len = 0; - if (!ssl->s3->aead_write_ctx->is_null_cipher() && - ssl->s3->aead_write_ctx->ProtocolVersion() >= TLS1_3_VERSION) { + if (!aead->is_null_cipher() && + aead->ProtocolVersion() >= TLS1_3_VERSION) { // TLS 1.3 hides the actual record type inside the encrypted data. extra_in = &type; extra_in_len = 1; } - size_t suffix_len; - if (!ssl->s3->aead_write_ctx->SuffixLen(&suffix_len, in_len, extra_in_len)) { + size_t suffix_len, ciphertext_len; + if (!aead->SuffixLen(&suffix_len, in_len, extra_in_len) || + !aead->CiphertextLen(&ciphertext_len, in_len, extra_in_len)) { OPENSSL_PUT_ERROR(SSL, SSL_R_RECORD_TOO_LARGE); return 0; } - size_t ciphertext_len = - ssl->s3->aead_write_ctx->ExplicitNonceLen() + suffix_len; - if (ciphertext_len + in_len < ciphertext_len) { - OPENSSL_PUT_ERROR(SSL, SSL_R_RECORD_TOO_LARGE); - return 0; - } - ciphertext_len += in_len; assert(in == out || !buffers_alias(in, in_len, out, in_len)); assert(!buffers_alias(in, in_len, out_prefix, ssl_record_prefix_len(ssl))); @@ -408,28 +403,27 @@ out_prefix[0] = type; } - uint16_t record_version = ssl->s3->aead_write_ctx->RecordVersion(); + uint16_t record_version = aead->RecordVersion(); out_prefix[1] = record_version >> 8; out_prefix[2] = record_version & 0xff; out_prefix[3] = ciphertext_len >> 8; out_prefix[4] = ciphertext_len & 0xff; + Span<const uint8_t> header = MakeSpan(out_prefix, SSL3_RT_HEADER_LENGTH); - if (!ssl->s3->aead_write_ctx->SealScatter( - out_prefix + SSL3_RT_HEADER_LENGTH, out, out_suffix, out_prefix[0], - record_version, ssl->s3->write_sequence, in, in_len, extra_in, - extra_in_len) || + if (!aead->SealScatter(out_prefix + SSL3_RT_HEADER_LENGTH, out, out_suffix, + out_prefix[0], record_version, ssl->s3->write_sequence, + header, in, in_len, extra_in, extra_in_len) || !ssl_record_sequence_update(ssl->s3->write_sequence, 8)) { return 0; } - ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, - MakeSpan(out_prefix, SSL3_RT_HEADER_LENGTH)); + ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, header); return 1; } static size_t tls_seal_scatter_prefix_len(const SSL *ssl, uint8_t type, - size_t in_len) { + size_t in_len) { size_t ret = SSL3_RT_HEADER_LENGTH; if (type == SSL3_RT_APPLICATION_DATA && in_len > 1 && ssl_needs_record_splitting(ssl)) {