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

Add QUIC token parsing and QUIC packet type support #36

Merged
merged 3 commits into from
Jul 12, 2024

Conversation

sippejw
Copy link
Contributor

@sippejw sippejw commented Jul 11, 2024

Changes

  • Adds three new fields to the QuicLongHeader struct: token_len, token, and retry_tag. These are optional fields populated based on the packet type as outlined in RFC 9000 17.2.
  • Modifies the packet_type field to be an enum instead of u8, improving error handling for malformed packets.
  • Consolidates data slice access validation by creating access function
  • Adds offset for tracking parse depth

Testing

  • All pcaps in traces, including new pcap for QUIC Retry packets (traces/quic_retry.pcapng)
  • Five minutes on live network traffic
Port 0 statistics
SW Capture %: UNKNOWN
Out of Buffer %: UNKNOWN
HW Discard %: UNKNOWN
+--------------+---------------------------+
|    616260180 | rx_good_packets           |
| 410049028371 | rx_good_bytes             |
|      1571836 | rx_missed_errors          |
|            0 | rx_mbuf_allocation_errors |
+--------------+---------------------------+
----------------------------------------------
Current time: 299s
mempool_0 avail: 8355990, in use: 32618 (0.389%)
Ingress: 0 bps / 0 pps
Good:    10976513488 bps / 2048164 pps
Process: 10976513488 bps / 2048164 pps
Drop: 330 pps (inf%)
HW Dropped: 0 pkts (NaN%)
SW Dropped: 1571836 pkts (inf%)
Total Dropped: 1571836 pkts (inf%)
----------------------------------------------
AVERAGE Ingress: 0.000 bps / 0.000 pps
AVERAGE Good:    11405081146.067 bps / 2067987.181 pps
AVERAGE Process: 11405081146.067 bps / 2067987.181 pps
DROPPED: 1571836 pkts (inf%)

Main done. Ran for 300.100601911s
Done. Logged 19779053 Quic stream to "quic.jsonl"

Copy link
Collaborator

@thearossman thearossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for submitting! Excited to get this merged.

When I tried to test this on our traffic, I got a panic here:

Initializing Retina runtime...
thread '<unnamed>' panicked at core/src/protocols/stream/quic/parser.rs:225:44:
range end index 2675 out of range for slice of length 26
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
Aborted

I'm guessing that there are length/offset validations missing, but I haven't looked into it deeply.

Separately, could you clarify what testing you've done in the PR? It's helpful to have this documented for PRs since we don't have automated live traffic tests.

Thank you!

@sippejw
Copy link
Contributor Author

sippejw commented Jul 11, 2024

Hi @thearossman, apologies for that. Would you be able to provide the pcap you were testing with? I have tested with all of the pcaps currently in the traces directory. However, when running on our live traffic I am now seeing the same issue.

@thearossman
Copy link
Collaborator

nw! Unfortunately, we're running it on live traffic and can't capture traffic (or write data to disk) for privacy reasons. It sounds like you have access to separate live traffic and can repro?

@sippejw
Copy link
Contributor Author

sippejw commented Jul 11, 2024

Yes, the issue is intermittent, and I've been able reproduce once, so I am unable to determine exactly what packets are causing this. Regardless, I am making some changes to the length validation and will update the PR once they are done. Thanks!

@sippejw sippejw requested a review from thearossman July 11, 2024 21:19
Copy link
Collaborator

@thearossman thearossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I ran this for a while and eventually got a start > end slice index error at L263. Super weird, but looks like slice indices may need to be validated too (for overflow/underflow?)

thread '<unnamed>' panicked at core/src/protocols/stream/quic/parser.rs:263:44:
slice index starts at 27 but ends at 15
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
Aborted

and on a different core --

thread '<unnamed>' panicked at core/src/protocols/stream/quic/parser.rs:263:44:
slice index starts at 8 but ends at 6

@sippejw
Copy link
Contributor Author

sippejw commented Jul 12, 2024

Those panics were caused due to the way retry packets are being parsed, the length of the token is unknown so you must work backwards and subtract the fixed length of the integrity tag. I've reworked how the data slice is accessed so that validation is consistent for each access and indices where start > end throws an error. This should solve the issue, but I was unable to reproduce this particular problem. We do not see many retry packets on our network.

@sippejw sippejw requested a review from thearossman July 12, 2024 17:11
Copy link
Collaborator

@thearossman thearossman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's subtle. I just ran this for about 10 minutes with no panics and what looks like reasonable/correct output, and it LGTM. Going to merge. Thanks again.

@thearossman thearossman merged commit b7b2c9f into stanford-esrg:main Jul 12, 2024
2 checks passed
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.

2 participants