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

Fixed fragment data size in IPv6 defragmentation #4228

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shapaz
Copy link

@shapaz shapaz commented Jan 24, 2024

Checklist:

  • If you are new to Scapy: I have checked CONTRIBUTING.md (esp. section submitting-pull-requests)
  • I squashed commits belonging together
  • I added unit tests or explained why they are not relevant
  • I executed the regression tests (using cd test && ./run_tests or tox)
  • If the PR is still not finished, please create a Draft Pull Request

fixes #xxx

@guedou
Copy link
Member

guedou commented Jan 24, 2024

Could you share an example that triggers the issue that your are aiming to fix? That will allow us to understand your PR.

Copy link

codecov bot commented Jan 24, 2024

Codecov Report

Merging #4228 (6c901ba) into master (d71014a) will decrease coverage by 32.67%.
Report is 3 commits behind head on master.
The diff coverage is 0.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4228       +/-   ##
===========================================
- Coverage   81.77%   49.10%   -32.67%     
===========================================
  Files         331      331               
  Lines       76721    76725        +4     
===========================================
- Hits        62736    37675    -25061     
- Misses      13985    39050    +25065     
Files Coverage Δ
scapy/layers/inet6.py 41.04% <0.00%> (-47.50%) ⬇️

... and 234 files with indirect coverage changes

@shapaz
Copy link
Author

shapaz commented Jan 24, 2024

@guedou When there are extra bytes after the IP packet (e.g., Ethernet trailer displayed as a Padding layer), it becomes part of q.payload which is appended to fragmentable. This causes fragmentable to include wrong data and to grow in size beyond the intended fragment data size. In this case warnings will be issued for the next fragment, for having a wrong offset field (since fragmentable became too large).

Example that triggers the issue:

pkt = Ether() / IPv6() / ICMPv6EchoRequest(data='b'*100)
frags = fragment6( pkt, 100 )
defragment6( frag / Padding('a'*8) for frag in frags )

My pull request correctly builds the fragment data that is appended to fragmentable.

@guedou
Copy link
Member

guedou commented Jan 26, 2024

Thanks for your explanation. Could you add a unit test based on your example at https://github.com/secdev/scapy/blob/master/test/scapy/layers/inet6.uts#L1817 ?

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

Successfully merging this pull request may close these issues.

2 participants