Maintain the sequence number as a uint64_t.
We spend a lot of effort implementing a big-endian sequence number
update, etc., when the sequence number is just a 64-bit counter. (Or
48-bit counter in DTLS because we currently retain the epoch
separately. We can probably tidy that a bit too, but I'll leave that
for later. Right now the DTLS record layer state is a bit entwined
with the TLS one.)
Just store it as uint64_t. This should also simplify
https://boringssl-review.googlesource.com/c/boringssl/+/54325 a little.
Change-Id: I95233f924a660bc523b21496fdc9211055b75073
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/54505
Reviewed-by: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/ssl/dtls_method.cc b/ssl/dtls_method.cc
index a28dcdc..677418b 100644
--- a/ssl/dtls_method.cc
+++ b/ssl/dtls_method.cc
@@ -90,7 +90,7 @@
ssl->d1->r_epoch++;
OPENSSL_memset(&ssl->d1->bitmap, 0, sizeof(ssl->d1->bitmap));
- OPENSSL_memset(ssl->s3->read_sequence, 0, sizeof(ssl->s3->read_sequence));
+ ssl->s3->read_sequence = 0;
ssl->s3->aead_read_ctx = std::move(aead_ctx);
ssl->s3->read_level = level;
@@ -103,9 +103,8 @@
Span<const uint8_t> secret_for_quic) {
assert(secret_for_quic.empty()); // QUIC does not use DTLS.
ssl->d1->w_epoch++;
- OPENSSL_memcpy(ssl->d1->last_write_sequence, ssl->s3->write_sequence,
- sizeof(ssl->s3->write_sequence));
- OPENSSL_memset(ssl->s3->write_sequence, 0, sizeof(ssl->s3->write_sequence));
+ ssl->d1->last_write_sequence = ssl->s3->write_sequence;
+ ssl->s3->write_sequence = 0;
ssl->d1->last_aead_write_ctx = std::move(ssl->s3->aead_write_ctx);
ssl->s3->aead_write_ctx = std::move(aead_ctx);
diff --git a/ssl/dtls_record.cc b/ssl/dtls_record.cc
index 992fb52..eb3df69 100644
--- a/ssl/dtls_record.cc
+++ b/ssl/dtls_record.cc
@@ -123,52 +123,37 @@
BSSL_NAMESPACE_BEGIN
-// to_u64_be treats |in| as a 8-byte big-endian integer and returns the value as
-// a |uint64_t|.
-static uint64_t to_u64_be(const uint8_t in[8]) {
- uint64_t ret = 0;
- unsigned i;
- for (i = 0; i < 8; i++) {
- ret <<= 8;
- ret |= in[i];
- }
- return ret;
-}
-
// dtls1_bitmap_should_discard returns one if |seq_num| has been seen in
// |bitmap| or is stale. Otherwise it returns zero.
static bool dtls1_bitmap_should_discard(DTLS1_BITMAP *bitmap,
- const uint8_t seq_num[8]) {
+ uint64_t seq_num) {
const unsigned kWindowSize = sizeof(bitmap->map) * 8;
- uint64_t seq_num_u = to_u64_be(seq_num);
- if (seq_num_u > bitmap->max_seq_num) {
+ if (seq_num > bitmap->max_seq_num) {
return false;
}
- uint64_t idx = bitmap->max_seq_num - seq_num_u;
+ uint64_t idx = bitmap->max_seq_num - seq_num;
return idx >= kWindowSize || (bitmap->map & (((uint64_t)1) << idx));
}
// dtls1_bitmap_record updates |bitmap| to record receipt of sequence number
// |seq_num|. It slides the window forward if needed. It is an error to call
// this function on a stale sequence number.
-static void dtls1_bitmap_record(DTLS1_BITMAP *bitmap,
- const uint8_t seq_num[8]) {
+static void dtls1_bitmap_record(DTLS1_BITMAP *bitmap, uint64_t seq_num) {
const unsigned kWindowSize = sizeof(bitmap->map) * 8;
- uint64_t seq_num_u = to_u64_be(seq_num);
// Shift the window if necessary.
- if (seq_num_u > bitmap->max_seq_num) {
- uint64_t shift = seq_num_u - bitmap->max_seq_num;
+ if (seq_num > bitmap->max_seq_num) {
+ uint64_t shift = seq_num - bitmap->max_seq_num;
if (shift >= kWindowSize) {
bitmap->map = 0;
} else {
bitmap->map <<= shift;
}
- bitmap->max_seq_num = seq_num_u;
+ bitmap->max_seq_num = seq_num;
}
- uint64_t idx = bitmap->max_seq_num - seq_num_u;
+ uint64_t idx = bitmap->max_seq_num - seq_num;
if (idx < kWindowSize) {
bitmap->map |= ((uint64_t)1) << idx;
}
@@ -192,11 +177,11 @@
// Decode the record.
uint8_t type;
uint16_t version;
- uint8_t sequence[8];
+ uint8_t sequence_bytes[8];
CBS body;
if (!CBS_get_u8(&cbs, &type) ||
!CBS_get_u16(&cbs, &version) ||
- !CBS_copy_bytes(&cbs, sequence, 8) ||
+ !CBS_copy_bytes(&cbs, sequence_bytes, sizeof(sequence_bytes)) ||
!CBS_get_u16_length_prefixed(&cbs, &body) ||
CBS_len(&body) > SSL3_RT_MAX_ENCRYPTED_LENGTH) {
// The record header was incomplete or malformed. Drop the entire packet.
@@ -222,7 +207,8 @@
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];
+ uint64_t sequence = CRYPTO_load_u64_be(sequence_bytes);
+ uint16_t epoch = static_cast<uint16_t>(sequence >> 48);
if (epoch != ssl->d1->r_epoch ||
dtls1_bitmap_should_discard(&ssl->d1->bitmap, sequence)) {
// Drop this record. It's from the wrong epoch or is a replay. Note that if
@@ -304,12 +290,12 @@
// Determine the parameters for the current epoch.
uint16_t epoch = ssl->d1->w_epoch;
SSLAEADContext *aead = ssl->s3->aead_write_ctx.get();
- uint8_t *seq = ssl->s3->write_sequence;
+ uint64_t *seq = &ssl->s3->write_sequence;
if (use_epoch == dtls1_use_previous_epoch) {
assert(ssl->d1->w_epoch >= 1);
epoch = ssl->d1->w_epoch - 1;
aead = ssl->d1->last_aead_write_ctx.get();
- seq = ssl->d1->last_write_sequence;
+ seq = &ssl->d1->last_write_sequence;
}
if (max_out < DTLS1_RT_HEADER_LENGTH) {
@@ -323,9 +309,15 @@
out[1] = record_version >> 8;
out[2] = record_version & 0xff;
- out[3] = epoch >> 8;
- out[4] = epoch & 0xff;
- OPENSSL_memcpy(&out[5], &seq[2], 6);
+ // Ensure the sequence number update does not overflow.
+ const uint64_t kMaxSequenceNumber = (uint64_t{1} << 48) - 1;
+ if (*seq + 1 > kMaxSequenceNumber) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
+ return false;
+ }
+
+ uint64_t seq_with_epoch = (uint64_t{epoch} << 48) | *seq;
+ CRYPTO_store_u64_be(&out[3], seq_with_epoch);
size_t ciphertext_len;
if (!aead->CiphertextLen(&ciphertext_len, in_len, 0)) {
@@ -339,12 +331,12 @@
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)) {
+ seq_with_epoch, header, in, in_len)) {
return false;
}
assert(ciphertext_len == len_copy);
+ (*seq)++;
*out_len = DTLS1_RT_HEADER_LENGTH + ciphertext_len;
ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, header);
return true;
diff --git a/ssl/handoff.cc b/ssl/handoff.cc
index 736ab47..dd24811 100644
--- a/ssl/handoff.cc
+++ b/ssl/handoff.cc
@@ -17,6 +17,7 @@
#include <openssl/bytestring.h>
#include <openssl/err.h>
+#include "../crypto/internal.h"
#include "internal.h"
@@ -338,14 +339,16 @@
} else {
session = s3->session_reused ? ssl->session.get() : hs->new_session.get();
}
+ uint8_t read_sequence[8], write_sequence[8];
+ CRYPTO_store_u64_be(read_sequence, s3->read_sequence);
+ CRYPTO_store_u64_be(write_sequence, s3->write_sequence);
static const uint8_t kUnusedChannelID[64] = {0};
if (!CBB_add_asn1(out, &seq, CBS_ASN1_SEQUENCE) ||
!CBB_add_asn1_uint64(&seq, kHandbackVersion) ||
!CBB_add_asn1_uint64(&seq, type) ||
- !CBB_add_asn1_octet_string(&seq, s3->read_sequence,
- sizeof(s3->read_sequence)) ||
- !CBB_add_asn1_octet_string(&seq, s3->write_sequence,
- sizeof(s3->write_sequence)) ||
+ !CBB_add_asn1_octet_string(&seq, read_sequence, sizeof(read_sequence)) ||
+ !CBB_add_asn1_octet_string(&seq, write_sequence,
+ sizeof(write_sequence)) ||
!CBB_add_asn1_octet_string(&seq, s3->server_random,
sizeof(s3->server_random)) ||
!CBB_add_asn1_octet_string(&seq, s3->client_random,
@@ -366,7 +369,7 @@
sizeof(kUnusedChannelID)) ||
// These two fields were historically |token_binding_negotiated| and
// |negotiated_token_binding_param|.
- !CBB_add_asn1_bool(&seq, 0) ||
+ !CBB_add_asn1_bool(&seq, 0) || //
!CBB_add_asn1_uint64(&seq, 0) ||
!CBB_add_asn1_bool(&seq, s3->hs->next_proto_neg_seen) ||
!CBB_add_asn1_bool(&seq, s3->hs->cert_request) ||
@@ -694,11 +697,13 @@
}
break;
}
- if (!CopyExact({s3->read_sequence, sizeof(s3->read_sequence)}, &read_seq) ||
- !CopyExact({s3->write_sequence, sizeof(s3->write_sequence)},
- &write_seq)) {
+ uint8_t read_sequence[8], write_sequence[8];
+ if (!CopyExact(read_sequence, &read_seq) ||
+ !CopyExact(write_sequence, &write_seq)) {
return false;
}
+ s3->read_sequence = CRYPTO_load_u64_be(read_sequence);
+ s3->write_sequence = CRYPTO_load_u64_be(write_sequence);
if (type == handback_after_ecdhe &&
(hs->key_shares[0] = SSLKeyShare::Create(&key_share)) == nullptr) {
return false;
diff --git a/ssl/internal.h b/ssl/internal.h
index 71ff0ff..8ef1509 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -829,15 +829,14 @@
// 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<const uint8_t> header,
- Span<uint8_t> in);
+ uint64_t seqnum, 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],
+ uint16_t record_version, uint64_t seqnum,
Span<const uint8_t> header, const uint8_t *in, size_t in_len);
// SealScatter encrypts and authenticates |in_len| bytes from |in| and splits
@@ -856,10 +855,9 @@
// If |in| and |out| alias then |out| must be == |in|. Other arguments may not
// 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], Span<const uint8_t> header,
- const uint8_t *in, size_t in_len, const uint8_t *extra_in,
- size_t extra_in_len);
+ uint8_t type, uint16_t record_version, uint64_t seqnum,
+ 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;
@@ -868,8 +866,7 @@
// 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,
+ uint64_t seqnum, size_t plaintext_len,
Span<const uint8_t> header);
const SSL_CIPHER *cipher_;
@@ -916,10 +913,6 @@
// Record layer.
-// ssl_record_sequence_update increments the sequence number in |seq|. It
-// returns true on success and false on wraparound.
-bool ssl_record_sequence_update(uint8_t *seq, size_t seq_len);
-
// ssl_record_prefix_len returns the length of the prefix before the ciphertext
// of a record for |ssl|.
//
@@ -2644,8 +2637,8 @@
SSL3_STATE();
~SSL3_STATE();
- uint8_t read_sequence[8] = {0};
- uint8_t write_sequence[8] = {0};
+ uint64_t read_sequence = 0;
+ uint64_t write_sequence = 0;
uint8_t server_random[SSL3_RANDOM_SIZE] = {0};
uint8_t client_random[SSL3_RANDOM_SIZE] = {0};
@@ -2935,7 +2928,7 @@
uint16_t handshake_read_seq = 0;
// save last sequence number for retransmissions
- uint8_t last_write_sequence[8] = {0};
+ uint64_t last_write_sequence = 0;
UniquePtr<SSLAEADContext> last_aead_write_ctx;
// incoming_messages is a ring buffer of incoming handshake messages that have
diff --git a/ssl/ssl_aead_ctx.cc b/ssl/ssl_aead_ctx.cc
index 0bad266..27f0084 100644
--- a/ssl/ssl_aead_ctx.cc
+++ b/ssl/ssl_aead_ctx.cc
@@ -220,13 +220,13 @@
}
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) {
+ uint8_t storage[13], uint8_t type, uint16_t record_version, uint64_t seqnum,
+ size_t plaintext_len, Span<const uint8_t> header) {
if (ad_is_header_) {
return header;
}
- OPENSSL_memcpy(storage, seqnum, 8);
+ CRYPTO_store_u64_be(storage, seqnum);
size_t len = 8;
storage[len++] = type;
storage[len++] = static_cast<uint8_t>((record_version >> 8));
@@ -239,7 +239,7 @@
}
bool SSLAEADContext::Open(Span<uint8_t> *out, uint8_t type,
- uint16_t record_version, const uint8_t seqnum[8],
+ uint16_t record_version, uint64_t seqnum,
Span<const uint8_t> header, Span<uint8_t> in) {
if (is_null_cipher() || FUZZER_MODE) {
// Handle the initial NULL cipher.
@@ -288,7 +288,7 @@
in = in.subspan(variable_nonce_len_);
} else {
assert(variable_nonce_len_ == 8);
- OPENSSL_memcpy(nonce + nonce_len, seqnum, variable_nonce_len_);
+ CRYPTO_store_u64_be(nonce + nonce_len, seqnum);
}
nonce_len += variable_nonce_len_;
@@ -313,8 +313,7 @@
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],
+ uint16_t record_version, uint64_t seqnum,
Span<const uint8_t> header, const uint8_t *in,
size_t in_len, const uint8_t *extra_in,
size_t extra_in_len) {
@@ -365,7 +364,7 @@
// When sending we use the sequence number as the variable part of the
// nonce.
assert(variable_nonce_len_ == 8);
- OPENSSL_memcpy(nonce + nonce_len, seqnum, variable_nonce_len_);
+ CRYPTO_store_u64_be(nonce + nonce_len, seqnum);
}
nonce_len += variable_nonce_len_;
@@ -398,7 +397,7 @@
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], Span<const uint8_t> header,
+ uint64_t seqnum, Span<const uint8_t> header,
const uint8_t *in, size_t in_len) {
const size_t prefix_len = ExplicitNonceLen();
size_t suffix_len;
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 1b2e9f4..4d56d37 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -2823,20 +2823,19 @@
}
uint64_t SSL_get_read_sequence(const SSL *ssl) {
- // TODO(davidben): Internally represent sequence numbers as uint64_t.
if (SSL_is_dtls(ssl)) {
// max_seq_num already includes the epoch.
assert(ssl->d1->r_epoch == (ssl->d1->bitmap.max_seq_num >> 48));
return ssl->d1->bitmap.max_seq_num;
}
- return CRYPTO_load_u64_be(ssl->s3->read_sequence);
+ return ssl->s3->read_sequence;
}
uint64_t SSL_get_write_sequence(const SSL *ssl) {
- uint64_t ret = CRYPTO_load_u64_be(ssl->s3->write_sequence);
+ uint64_t ret = ssl->s3->write_sequence;
if (SSL_is_dtls(ssl)) {
assert((ret >> 48) == 0);
- ret |= ((uint64_t)ssl->d1->w_epoch) << 48;
+ ret |= uint64_t{ssl->d1->w_epoch} << 48;
}
return ret;
}
diff --git a/ssl/tls_method.cc b/ssl/tls_method.cc
index 326cbe7..5fcf684 100644
--- a/ssl/tls_method.cc
+++ b/ssl/tls_method.cc
@@ -108,7 +108,7 @@
}
}
- OPENSSL_memset(ssl->s3->read_sequence, 0, sizeof(ssl->s3->read_sequence));
+ ssl->s3->read_sequence = 0;
ssl->s3->aead_read_ctx = std::move(aead_ctx);
ssl->s3->read_level = level;
return true;
@@ -137,7 +137,7 @@
}
}
- OPENSSL_memset(ssl->s3->write_sequence, 0, sizeof(ssl->s3->write_sequence));
+ ssl->s3->write_sequence = 0;
ssl->s3->aead_write_ctx = std::move(aead_ctx);
ssl->s3->write_level = level;
return true;
diff --git a/ssl/tls_record.cc b/ssl/tls_record.cc
index acff1ad..88b8ef9 100644
--- a/ssl/tls_record.cc
+++ b/ssl/tls_record.cc
@@ -151,17 +151,6 @@
#endif
}
-bool ssl_record_sequence_update(uint8_t *seq, size_t seq_len) {
- for (size_t i = seq_len - 1; i < seq_len; i--) {
- ++seq[i];
- if (seq[i] != 0) {
- return true;
- }
- }
- OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
- return false;
-}
-
size_t ssl_record_prefix_len(const SSL *ssl) {
size_t header_len;
if (SSL_is_dtls(ssl)) {
@@ -286,6 +275,13 @@
return skip_early_data(ssl, out_alert, *out_consumed);
}
+ // Ensure the sequence number update does not overflow.
+ if (ssl->s3->read_sequence + 1 == 0) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
+ *out_alert = SSL_AD_INTERNAL_ERROR;
+ return ssl_open_record_error;
+ }
+
// Decrypt the body in-place.
if (!ssl->s3->aead_read_ctx->Open(
out, type, version, ssl->s3->read_sequence, header,
@@ -301,11 +297,7 @@
}
ssl->s3->skip_early_data = false;
-
- if (!ssl_record_sequence_update(ssl->s3->read_sequence, 8)) {
- *out_alert = SSL_AD_INTERNAL_ERROR;
- return ssl_open_record_error;
- }
+ ssl->s3->read_sequence++;
// TLS 1.3 hides the record type inside the encrypted data.
bool has_padding =
@@ -411,13 +403,19 @@
out_prefix[4] = ciphertext_len & 0xff;
Span<const uint8_t> header = MakeSpan(out_prefix, SSL3_RT_HEADER_LENGTH);
- 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)) {
+ // Ensure the sequence number update does not overflow.
+ if (ssl->s3->write_sequence + 1 == 0) {
+ OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW);
return false;
}
+ 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)) {
+ return false;
+ }
+
+ ssl->s3->write_sequence++;
ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, header);
return true;
}
