Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quic crypto reassembly 7556 v7 #12626

Closed

Conversation

catenacyber
Copy link
Contributor

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7556

Describe changes:

  • quic: handle fragment reassembly
  • quic: parse ack3 frames
  • quic: handle retry packets

SV_BRANCH=OISF/suricata-verify#2307

#12617 with

  • rustfmt fix
  • another commit to handle quic retry packets (resetting the keys)

Will alow to have decode_frames accept one additional parameter
with past fragment data
cf rfc9000 section 19.3. ACK Frames

Ticket: 7556
Ticket: 7556

To do so, we need to add 2 buffers (one for each direction)
to the QuicState structure, so that on parsing the second packet
with hello/crypto fragment, we still have the data of the first
hello/crypto fragment.

Use a hardcoded limit so that these buffers cannot grow indefinitely
and set an event when reaching the limit
Ticket: 7556

Avoids failed_decrypt events when the first packet seen is not
a Quic Initial packet
@catenacyber catenacyber requested a review from a team as a code owner February 19, 2025 12:27
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 89.71963% with 11 lines in your changes missing coverage. Please review.

Project coverage is 80.76%. Comparing base (10ede91) to head (88a0e9d).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12626      +/-   ##
==========================================
+ Coverage   80.74%   80.76%   +0.01%     
==========================================
  Files         931      931              
  Lines      259144   259224      +80     
==========================================
+ Hits       209242   209352     +110     
+ Misses      49902    49872      -30     
Flag Coverage Δ
fuzzcorpus 56.97% <60.95%> (+<0.01%) ⬆️
livemode 19.37% <0.00%> (-0.02%) ⬇️
pcap 43.99% <89.52%> (-0.16%) ⬇️
suricata-verify 63.44% <89.52%> (+<0.01%) ⬆️
unittests 58.33% <30.84%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@victorjulien victorjulien added this to the 8.0 milestone Feb 19, 2025
@victorjulien
Copy link
Member

Passed my QA. Ran this PR with SV PR OISF/suricata-verify#2307. Local pipeline 5256, run 735.

@catenacyber
Copy link
Contributor Author

🤞

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.error.quic.parser 35 0 -
.app_layer.tx.quic 995 1030 103.52%

Pipeline 24812

@victorjulien
Copy link
Member

Merged in #12631, thanks!

@victorjulien
Copy link
Member

Great work, thanks @catenacyber. I think there are still some more fixes to be done, see the last pcap I added to the ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants