diff --git a/ssl/d1_both.cc b/ssl/d1_both.cc index dd7d14605..4bd30b623 100644 --- a/ssl/d1_both.cc +++ b/ssl/d1_both.cc @@ -412,8 +412,10 @@ bool dtls1_process_handshake_fragments(SSL *ssl, uint8_t *out_alert, ssl_open_record_t dtls1_open_handshake(SSL *ssl, size_t *out_consumed, uint8_t *out_alert, Span in) { uint8_t type; + DTLSRecordNumber record_number; Span record; - auto ret = dtls_open_record(ssl, &type, &record, out_consumed, out_alert, in); + auto ret = dtls_open_record(ssl, &type, &record_number, &record, out_consumed, + out_alert, in); if (ret != ssl_open_record_success) { return ret; } @@ -709,7 +711,8 @@ static enum seal_result_t seal_next_message(SSL *ssl, uint8_t *out, return seal_no_progress; } - if (!dtls_seal_record(ssl, out, out_len, max_out, + DTLSRecordNumber record_number; + if (!dtls_seal_record(ssl, &record_number, out, out_len, max_out, SSL3_RT_CHANGE_CIPHER_SPEC, kChangeCipherSpec, sizeof(kChangeCipherSpec), msg->epoch)) { return seal_error; @@ -763,8 +766,10 @@ static enum seal_result_t seal_next_message(SSL *ssl, uint8_t *out, ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HANDSHAKE, MakeSpan(frag, frag_len)); - if (!dtls_seal_record(ssl, out, out_len, max_out, SSL3_RT_HANDSHAKE, - out + prefix, frag_len, msg->epoch)) { + DTLSRecordNumber record_number; + if (!dtls_seal_record(ssl, &record_number, out, out_len, max_out, + SSL3_RT_HANDSHAKE, out + prefix, frag_len, + msg->epoch)) { return seal_error; } diff --git a/ssl/d1_pkt.cc b/ssl/d1_pkt.cc index e768e0397..18cd5eee0 100644 --- a/ssl/d1_pkt.cc +++ b/ssl/d1_pkt.cc @@ -149,8 +149,10 @@ ssl_open_record_t dtls1_open_app_data(SSL *ssl, Span *out, assert(!SSL_in_init(ssl)); uint8_t type; + DTLSRecordNumber record_number; Span record; - auto ret = dtls_open_record(ssl, &type, &record, out_consumed, out_alert, in); + auto ret = dtls_open_record(ssl, &type, &record_number, &record, out_consumed, + out_alert, in); if (ret != ssl_open_record_success) { return ret; } @@ -259,12 +261,13 @@ int dtls1_write_record(SSL *ssl, int type, Span in, return -1; } + DTLSRecordNumber record_number; size_t ciphertext_len; if (!buf->EnsureCap(dtls_seal_prefix_len(ssl, epoch), in.size() + SSL_max_seal_overhead(ssl)) || - !dtls_seal_record(ssl, buf->remaining().data(), &ciphertext_len, - buf->remaining().size(), type, in.data(), in.size(), - epoch)) { + !dtls_seal_record(ssl, &record_number, buf->remaining().data(), + &ciphertext_len, buf->remaining().size(), type, + in.data(), in.size(), epoch)) { buf->Clear(); return -1; } diff --git a/ssl/dtls_record.cc b/ssl/dtls_record.cc index f76969618..5ab89bbd1 100644 --- a/ssl/dtls_record.cc +++ b/ssl/dtls_record.cc @@ -123,8 +123,6 @@ BSSL_NAMESPACE_BEGIN -static constexpr uint64_t kMaxSequenceNumber = (uint64_t{1} << 48) - 1; - bool DTLSReplayBitmap::ShouldDiscard(uint64_t seq_num) const { const size_t kWindowSize = map_.size(); @@ -167,6 +165,15 @@ static uint16_t dtls_record_version(const SSL *ssl) { : ssl->s3->version; } +static uint64_t dtls_aead_sequence(const SSL *ssl, DTLSRecordNumber num) { + // DTLS 1.3 uses the sequence number with the AEAD, while DTLS 1.2 uses the + // combined value. If the version is not known, the epoch is unencrypted and + // the value is ignored. + return (ssl->s3->version != 0 && ssl_protocol_version(ssl) >= TLS1_3_VERSION) + ? num.sequence() + : num.combined(); +} + // reconstruct_epoch finds the largest epoch that ends with the epoch bits from // |wire_epoch| that is less than or equal to |current_epoch|, to match the // epoch reconstruction algorithm described in RFC 9147 section 4.2.2. @@ -186,7 +193,7 @@ uint64_t reconstruct_seqnum(uint16_t wire_seq, uint64_t seq_mask, // bytes), no payload, and 16 byte AEAD overhead, sending 2^48 records would // require 5 petabytes. This allows us to continue to pack a DTLS record // number into an 8-byte structure. - assert(max_valid_seqnum <= kMaxSequenceNumber); + assert(max_valid_seqnum <= DTLSRecordNumber::kMaxSequence); assert(seq_mask == 0xff || seq_mask == 0xffff); uint64_t max_seqnum_plus_one = max_valid_seqnum + 1; @@ -195,7 +202,7 @@ uint64_t reconstruct_seqnum(uint16_t wire_seq, uint64_t seq_mask, // This addition cannot overflow. It is at most 2^48 + seq_mask. It, however, // may exceed 2^48-1. uint64_t seqnum = max_seqnum_plus_one + diff; - bool too_large = seqnum > kMaxSequenceNumber; + bool too_large = seqnum > DTLSRecordNumber::kMaxSequence; // If the diff is larger than half the step size, then the closest seqnum // to max_seqnum_plus_one (in Z_{2^64}) is seqnum minus step instead of // seqnum. @@ -205,7 +212,7 @@ uint64_t reconstruct_seqnum(uint16_t wire_seq, uint64_t seq_mask, if (too_large || (closer_is_less && !would_underflow)) { seqnum -= step; } - assert(seqnum <= kMaxSequenceNumber); + assert(seqnum <= DTLSRecordNumber::kMaxSequence); return seqnum; } @@ -215,11 +222,9 @@ static Span cbs_to_writable_bytes(CBS cbs) { struct ParsedDTLSRecord { // read_epoch will be null if the record is for an unrecognized epoch. In that - // case, sequence may be zero if we are unable to decrypt the sequence number. + // case, |number| may be unset. DTLSReadEpoch *read_epoch = nullptr; - // sequence includes the epoch for DTLS 1.2 records and does not include it - // for DTLS 1.3. - uint64_t sequence = 0; + DTLSRecordNumber number; CBS header, body; uint8_t type = 0; uint16_t version = 0; @@ -282,21 +287,24 @@ static bool parse_dtls13_record(SSL *ssl, CBS *in, ParsedDTLSRecord *out) { writable_seq[i] ^= mask[i]; seq = (seq << 8) | writable_seq[i]; } - out->sequence = reconstruct_seqnum(seq, (1 << (seq_len * 8)) - 1, - out->read_epoch->bitmap.max_seq_num()); + uint64_t full_seq = reconstruct_seqnum( + seq, (1 << (seq_len * 8)) - 1, out->read_epoch->bitmap.max_seq_num()); + out->number = DTLSRecordNumber(epoch, full_seq); } return true; } static bool parse_dtls12_record(SSL *ssl, CBS *in, ParsedDTLSRecord *out) { + uint64_t epoch_and_seq; if (!CBS_get_u16(in, &out->version) || // - !CBS_get_u64(in, &out->sequence) || + !CBS_get_u64(in, &epoch_and_seq) || !CBS_get_u16_length_prefixed(in, &out->body)) { return false; } + out->number = DTLSRecordNumber::FromCombined(epoch_and_seq); - uint16_t epoch = static_cast(out->sequence >> 48); + uint16_t epoch = out->number.epoch(); bool version_ok; if (epoch == 0) { // Only check the first byte. Enforcing beyond that can prevent decoding @@ -345,6 +353,7 @@ static bool parse_dtls_record(SSL *ssl, CBS *cbs, ParsedDTLSRecord *out) { } enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, + DTLSRecordNumber *out_number, Span *out, size_t *out_consumed, uint8_t *out_alert, Span in) { @@ -368,7 +377,7 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HEADER, record.header); if (record.read_epoch == nullptr || - record.read_epoch->bitmap.ShouldDiscard(record.sequence)) { + record.read_epoch->bitmap.ShouldDiscard(record.number.sequence())) { // Drop this record. It's from an unknown epoch or is a replay. Note that if // the record is from next epoch, it could be buffered for later. For // simplicity, drop it and expect retransmit to handle it later; DTLS must @@ -377,9 +386,10 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, return ssl_open_record_discard; } - // discard the body in-place. + // Decrypt the body in-place. if (!record.read_epoch->aead->Open(out, record.type, record.version, - record.sequence, record.header, + dtls_aead_sequence(ssl, record.number), + record.header, cbs_to_writable_bytes(record.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 @@ -416,7 +426,7 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, } while (record.type == 0); } - record.read_epoch->bitmap.Record(record.sequence); + record.read_epoch->bitmap.Record(record.number.sequence()); // TODO(davidben): Limit the number of empty records as in TLS? This is only // useful if we also limit discarded packets. @@ -428,6 +438,7 @@ enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, ssl->s3->warning_alert_count = 0; *out_type = record.type; + *out_number = record.number; return ssl_open_record_success; } @@ -479,9 +490,9 @@ size_t dtls_seal_prefix_len(const SSL *ssl, uint16_t epoch) { write_epoch->aead->ExplicitNonceLen(); } -bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, - uint8_t type, const uint8_t *in, size_t in_len, - uint16_t epoch) { +bool dtls_seal_record(SSL *ssl, DTLSRecordNumber *out_number, uint8_t *out, + size_t *out_len, size_t max_out, uint8_t type, + const uint8_t *in, size_t in_len, uint16_t epoch) { const size_t prefix = dtls_seal_prefix_len(ssl, epoch); if (buffers_alias(in, in_len, out, max_out) && (max_out < prefix || out + prefix != in)) { @@ -499,7 +510,7 @@ bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, const size_t record_header_len = dtls_record_header_write_len(ssl, epoch); // Ensure the sequence number update does not overflow. - if (write_epoch->next_seq + 1 > kMaxSequenceNumber) { + if (write_epoch->next_seq + 1 > DTLSRecordNumber::kMaxSequence) { OPENSSL_PUT_ERROR(SSL, ERR_R_OVERFLOW); return false; } @@ -524,7 +535,7 @@ bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, } uint16_t record_version = dtls_record_version(ssl); - uint64_t aead_seq; + DTLSRecordNumber record_number(epoch, write_epoch->next_seq); if (dtls13_header) { // The first byte of the DTLS 1.3 record header has the following format: // 0 1 2 3 4 5 6 7 @@ -549,25 +560,21 @@ bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, // omit the length. out[3] = ciphertext_len >> 8; out[4] = ciphertext_len & 0xff; - // DTLS 1.3 uses the sequence number without the epoch for the AEAD. - aead_seq = write_epoch->next_seq; } else { out[0] = type; out[1] = record_version >> 8; out[2] = record_version & 0xff; - // DTLS 1.2 uses the sequence number with the epoch for the AEAD. - aead_seq = (uint64_t{epoch} << 48) | write_epoch->next_seq; - CRYPTO_store_u64_be(&out[3], aead_seq); + CRYPTO_store_u64_be(&out[3], record_number.combined()); out[11] = ciphertext_len >> 8; out[12] = ciphertext_len & 0xff; } Span header = MakeConstSpan(out, record_header_len); - if (!write_epoch->aead->SealScatter(out + record_header_len, out + prefix, - out + prefix + in_len, type, - record_version, aead_seq, header, in, - in_len, extra_in, extra_in_len)) { + if (!write_epoch->aead->SealScatter( + out + record_header_len, out + prefix, out + prefix + in_len, type, + record_version, dtls_aead_sequence(ssl, record_number), header, in, + in_len, extra_in, extra_in_len)) { return false; } @@ -587,6 +594,7 @@ bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, out[2] ^= mask[1]; } + *out_number = record_number; write_epoch->next_seq++; *out_len = record_header_len + ciphertext_len; ssl_do_msg_callback(ssl, 1 /* write */, SSL3_RT_HEADER, header); diff --git a/ssl/internal.h b/ssl/internal.h index 0f3bd8229..d9b651976 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -1231,6 +1231,31 @@ OPENSSL_EXPORT uint64_t reconstruct_seqnum(uint16_t wire_seq, uint64_t seq_mask, // Record layer. +// TODO(davidben): Use this type more extensively in the epoch state. +class DTLSRecordNumber { + public: + static constexpr uint64_t kMaxSequence = (uint64_t{1} << 48) - 1; + + DTLSRecordNumber() = default; + DTLSRecordNumber(uint16_t epoch, uint64_t sequence) { + BSSL_CHECK(sequence <= kMaxSequence); + combined_ = (uint64_t{epoch} << 48) | sequence; + } + + static DTLSRecordNumber FromCombined(uint64_t combined) { + return DTLSRecordNumber(combined); + } + + uint64_t combined() const { return combined_; } + uint16_t epoch() const { return combined_ >> 48; } + uint64_t sequence() const { return combined_ & kMaxSequence; } + + private: + explicit DTLSRecordNumber(uint64_t combined) : combined_(combined) {} + + uint64_t combined_ = 0; +}; + class RecordNumberEncrypter { public: static constexpr bool kAllowUniquePtr = true; @@ -1309,8 +1334,10 @@ enum ssl_open_record_t tls_open_record(SSL *ssl, uint8_t *out_type, // dtls_open_record implements |tls_open_record| for DTLS. It only returns // |ssl_open_record_partial| if |in| was empty and sets |*out_consumed| to -// zero. The caller should read one packet and try again. +// zero. The caller should read one packet and try again. On success, +// |*out_number| is set to the record number of the record. enum ssl_open_record_t dtls_open_record(SSL *ssl, uint8_t *out_type, + DTLSRecordNumber *out_number, Span *out, size_t *out_consumed, uint8_t *out_alert, Span in); @@ -1348,10 +1375,11 @@ size_t dtls_seal_prefix_len(const SSL *ssl, uint16_t epoch); // dtls_seal_record implements |tls_seal_record| for DTLS. |epoch| selects which // epoch's cipher state to use. Unlike |tls_seal_record|, |in| and |out| may // alias but, if they do, |in| must be exactly |dtls_seal_prefix_len| bytes -// ahead of |out|. -bool dtls_seal_record(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, - uint8_t type, const uint8_t *in, size_t in_len, - uint16_t epoch); +// ahead of |out|. On success, |*out_number| is set to the record number of the +// record. +bool dtls_seal_record(SSL *ssl, DTLSRecordNumber *out_number, uint8_t *out, + size_t *out_len, size_t max_out, uint8_t type, + const uint8_t *in, size_t in_len, uint16_t epoch); // ssl_process_alert processes |in| as an alert and updates |ssl|'s shutdown // state. It returns one of |ssl_open_record_discard|, |ssl_open_record_error|, diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index ef4b7d5dd..a5023c86d 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -2952,20 +2952,24 @@ int SSL_get_ivs(const SSL *ssl, const uint8_t **out_read_iv, uint64_t SSL_get_read_sequence(const SSL *ssl) { if (SSL_is_dtls(ssl)) { - // TODO(crbug.com/42290608): The API for read sequences in DTLS 1.3 needs to - // reworked. In DTLS 1.3, the read epoch is updated once new keys are - // derived (before we receive a message encrypted with those keys), which - // results in the read epoch being ahead of the highest record received. - // Additionally, when we process a KeyUpdate, we will install new read keys - // for the new epoch, but we may receive messages from the old epoch for - // some time if the ACK gets lost or there is reordering. - - // max_seq_num already includes the epoch. However, the current epoch may - // be one ahead of the highest record received, immediately after a key - // change. + // TODO(crbug.com/42290608): This API needs to reworked. Right at an epoch + // transition, it is possible that |read_epoch| has not received any + // records. We will then return that sequence 0 is the highest received, but + // this is not quite right. + // + // This is mostly moot in DTLS 1.2 because, after the handshake, we will + // never be in this state. In DTLS 1.3, there is a key transition + // immediately after the handshake, and in the steady state with KeyUpdate. + // While not yet implemented, DTLS 1.3 will handle key changes by having two + // epochs live at once (current and optional next), only cycling forward + // when we receive a record at the new epoch. + // + // When we implement this, this sequence 0 edge case will be gone, but + // replaced with a different issue: our record layer APIs have no way to + // report transition state. We'll likely need a new API for DTLS offload. const DTLSReadEpoch *read_epoch = &ssl->d1->read_epoch; - assert(read_epoch->epoch >= read_epoch->bitmap.max_seq_num() >> 48); - return read_epoch->bitmap.max_seq_num(); + return DTLSRecordNumber(read_epoch->epoch, read_epoch->bitmap.max_seq_num()) + .combined(); } return ssl->s3->read_sequence; } @@ -2973,12 +2977,8 @@ uint64_t SSL_get_read_sequence(const SSL *ssl) { uint64_t SSL_get_write_sequence(const SSL *ssl) { if (SSL_is_dtls(ssl)) { const DTLSWriteEpoch *write_epoch = &ssl->d1->write_epoch; - uint64_t ret = write_epoch->next_seq; - if (SSL_is_dtls(ssl)) { - assert((ret >> 48) == 0); - ret |= uint64_t{write_epoch->epoch} << 48; - } - return ret; + return DTLSRecordNumber(write_epoch->epoch, write_epoch->next_seq) + .combined(); } return ssl->s3->write_sequence; diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index eff071242..06f90f430 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -2809,22 +2809,28 @@ TEST_P(SSLVersionTest, SequenceNumber) { if (is_dtls()) { if (version() == DTLS1_3_EXPERIMENTAL_VERSION) { - // Client and server write epochs should be at 3 (application data). + // Both client and server must be at epoch 3 (application data). + EXPECT_EQ(EpochFromSequence(client_read_seq), 3); EXPECT_EQ(EpochFromSequence(client_write_seq), 3); + EXPECT_EQ(EpochFromSequence(server_read_seq), 3); EXPECT_EQ(EpochFromSequence(server_write_seq), 3); - // TODO(crbug.com/42290608): The read sequences aren't checked because the - // SSL_get_read_sequence API needs to be reworked for DTLS 1.3. + + // TODO(crbug.com/42290608): The next record to be written should exceed + // the largest received, but they'll actually be equal because the + // |SSL_get_read_sequence| API cannot represent DTLS key transitions. + EXPECT_GE(client_write_seq, server_read_seq); + EXPECT_GE(server_write_seq, client_read_seq); } else { // Both client and server must be at epoch 1. EXPECT_EQ(EpochFromSequence(client_read_seq), 1); EXPECT_EQ(EpochFromSequence(client_write_seq), 1); EXPECT_EQ(EpochFromSequence(server_read_seq), 1); EXPECT_EQ(EpochFromSequence(server_write_seq), 1); - } - // The next record to be written should exceed the largest received. - EXPECT_GT(client_write_seq, server_read_seq); - EXPECT_GT(server_write_seq, client_read_seq); + // The next record to be written should exceed the largest received. + EXPECT_GT(client_write_seq, server_read_seq); + EXPECT_GT(server_write_seq, client_read_seq); + } } else { // The next record to be written should equal the next to be received. EXPECT_EQ(client_write_seq, server_read_seq);