Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5727 +/- ##
==========================================
- Coverage 86.21% 84.56% -1.66%
==========================================
Files 60 60
Lines 18712 18729 +17
==========================================
- Hits 16133 15838 -295
- Misses 2579 2891 +312 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…gaurasingh/1rtt-fuzz
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
Please expend the PR description to explain more specifically what you added / what is not implemented for now.
guhetier
left a comment
guhetier
left a comment
There was a problem hiding this comment.
Some nitpicks, but the logic seems largely good to me.
| 0, | ||
| 1 | ||
| }; | ||
| PacketParams.PacketType = QUIC_INITIAL_V1; |
There was a problem hiding this comment.
nit: I would avoid mixing an init list and direct member initialization, it makes it hard to know what is initialized.
| ClientContext.CreateContext(PacketParams.SourceCid); | ||
| CXPLAT_FRE_ASSERT(ClientContext.ProcessData() & CXPLAT_TLS_RESULT_DATA); | ||
|
|
||
| if (CompleteHandshake(Binding, Route, StartTimeMs, &PacketParams, &ClientContext)) { |
There was a problem hiding this comment.
nit: Consider returning early instead of nesting if blocks.
It will make the flow easier to understand: if we didn't get to complete the handshake, we do
nothing. If we don't get 1 RTT write keys, we do nothing. Then, if we have all we need, we get to
the actual body
| // | ||
| do { | ||
| BuildAndSendPackets(Binding, Route, &PacketParams, &ClientContext, false); // Don't fuzz this one | ||
| BuildAndSendLongHeaderPackets(Binding, Route, PacketParams, ClientContext, false); |
There was a problem hiding this comment.
Please keep / improve the comment here.
Even better if you clarify which packet this is (I assume this is meant to be the INITIAL?)
| PacketParams.NumFrames = 1; | ||
| PacketParams.FrameTypes[0] = QUIC_FRAME_CRYPTO; | ||
| PacketParams.NumPackets = 1; | ||
| BuildAndSendLongHeaderPackets(Binding, Route, &PacketParams, &ClientContext, false); |
There was a problem hiding this comment.
I am slightly confused by this, why isn't CompleteHandshake taking care of it?
| // Use the provided frame type (which encodes FIN, LEN, and OFF bits) | ||
| bool HasOffset = (FrameType & 0x04) != 0; | ||
| bool HasLength = (FrameType & 0x02) != 0; | ||
| bool HasFin = (FrameType & 0x01) != 0; |
| _Out_writes_to_(BufferLength, *Offset) uint8_t* Buffer | ||
| ) | ||
| { | ||
| CXPLAT_FRE_ASSERT(*Offset < BufferLength); |
There was a problem hiding this comment.
nit: The AI review pointed out that this assert won't prevent writing out of bounds.
While the assert is true and makes sense, a bit more validation could help avoid memory corruption issues (at least asserting the final offset is still smaller than BufferLength, if you assume the caller knowns what it does.
Same for all Write* functions below.
2 participants
Description
Implemented 1rtt fuzzing in recvfuzz tool
Fixes #5712.
Purpose
Fuzzing is done to find vulnerabilites
Testing
Locally
Documentation
No