DTLS 1.3: plaintext ACKs, empty legacy_session_id_echo, legacy_cookie check (RFC 9147 / 9147bis conformance) - #2
Conversation
…atagrams Adds the ACK-driven reliable handshake of RFC 9147 sections 5.8 and 7, and packs handshake flights into as few datagrams as the MTU allows. The packing applies to DTLS 1.2 as well as 1.3, since that is what github bcgit#1487 asks for: on the existing aggregated-handshake test a client flight went from 5 datagrams to 3 for the same 1279 bytes. Only handshake records are packed, and a non-handshake record flushes the buffer before it leaves, so write order is preserved; that matters for the implicit change_cipher_spec, which must not overtake the flight it follows. The reliable handshake registers each written fragment against the record number that carried it, retires fragments when an ACK arrives, retransmits only what is outstanding, and emits ACKs on the RFC 9147 7.1 triggers. Inbound ACKs are filtered by the epoch of the record carrying them, and both ACK emission and the accumulated record-number list are bounded by what fits in one datagram. DTLS 1.3 still cannot be negotiated, so none of the 1.3 paths are reachable yet. DTLSReassembler.contributeFragment changed from void to boolean and gained acceptsFragment and getNextExpectedOffset; the admission predicate is unchanged, and DTLS 1.2 behaviour is unaffected. Two test helpers, MinimalHandshakeAggregator and ServerHandshakeDropper, decided what to do by inspecting only the first record of a datagram, which was exact only while each datagram carried one record. Both now walk every record. relates to github bcgit#1468. closes github bcgit#1487.
…relates to github bcgit#1468.
…relates to github bcgit#1468.
…t, relates to github bcgit#1468.
…overage, relates to github bcgit#1468.
…s retransmission, relates to github bcgit#1468.
…on, relates to github bcgit#1468.
…etention, relates to github bcgit#1468.
…, relates to github bcgit#1468.
…seq, relates to github bcgit#1468. The re-review of the previous fixes found two comments claiming more than the code does and two test assertions weaker than their messages. The notification comment said the call was placed exactly as TlsServerProtocol does it. The version gating is mirrored; the ordering relative to the other TlsServer callbacks is not, and cannot be, because DTLS selects the version in generateServerHello, by which point establishClientSigAlgs and processClientExtensions have already run. Say both, rather than claiming a wholesale mirror. DTLSVerifier produces a DTLSRequest for any ClientHello carrying a cookie it has verified, so it need not have sent the HelloVerifyRequest during that call. The invariant is that the ClientHello arrived through the cookie exchange. The server-refusal test asserted the internal_error the client receives, which is not specific to this cause, so the harness now records what the server itself threw and the test asserts that diagnostic. The harness also stops printing a stack trace for a server abort a test expects, which otherwise makes a passing test look like a failing one. Counting datagrams that carry a ClientHello cannot tell a new message from a retransmission of the first, so the scripted transport records each ClientHello's message_seq and the test requires it to advance to 1, per RFC 6347 4.2.2.
…ke, relates to github bcgit#1468. TlsUtils.initCipher and the TlsAEADCipher constructor both read getSecurityParametersHandshake(), which AbstractTlsContext.handshakeComplete nulls, so neither could build a cipher once a connection was established. A DTLS 1.3 key update (RFC 9147 4.6.3) has to do exactly that. Add TlsCryptoParameters.getSecurityParameters(), mirroring AbstractTlsContext.getSecurityParameters(): the handshake parameters while a handshake is in progress, the connection parameters otherwise. Read the parameters through it at those two sites. TLS over TCP and DTLS 1.2 are unchanged. handshakeComplete moves the single SecurityParameters instance from the handshake field to the connection field; while the handshake field is non-null, getSecurityParameters() returns that same object, so the new accessor is identical to the old one for every caller that runs during a handshake. Every production caller of initCipher is a handshake path in TlsClientProtocol, TlsServerProtocol, DTLSClientProtocol and DTLSServerProtocol, including renegotiation, which reopens handshake parameters via handshakeBeginning first. TLS 1.3 key update over TCP does not come through here at all: it rekeys the existing cipher in place through RecordStream's rekeyDecoder/rekeyEncoder. The change is therefore purely a widening: a call that used to throw NullPointerException now resolves. The full TLS, DTLS and JSSE suites were run against it. TlsImplUtils.calculateKeyBlock, which still reads the handshake parameters, is unreachable post-handshake: the TLS 1.3 branch of the TlsAEADCipher constructor returns before it.
…sors, relates to github bcgit#1468. Review of the preceding commit found two defects in it. TlsCryptoParameters.getSecurityParameters() delegated straight to the context, bypassing this class's own public, non-final accessors. This repository already contains subclasses constructed with a null context that override only the accessors they need, which is what a third party stubbing a cipher would write; such a subclass was silently bypassed, and one with a null context threw where it previously worked. Tls13NullCipher still read the handshake security parameters, so the same post-handshake limitation remained one class over. The RFC 9150 integrity-only suites reach it through create13NullCipher, are accepted by getPRFAlgorithm for any 1.3 version with no DTLS exclusion, and are exposed by the JSSE provider, so a DTLS 1.3 key update would have thrown whenever one was negotiated.
…thub bcgit#1468. processDTLS13Record probed exactly two candidates, the current read epoch and the epoch retained for retransmission. That is right while those are the only epochs the record layer holds, but it has no notion of recency, so it cannot express RFC 9147 4.2.2's rule that a record whose low two epoch bits match more than one held epoch belongs to the most recent of them - and it has nowhere to put the pre-update read epoch that RFC 9147 8 requires be retained across a key update. Replace the probe with one ordered collection of live read epochs, iterated most recent first, so that 4.2.2's rule is the iteration order rather than a special case. getEpochForRetransmit resolves through the same collection, so the send and receive sides cannot disagree about which epochs are held. The collection is bounded: at most one read epoch is retained across a key update, and retainReadEpoch asserts it. Section 8 forbids sending at a new epoch before the peer's KeyUpdate is acknowledged, so a second update cannot begin while the first is retained; a set that grew without that bound would be a memory-growth denial of service driven by peer-controlled key updates. No behaviour changes here. The retained slot is always empty until key update lands, and the plaintext epoch that now appears in the collection was already rejected a few lines later by the DTLSCiphertext cipher check.
…github bcgit#1468. RFC 9147 8 forbids sending under a new epoch until the peer has acknowledged the KeyUpdate, while updating the local traffic secret destroys the secret the current write epoch was keyed from, so the new epoch must be built at the moment the KeyUpdate is generated and then held unused. Neither existing mechanism can hold it. enablePendingEpochWrite installs the pending epoch the instant it is asked to, and commitPendingEpochIfCurrent retires the old epoch only once both directions have reached the pending one; a key update advances exactly one direction, so it satisfies neither. A shared pending epoch would also force one epoch number on both directions, which is wrong once each direction advances on its own key updates - so the number here is derived from the write epoch alone rather than reusing initPendingEpoch's. Add pendingWriteEpoch with derivePendingWriteEpoch to build and hold it and installPendingWriteEpoch to make it the write epoch. The held epoch is deliberately not resolvable by getEpochForRetransmit: nothing has been sent under it, so there is nothing to retransmit under it. A second derivation while one is held is refused, matching 5.8.4's ban on a second KeyUpdate while one is outstanding. No behaviour changes here. Nothing calls either method yet, so the handshake's epoch progression is untouched; key update itself follows.
… github bcgit#1468. DTLSReliableHandshake.finish() unregistered the record layer's ACK listener outright, with a comment that post-handshake ACKs belong to the post-handshake state machines. Nothing owned them, so an ACK or a handshake message arriving after the handshake was decoded and dropped. DTLS13PostHandshake is that owner. It receives every handshake record that arrives at epoch 3 or above (RFC 9147 6.1), reassembles it with DTLSReassembler - a NewSessionTicket routinely exceeds an MTU, so refusing fragments would be an interoperability failure our own peers could never show - dispatches by message type, and acknowledges the record. NewSessionTicket is received, acknowledged and discarded: resumption and pre-shared keys are not supported here. This implements the receiver's acknowledgement obligation, not the sender's no-send-before-acknowledgement rule; the sending state machines of RFC 9147 5.8.4 arrive with key update. Two rules are easy to get backwards and are deliberate here: - RFC 9147 7 requires a post-handshake record to be acknowledged even when the message it carries is discarded as a duplicate, which is the opposite of the handshake-time rule. During the handshake the flight's own quarter timer will ACK anyway; after it the peer is waiting on exactly this ACK and would otherwise retransmit for as long as its state machine runs. The handshake-time test is untouched. - The RFC 9147 5.8.1 final-flight hook expires with the retransmit timeout at twice the MSL. This owner does not: a KeyUpdate may arrive at any point in a connection's life, and an owner that expired with that timeout would work in every test that finishes in milliseconds and die on exactly the long-lived connections it exists for. Both handlers are offered the record and partition it by epoch. The message_seq counters are carried across the handshake boundary rather than restarted, and a KeyUpdate arriving before the handshake completes is now fatal with unexpected_message (RFC 8446 4.6.3) instead of being dropped quietly.
…old one on first decrypt, relates to github bcgit#1468. DTLS13PostHandshake validated, counted and acknowledged a received KeyUpdate but did nothing with it, so an acknowledged key update left the peer writing under keys the connection could not read. This closes that. This implements the RECEIVER half of RFC 9147 8, whose trigger is the first successful DECRYPTION at the new epoch - not the sender's half, whose trigger is an acknowledgement of its own KeyUpdate. An ACK is not a decryption, and nothing here reads one. updatePeerReadEpoch is the whole of a received key update's effect on the read side, and is one operation on purpose. update13TrafficSecretPeer destroys the secret it replaces, so a cipher not built from the updated secret at that moment can never be built from the superseded one afterwards, and the epoch retained a line later would be keyed from material that no longer exists. Everything that can refuse the update is therefore checked before the secret is touched, and the construction is not deferred. Note that building a cipher after the handshake no longer throws now that the security parameters resolve to the connection ones, so that call cannot be relied on to object if it is reached in the wrong order; only the order protects this. The pre-update epoch is released at the point RFC 9147 8 names - a record has just decrypted at the current read epoch - and before the record is dispatched. That ordering is load-bearing: our acknowledgement is what frees the peer to send at its new epoch and it need not send anything else first, so a KeyUpdate may legitimately be the first thing to arrive there, and a later release would make that look like a second update while one was still retained. One that really is that is refused with unexpected_message rather than being allowed to drop keys records are still arriving under or to grow the retained set at the peer's discretion. An update_requested is recorded as a pending obligation rather than answered from the receive path, which is what TlsProtocol.receive13KeyUpdate does too. Over DTLS there is a second reason: RFC 9147 5.8.4 makes a KeyUpdate a single-flight message with its own retransmit timer and section 8 forbids sending under the new epoch until it is acknowledged, so the answer belongs to the sending state machine - which is also the only side that can apply section 8's override of RFC 8446 at the epoch limit. Three things inherited from the epoch commits first become live here: - installPendingWriteEpoch now advances currentEpoch with the write epoch. Until a key update both directions share one DTLSEpoch, so the superseded number stays resolvable through the other direction and nothing is stale; once both have moved and the retained read epoch is released, nothing holds that epoch but a currentEpoch left behind, and getEpochForRetransmit falls back to it - a released epoch resolving to stale keys and a used sequence number. - The superseded write epoch is retained by nothing and released there. 5.8.4's sending state machine has had its ACK by then, and section 7 requires a post-handshake ACK at the highest available sending epoch, so nothing is ever sent under it again. Only the read side needs retaining. - nextEpoch is the epoch-number overflow guard, used by both directions' key update. Section 8's own cap is 2^48-1 for a sender and unenforceable for a receiver; neither binds before the int a DTLSEpoch holds its epoch in, and a silent wrap would give a negative epoch or one aliasing an epoch already held. commitPendingEpochIfCurrent is left alone: its both-directions test is an invariant a key update never reaches, not one it falsifies, and that is now documented rather than assumed. The paired-record-layer harness set prfCryptoHashAlgorithm without prfHashLength, which production sets in the same place. It could build a cipher but not update a traffic secret, which no test before this one did.
…elates to github bcgit#1468. This is the SENDING half of RFC 9147 section 8. Its trigger is the acknowledgement: "implementations MUST NOT send records with the new keys or send a new KeyUpdate until the previous KeyUpdate has been acknowledged". The receiving half, whose trigger is the first successful decryption at the new epoch, landed separately and is not touched here. An ACK is not a decryption. The state machine is the one RFC 9147 5.8.4 describes for a single-flight post-handshake message: derive the next write epoch and hold it, send the KeyUpdate at the old epoch, wait for an ACK, retransmit until it arrives, and install the epoch only then. It is driven from the receive path, not the send path, because a peer that has just sent a KeyUpdate may have nothing further to send - and section 8 has left it unable to use the epoch it wants. A send-driven timer would stall exactly that peer. A key update starts by itself once the write epoch's sequence number reaches the threshold RecordStream.needsKeyUpdate applies over TLS, and a second one is refused while one is outstanding (5.8.4). Two acknowledgement defects that stalled this are fixed with it, both from applying a handshake-time rule after the handshake. RFC 9147 7 says "During the handshake, ACK records MUST be sent with an epoch which is equal to or higher than the record which is being acknowledged", but "After the handshake, implementations MUST use the highest available sending epoch" - with no floor. The two directions' epochs advance on their own key updates, so a peer that has updated its sending keys while we have not both sends its KeyUpdate above our epoch and acknowledges ours from below. sendAck refused the first and the ACK record-number filter discarded the second, each leaving a KeyUpdate unacknowledged and its sender retransmitting for as long as its state machine runs. Both rules now apply during the handshake only; the filter additionally still applies to an unprotected ACK at epoch 0 after it, which is the threat it was written for. An epoch built from the peer's updated traffic secret is now marked as such and is no longer resolvable for writing. A DTLSEpoch carries one cipher and one sequence number counter for both directions, so writing at the peer's epoch would encrypt under the peer's key at sequence numbers the peer has already used. Nothing reaches it today, but the two directions' epoch numbers coincide as a matter of course, so it would be reached by number collision rather than by anything obviously wrong.
…elates to github bcgit#1468. The whole-branch review returned zero Critical findings; these are its six Important and four Minor ones, applied in one pass. No protocol behaviour changes. The substantive one is that the peerKeyed guard was correct but its reasoning was not, and the reasoning is what a maintainer would use to decide whether the guard is still needed. TlsUtils.initCipher builds a cipher for BOTH directions: TlsAEADCipher's TLS 1.3 constructor calls rekeyCipher once for the decrypt side, keyed from the peer's traffic secret, and once for the encrypt side, keyed from the local one. updatePeerReadEpoch updates only the peer's secret, so the epoch it builds carries an encrypt side keyed identically to the current write epoch's, paired with a sequence number counter starting at zero. Writing at such an epoch therefore does not produce records the peer cannot read, as the comments claimed: it produces records under the same AEAD key at nonces the current write epoch has already used - nonce reuse under a live key, silent, and for GCM enough to recover the authentication key. Both comments now say that, and record the structural cause: updatePeerReadEpoch builds a full bidirectional cipher when it needs only the read direction, so it leaves a correctly-keyed encryptor in an object that must never encrypt. Also: - Disclose the one deliberate specification deviation. RFC 8446 4.6.3 has a peer that receives 'update_requested' send its own KeyUpdate before its next Application Data record; RFC 9147 5.8.4 forbids sending one while an earlier one is unacknowledged. 9147 is followed and the obligation is deferred past application data. checkKeyUpdateBeforeSend now names both rules, which wins and why, handleMessage's 8446 quote points at it, and a new test exercises the deferral and its discharge from the wire. - Take getLiveReadEpochs off the per-record receive path. Both directions now resolve by walking one private ordering of the four epoch slots in place, instead of building a Vector and scanning it for every received unified-header record; getLiveReadEpochs is retained as the test-facing view over the same ordering. - Synchronize DTLS13PostHandshake. DTLSRecordLayer supports concurrent send and receive and reaches this object from both, so its mutable state needed guarding (not a key-confusion case: that was checked and is unreachable). No call site holds the record layer's write lock, so the added monitor cannot invert against it. - Make sendKeyUpdate's "armed last" comment true by construction: the flight tracker is now reset and registered before the latch that speaks for it, rather than relying on isComplete() being false on an empty tracker. - Minors: remove the unused getNextSendSeq; say that releaseRetainedReadEpoch is not a zeroisation guarantee and often not even the last reference; qualify getLiveReadEpochs's javadoc to the unified-header path; rename getRecordLayerForTest to getRecordLayer with the package-private contract stated; trim the end-to-end key update pump deadlines.
…r, relates to github bcgit#1468. The scoped re-review of the previous commit found that its corrected comment was still false in one reachable state, and that state is the one the new test creates. deriveNextWriteEpoch advances the local traffic secret when a KeyUpdate is sent, while installPendingWriteEpoch swaps the epoch in only when the acknowledgement arrives. So a peer read epoch derived while our own KeyUpdate is outstanding carries an encrypt side keyed like the PENDING write epoch, not the current one. The conclusion is unchanged, since the key is live either way and both sequence counters start at zero, but the sentence a maintainer would reason from was wrong, and checking it against the outstanding-KeyUpdate case would have suggested the guard was over-cautious. The lock order between DTLS13PostHandshake's monitor and the record layer's write lock was held by construction and documented nowhere. It is now stated at both ends: the monitor is taken first, and a call into that class from inside a write-lock block would invert the order. Two test comments named mutations that no longer do what they claim. The epoch ordering moved out of the test-facing view into getLiveReadEpoch, so reversing the view proves nothing; and relaxing only the first of the two guards on a second KeyUpdate fails on the exception raised by the second, not on the datagram count. The acknowledgement probe's settle period had been trimmed along with the key update deadlines it does not share. Its claim is a count of records that did not arrive, so the margin is restored under its own name.
…ctions 4.1 and 7), which the DTLSPlaintext dispatch was discarding unread so a peer's epoch-0 acknowledgement of a fragmented flight was ignored and the flight retransmitted on the timer, relates to github bcgit#1468.
…HelloRetryRequest, and as a client require it to be empty (RFC 9147 section 5, draft-ietf-tls-rfc9147bis), instead of echoing and requiring the client's legacy_session_id; also refuse a non-empty legacy_cookie in a DTLS 1.3 ClientHello (RFC 9147 section 5.3), relates to github bcgit#1468.
…ached DTLS 1.2 session, and for a DTLS 1.3 ClientHello carrying a legacy_cookie, relates to github bcgit#1468.
|
[AI] Review of this PR, produced with generative-AI assistance and checked against RFC 9147 and draft-ietf-tls-rfc9147bis-02. All three changes are correct, and on this branch 1. The 2. Three new members were inserted between an existing javadoc and its method, so those javadocs now sit on top of the new member's own javadoc and the original members lost theirs:
Minor: the three imports added to On the other points:
These fixes belong in part 3 rather than part 4, so once this is updated I plan to cherry-pick the commits there, resolving the conflict in the |
b022ef2 to
9d43a57
Compare
|
[AI] Heads-up: the You don't need to rebase. Push the |
Three conformance fixes from reading the series against RFC 9147 and draft-ietf-tls-rfc9147bis (the RFC 9147 revision in progress at tlswg/dtls13-spec), each with a test that fails on the current branch. None of them affected the Chrome or Firefox handshakes in #1; two of them would bite other peers or other handshake shapes.
1. Plaintext (epoch 0) ACK records were dropped unread
RFC 9147 4.1 lists ack(26) among the DTLSPlaintext content types, and section 7 has a peer with no protected epoch to send in yet acknowledge a fragmented ClientHello or ServerHello at epoch 0. The bis draft (tlswg/dtls13-spec#327) makes this explicit: after DTLS 1.3 is known, an endpoint "MAY send and process ACK records in epoch 0".
DTLSRecordLayer.processRecordonly recognisedackinside a DTLSCiphertext record. The DTLSPlaintext dispatch (alert, application_data, change_cipher_spec, handshake, heartbeat, tls12_cid) returned -1 for it, so every epoch-0 ACK was discarded before decode and the sender's whole flight was retransmitted on the timer instead. The epoch-0 branch offilterAckRecordNumbersand the "epoch-0 defence" it documents were unreachable. BC itself sends epoch-0 ACKs (sendAckat the current write epoch), so two BC peers were dropping each other's.Fix: accept
ackin the plaintext dispatch oncedtls13is set (before that it stays an unknown content type, which is what the bis draft asks for: "Until that point, ACK records received in epoch 0 MUST be ignored"), and let the client's retained plaintext epoch accept it alongside a retransmitted handshake record. The existing filter then applies unchanged.Tests:
DTLSAckTransportTest.testPlaintextAckAtEpochZeroIsDelivered(a DTLS 1.3 record layer still reading at epoch 0 receives a plaintext ACK; the epoch-0 record number is delivered, the one above the ACK's epoch is filtered) andtestPlaintextAckBeforeDTLS13IsSelectedIsIgnored.2.
legacy_session_id_echowas echoed by the server and required by the clientRFC 9147 5: "DTLS servers MUST NOT echo the legacy_session_id value from the client and MUST send an empty legacy_session_id_echo". The bis draft (tlswg/dtls13-spec#298) adds the client side: "DTLS 1.3 clients MUST abort the handshake with an illegal_parameter alert if the field is not empty. This applies even if the legacy_session_id field of the ClientHello is non-empty due to a cached session set by a pre-DTLS 1.3 server."
generate13ServerHelloand the HelloRetryRequest copied the ClientHello's session ID into the echo, andprocess13ServerHello/process13HelloRetryRequestrequired the echo to equal what the client sent. Two BC peers agree with each other and hide it. Against a conforming peer: a BC client offering a session cached from a DTLS 1.2 server (whichDTLSClientProtocoldoes whenever DTLS 1.2 is still among its versions) aborts on the conforming server's empty echo, and a BC server makes a conforming client abort by echoing.Fix: server sends an empty echo in both messages; client requires an empty echo in both, with a message naming the check.
Tests:
DTLS13ProtocolTest.testServerSendsEmptyLegacySessionIdEcho(the harness client is given a cached DTLS 1.2 session and offers 1.3 and 1.2; the captured ClientHello is checked to really carry the 32-byte ID, and the ServerHello's echo is empty) andDTLS13ClientProtocolTest.testDTLSv13ServerHelloWithNonEmptyLegacySessionIdEchoRejected(scripted ServerHello with a 4-byte echo; the alert message pins the check, since the scripted message lacks a key_share too).3. A non-empty
legacy_cookiein a DTLS 1.3 ClientHello was not rejectedRFC 9147 5.3: "If a DTLS 1.3 ClientHello is received with any other value in this field, the server MUST abort the handshake with an illegal_parameter alert." The server never looked at the cookie on the 1.3 path, and a comment said it "MUST be ignored", which is not what the text says. No conforming peer sends one, so this is a strictness gap only.
Fix: check it when DTLS 1.3 is selected, after the existing HelloVerifyRequest refusal, so a ClientHello that legitimately carries a HelloVerifyRequest cookie still gets the diagnostic naming the front end.
Test:
DTLS13ProtocolTest.testClientHelloWithLegacyCookieRejectedinserts a 4-byte cookie into the first ClientHello on the path (adjusting the record, message and fragment lengths) and asserts the server's alert and message.Verification
tlsmodule: 897 tests, no failures;checkstyleMainclean. With the main-source changes stashed, the four positive tests fail on the original symptoms (plaintext ACK not delivered; echo present; wrong alert message; cookie accepted), and pass with them restored.Also noted, not changed here
DTLSAckTransportTest.testAckRecordNumbersAboveTheAckEpochSurviveAfterTheHandshakecovers the KeyUpdate case. I am raising the wording at tlswg/dtls13-spec, proposing that after the handshake the check become "references an epoch the receiver has never sent in"; if that is adopted,processDecodedRecord's post-handshake ACK path would want that check (it currently accepts any epoch). No change proposed here until the text settles.unexpected_message), and refusing to letmessage_seqwrap (Wrapping of message_seq tlswg/dtls13-spec#304;writeUint16truncates silently after 65535 KeyUpdates). Both only matter with a misbehaving peer or after an unrealistic number of updates, and both are small; happy to add them if wanted.Disclosure
This work was produced with generative-AI assistance, per the contributing guidelines. The RFC and draft text, the code paths and the tests were reviewed.
Relates to bcgit#1468, bcgit#2441, bcgit#2442.