Skip to content

Preventing problems with segments out of order #3533

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

Closed
wants to merge 1 commit into from

Conversation

plutec
Copy link

@plutec plutec commented Feb 17, 2022

Brief description

I detected an "out-of-order" TCP segment in the trace, could make the TCPSession stop inmediatly.

out_of_order_segment

The fix is pretty simple, some out of order segments could generate a sequence number < 0, that make the StringBuffer used to collect all the data from the segments raises an error. To prevent it, we just check the seq number is > 0 before to continue with the append of the data.

@plutec
Copy link
Author

plutec commented Feb 18, 2022

The checks are not working because the last commit to the master branch does not pass all the tests:
4f42d25

@gpotter2
Copy link
Member

I get the point although I'm not sure if the behavior in this case will be intended :/

@plutec
Copy link
Author

plutec commented Feb 24, 2022

What do you mean with "Intended"? If a tool is running the "sniff" function with TCPSession and it receives an "out-of-order" TCP segment, it will crash inmediatly. Under my point of view, I think is better to manage that, instead to stop the execution abruptly.

@shrddr
Copy link

shrddr commented Apr 10, 2022

An "out-of-order" TCP segment breaking TCPSession? Weird because the whole reason to use TCPSession is to reorder the segments that are out-of-order... Oh wait it's not doing its job at all because: https://github.com/secdev/scapy/blob/master/scapy/sessions.py#L323

@plutec
Copy link
Author

plutec commented Apr 10, 2022

Hi @shrddr,
The problem is not when the packet is missing, in that case nothing wrong must happen. The problem is when the sequence number is invalid (<0 after calculus).

@stryngs
Copy link

stryngs commented Dec 6, 2022

This sounds like a fun and useful bug. Will try to replicate and report back.

@stryngs
Copy link

stryngs commented Dec 7, 2022

I tested tonight, got this:

ValueError: Incorrect type of value for field seq:
struct.error('argument out of range')
To inject bytes into the field regardless of the type, use RawVal. See help(RawVal)

Set [TCP].seq = -3. Scapy did let me do this, but upon trying to assemble into something useful scapy throws an Exception. Can you produce a pcap that will crash scapy @plutec via a sniff()?

@plutec
Copy link
Author

plutec commented Dec 14, 2022

Hi @stryngs
Here is a video I made in February and the code to reproduce the error (included the pcap):
https://github.com/plutec/scapy_poc

Regards,

@gpotter2
Copy link
Member

gpotter2 commented Feb 4, 2024

fixed in #4082

@gpotter2 gpotter2 closed this Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants