Fix DTLS handshake retransmission edge cases#5696
Conversation
|
@Thiesius thanks for digging into this and resolving these longstanding issues. This PR is big so it may take a while to review completely but I'll try a first pass asap. |
Do not treat zero-length fragments of non-empty already-consumed DTLS handshake messages as evidence that the peer retransmitted an old flight. They are legal DTLS noise and should not trigger last-flight replay. Add a regression that injects an empty old Finished fragment after a completed DTLS handshake and verifies no final-flight retransmission is emitted.
No rush at all. BoGo tests seems to have caught one more regression that I have introduced in this PR. I have added a fix + unit test around it so that it's closer to the source. I didn't test it in the WSL, but if the CI/CD runners are costly, then I can maybe do that next time before sending a commit. I also feel like the |
|
I have enabled the build in WSL so that I could investigate the failures around BoGo. I think I have them all covered, but I have been checking the RFCs and seems like there is one more thing we don't have covered. I'm going to look into that in following day(s). @randombit Should I mark the PR as a draft until the pipeline is green? |
Distinguish active-association retransmissions from pending handshake input, reassemble terminal retransmitted messages before replaying flights, and preserve queued-message validation across activation. Cover reordered final flights, duplicate fragments, and DTLS renegotiation, including the corresponding BoGo error mapping.
|
No particular need, locally draft is more for signaling "don't even bother looking at this yet it's still going to probably change dramatically before being all the way done" whereas this sounds like it's just (hopefully) a few smaller changes. |
Alrighty. As for the follow-up: We should now be able to distinguish retransmissions from new handshake messages, and wait for complete terminal-flight "evidence" including CCS/Finished + correctly recognize Slightly stepping out of my comfortable domain here, since my stress testing was mainly focused on "vanilla" client-server handshakes and their reliability on unstable networks (5G, Radio, WiFi) 😅 Anyway I have also removed the obsolete I know that I originally mentioned that I would leave the The stress-test maybe discovered one more thingy that is not covered by Bogo or our unit tests. I will investigate it tomorrow. |
A delayed retransmission of the cookie-bearing ClientHello still uses epoch 0, but carries message_seq 1. Previously an active server treated every epoch-0 ClientHello as a new association and created pending handshake state, preventing recovery when the client later retransmitted its final flight. Only treat sequence-zero ClientHellos as new associations and route stale ClientHellos through retained-flight retransmission handling. Add a regression for the loss and reordering sequence found by the UDP fault-proxy stress test.
|
So, while stress-testing through the UDP fault proxy, one deterministic reorder seed still stalled after the server had activated. Packet tracing showed a delayed cookie-bearing |
reneme
left a comment
reneme
left a comment
There was a problem hiding this comment.
Some comments below. Generally this looks like a nice improvement. I still have to dig into the handshake I/O extensions.
| bool Channel_Impl_12::timeout_check() { | ||
| if(m_pending_state) { | ||
| return m_pending_state->handshake_io().timeout_check(); | ||
| } | ||
|
|
||
| if(m_is_datagram && !m_has_been_closed && m_active_state.has_value() && !m_dtls_peer_traffic_seen) { | ||
| if(auto* dtls_io = m_active_state->dtls_handshake_io()) { | ||
| return dtls_io->timeout_check(); | ||
| } | ||
| } | ||
|
|
||
| //FIXME: scan cipher suites and remove epochs older than 2*MSL | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I'm worried that this (fairly complex) logic is duplicated in requires_timeout_check(). We should just re-use it here by somewhat inverting the flow of the method. Like so:
bool Channel_Impl_12::timeout_check() {
if(!requires_timeout_check()) {
//FIXME: scan cipher suites and remove epochs older than 2*MSL
return false;
}
if(m_pending_state) {
return m_pending_state->handshake_io().timeout_check();
}
if(auto* dtls_io = m_active_state->dtls_handshake_io()) {
return dtls_io->timeout_check();
}
}... if the generic check sees no reason for the timeout check we just leave it be. Otherwise, we delegate to the pending state. Or, if no more pending state is there, an opportunistic delegation to the active state should be tried.
There was a problem hiding this comment.
Sure. I addressed this slightly differently alongside the timeout-hint API change.
Both timeout_check() and next_retransmission_timeout() now use a shared retransmission_io() helper, which selects the pending handshake first and otherwise the eligible active DTLS handshake. Now the state-selection logic should be deduplicated.
Its const/non-const overload pair uses a localized const_cast to preserve pointer mutability without duplicating state selection (therefor no UB here). Deducing this would make this cleaner, but I belive Botan currently targets C++20 and not C++23.
reneme
left a comment
reneme
left a comment
There was a problem hiding this comment.
From what I can tell, the changes look sound. Also a Claude Sonnet review didn't bring up anything substantial at this point.
I'm setting to "Request Changes" regarding my mentioned points, but generally this looks solid to me. 👍
| bool Channel_Impl_12::timeout_check() { | ||
| if(m_pending_state) { | ||
| return m_pending_state->handshake_io().timeout_check(); | ||
| } | ||
|
|
||
| if(m_is_datagram && !m_has_been_closed && m_active_state.has_value() && !m_dtls_peer_traffic_seen) { | ||
| if(auto* dtls_io = m_active_state->dtls_handshake_io()) { | ||
| return dtls_io->timeout_check(); | ||
| } | ||
| } | ||
|
|
||
| //FIXME: scan cipher suites and remove epochs older than 2*MSL | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Sure. I addressed this slightly differently alongside the timeout-hint API change.
Both timeout_check() and next_retransmission_timeout() now use a shared retransmission_io() helper, which selects the pending handshake first and otherwise the eligible active DTLS handshake. Now the state-selection logic should be deduplicated.
Its const/non-const overload pair uses a localized const_cast to preserve pointer mutability without duplicating state selection (therefor no UB here). Deducing this would make this cleaner, but I belive Botan currently targets C++20 and not C++23.
reneme
left a comment
reneme
left a comment
There was a problem hiding this comment.
Apart from the commented-on nits below:
I think there's one situation that we don't handle gracefully, yet: An epoch0_restart (ClientHello with epoch=0 and m_active_state.has_value()) while currently performing another handshake already. In other words: if the client sends a new epoch0 ClientHello before a previous handshake has been finished successfully.
Such a new ClientHello message will be passed into process_handshake_ccs() where it'll skip the first conditional because m_pending_state already exists. Therefore it'll just be forwarded into the lower handshake logic. So far I didn't check in detail what'll happen there, but very likely it won't be "the right thing".
To the best of my understanding RFC 6347 also doesn't really specify what should happen in such a case. Additionally, I think, a potential avenue for DoS has to be considered for this situation: i.e. a malicious client forcing the server to perpetually perform expensive handshake crypto by simply sending new ClientHellos all the time.
Perhaps that's something we should just gracefully catch for now and (probably in process_handshake_ccs()) address it in a folllow up.
|
I'm not very familiar with GitHub's review UI, and the inline threads became hidden after the latest commit, so I'll answer them together here to not bomb the mails with separate comments. What happens when another epoch-zero ClientHello arrives while a restart is already pending? Once the first I added a regression test which verifies that HelloVerifyRequest is replayed, cookie processing is not repeated, the existing association remains active, and the pending replacement association can still complete. Can Yes fully agree. Just AI pulling my leg. Not sure how I missed it. Good catch. Can the peer-progress state have a more specific name? Yes. It is now named around the exact thing being tracked: protected application data received from the peer. Should I left this unchanged pending @randombit's preference. But by a quick swoop through the repo we can see, that Botan already exposes chrono types in public TLS headers such as Is Done. It is now named Should the Yes. The comment now explains that this explicitly completes the final outgoing flight so it remains available for timeout retransmission. Should channel mode come from 100% . The code should be the authoritative source. The redundant protocol check was also removed where Can the redundant early returns be removed? Yes. The two unnecessary returns were removed. Control now falls through to the existing pending-state check. I left the larger function extraction for a possible follow-up to avoid complicating this PR further. Can the Yes. Only the DTLS retransmission logic currently consumes it. |
reneme
left a comment
reneme
left a comment
There was a problem hiding this comment.
Looks good to me. Tests are comprehensive albeit a little chatty. I don't think that should stand in the way of integrating this patch, though.
@randombit I think you should do another pass, perhaps focussed on the handshake I/O changes.
randombit
left a comment
randombit
left a comment
There was a problem hiding this comment.
In general looks good and makes sense. Three specific issues where I think the behavior is distinctly wrong, though admittedly I haven't tried writing a testcase to confirm the behavior.
| send_message(m_out_message_seq, epoch, msg_type, msg_bits); | ||
| // This message is not included in the handshake hashes, but it is a | ||
| // DTLS flight and must be available for retransmission if it is lost. | ||
| m_flights.rbegin()->push_back(m_out_message_seq); |
There was a problem hiding this comment.
I think this is not correct. HelloVerifyRequest doesn't need to be replayed directly but instead recreated as a byproduct of the peer sending us a new ClientHello. RFC 6347 section 3.2.1 also explicitly excludes it "Note that timeout and retransmission do not apply to the HelloVerifyRequest, ..."
| // Until protected application data confirms that the peer processed our | ||
| // final flight, keep it eligible for timeout-driven retransmission. | ||
| if(m_active_state.has_value() && !m_active_state->peer_sent_protected_application_data()) { | ||
| return m_active_state->dtls_handshake_io(); |
There was a problem hiding this comment.
This logic seems overly loose. A post-handshake retransmit is only required if we are the protocol side that would send the last flight. Also it seems it would, for a protocol that is completely unidirectional, end up sending periodic handshake retransmissions for the lifetime of the connection.
|
|
||
| uint16_t epoch = m_flight_data[flight[0]].epoch; | ||
| const uint16_t first_epoch = m_flight_data[flight[0]].epoch; | ||
| uint16_t epoch = (first_epoch == 0) ? 0 : first_epoch - 1; |
There was a problem hiding this comment.
IIUC this logic change will cause extra CCSs to be inserted in inappropriate circumstances. Consider renegotiation. The renegotiation ClientHello is sent under epoch 1, since it's already encrypted. This logic will then see that we're sending a message in epoch 1, decide an epoch gap has ensued, and insert a epoch 0 CCS. Depending on the peer this may cause the connection to fail immediately.
|
Thanks for the review, @randombit. I fixed the comments in two follow-up commits and then did one more pass over the resulting HelloVerifyRequest The regression checks both the missing timer and the recovery path. Good catch. Post-handshake retransmission Both sides keep the handshake IO for CCS inference and renegotiation CCS is now recorded when actually emitted, including its position and epoch. Retransmission replays only what was really there. The renegotiation tests cover both initiating sides, while the final-flight tests still verify real CCS replay. DTLS record replay windows There is now one replay window per epoch, using only that epoch’s 48-bit record sequence. Write-side sequencing is unchanged. I also "tightened": epoch-zero restart detection so only the configured server path treats it as a restart. An active client can process old epoch-zero retransmissions normally. I kept the changes in two commits to make them easier to review. The first addresses the three comments. The second is the additional FINISHED and replay-window pass. I ran the usual tests + stress-tests again. |
Nice catch there! |
randombit
left a comment
randombit
left a comment
There was a problem hiding this comment.
Another pass, a few more comments - mostly fallout from earlier review comments
| // handshake flight, so timeout-driven replay is no longer necessary. | ||
| bool peer_sent_protected_application_data() const { return m_peer_sent_protected_application_data; } | ||
|
|
||
| void mark_peer_as_having_sent_protected_application_data() { m_peer_sent_protected_application_data = true; } |
There was a problem hiding this comment.
IIUC, now that final-flight replay is reactive (to a replay from the peer) this flag is no longer needed anymore
| retransmit_last_flight(); | ||
|
|
||
| // Retransmission restarts the backoff window just like a normal write. | ||
| m_last_write = steady_clock_ms(); |
There was a problem hiding this comment.
The other calls to retransmit_last_flight in this file (which are reactive based on peer traffic) do not set m_last_write so IIUC we can end up in a scenario where we replay a final flight based on a peer retransmit, and then have a timer expire and replay again. Not the end of the world of course but seems easily avoidable by updating the clock [possibly the clock write should be moved into retransmit_last_flight?]
|
|
||
| // Once protected application data confirms peer progress, the completed | ||
| // handshake IO is no longer relevant to timeout scheduling. | ||
| if(m_active_state.has_value() && !m_active_state->peer_sent_protected_application_data()) { |
There was a problem hiding this comment.
This branch, as I understand it, is basically dead. In Datagram_Handshake_IO both next_retransmission_timeout and timeout_check return nullopt if m_finished is true. But if we have an active state, then m_finished must be true (right?). So the functions which actually use this return value in Channel_Impl_12 will just return nullopt anyway. Might as well skip it and just return nullptr if we do not have a pending state.
|
|
||
| bool Channel_Impl_12::timeout_check() { | ||
| Handshake_IO* Channel_Impl_12::retransmission_io() { | ||
| return const_cast<Handshake_IO*>(std::as_const(*this).retransmission_io()); |
There was a problem hiding this comment.
Not a big fan of const_cast. And especially with the simplification of not having to look at the active state anymore (see other comment on the main retransmission_io body) , it seems more straightforward to just duplicate the logic of retransmission_io into timeout_check and next_retransmission_timeout directly.
| "KeyChangeWithBufferedMessages-DTLS": "No DTLS 1.3", | ||
| "Renegotiate-DTLS-Server-Forbidden": "Botan tolerates DTLS renegotiation", | ||
| "Renegotiate-DTLS-Client-Forbidden": "Botan tolerates DTLS renegotiation", | ||
| "DTLS12-SendExtraFinished-*": "Botan tolerates extra Finished messages in DTLS", |
There was a problem hiding this comment.
This should also be fixed in config_no_tls13.json
|
Application-data progress flag Yep. Removed the flag and the application-data update. Final-flight recovery is reactive now, so the flag no longer serves a purpose. Reactive replay not updating Good catch. I moved the clock update into Dead active-state timeout branch Yep, that branch was dead once the handshake IO had entered
Agreed. With the active-state case gone, the helper was more code than the duplicated check.
Fixed there as well.
I also added a separate "cleanup" commit ( |
4 participants
Hello,
This PR fixes several DTLS 1.2 handshake retransmission edge cases around lost, duplicated, delayed/partial, and retransmitted handshake flights. Some of these issues were already tracked.
Fixes #2310 (DTLS last write update)
Fixes #2498 (The DTLS server 1.2 is not handling properly the retransmissions of flight 5 from the client)
Fixes #4022 (Facing retransmission issue while adding delay in the server side in the DTLS implementation)
Fixes #4036 (The DTLS server 1.2 is not handling properly the retransmissions in case of delay in flight at server side)
Related to #4782 (DTLS session resumption; frequent "Can't interleave application and handshake data" errors)
Some fixes were ported from our company repository, and I also stress-tested the implementation to discover and validate related edge cases.
I will not pretend that the fixes and especially the unit tests were not AI-assisted. However, I have thoroughly self-reviewed the changes and, more importantly, stress-tested them against lossy/reordered/delayed DTLS traffic.
So what is the context of these fixes
I think this might come up as obvious, but let's recap anyway. DTLS runs over unreliable datagrams. A peer may retransmit an earlier handshake flight after the local endpoint has already progressed, or even after it has locally activated the session. In those cases, we must be able to reproduce the correct previous response flight instead of treating the retransmission as a fresh or invalid handshake event.
Without these fixes, handshakes can stall or fail in cases such as:
ClientHellobecauseHelloVerifyRequestwas lostActual Fix List
HelloVerifyRequestas retransmittable DTLS flight data while still excluding it from handshake transcript hashes.Finished.Unit Test Coverage
I grouped the new coverage under DTLS core regression tests. The tests use Botan’s real DTLS client/server machinery, but simulate UDP behavior by moving emitted datagram bytes between peers manually. The test cpp has grown quite big, maybe worth being split from TLS tests in follow up PR.
Covered cases:
timeout_check()retransmission pacingHelloVerifyRequestHelloVerifyRequestServerHelloDoneLocal Stress Testing
The stress tools are not included in this PR. I used them only for local validation.
Originally I used
clumsy, a Windows traffic-control tool, but those tests were heavily dependent on randomness. I then used a simple UDP fault proxy and standalone DTLS stress app to make the scenarios easier to repeat and vary.Stress-tested scenarios included:
HelloVerifyRequestDropped DTLS application data was not treated as a bug. DTLS preserves datagram boundaries and protects records, but it does not make UDP application data reliable.