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

Incorrect RTCP SR + RR parsing #4348

Closed
cdleonard opened this issue Apr 10, 2024 · 6 comments · Fixed by #4349
Closed

Incorrect RTCP SR + RR parsing #4348

cdleonard opened this issue Apr 10, 2024 · 6 comments · Fixed by #4349

Comments

@cdleonard
Copy link
Contributor

Brief description

The RTCP parser fails to handle a packet that contains both Sender Report and Received Report, which is is the most common data for a two-way session.

It seems that the "sender_info" info contain a payload, this should be parsed as a ReceptionReport info

Incorrect behavior demonstrated in UTS here: 0bb9db2

Scapy version

main

Python version

3.10

Operating system

Linux 5.15.146

Additional environment information

No response

How to reproduce

Run tests on provided branch:

test/run_tests -P "load_contrib('rtcp')" -t test/contrib/rtcp.uts -F

Actual result

Demo test should fail.

ReceptionReport after SenderInfo should be parsed. SenderInfo should never have a payload, it's a fixed-sized struct

Expected result

The commented asserts should pass instead

Related resources

https://datatracker.ietf.org/doc/html/rfc3550

@cdleonard
Copy link
Contributor Author

@oborichkin

cdleonard added a commit to cdleonard/scapy that referenced this issue Apr 11, 2024
By default when parsing these packets parse everything as their
"payload" but they are in fact fixed-size.

Fixes Issue secdev#4348
@cdleonard
Copy link
Contributor Author

After some further digging it seems that when using consecutive PacketField those Packet sub-classes should implement a custom "extract_padding" function to determine their own length.

Made a PR for this. The test includes an adapted version of the earlier test which demonstrated the bug - it now demonstrates correct build+parse of a RTCP container SR and multiple RR

@oborichkin
Copy link
Contributor

@cdleonard see #3580
This issue is fixed by adding extract_padding to SenderInfo:

    def extract_padding(self, p):
        return "", p

I honestly don't remember why I kept MR in Draft, maybe fixing conflicts will be enough for most cases.

@oborichkin
Copy link
Contributor

On a second thought I remember that it was something to do with conditional fields and count_from/count_of. I'm not very happy with how they are ended up

@cdleonard
Copy link
Contributor Author

My PR is just for this specific issue and it seems obviously correct.

Maybe it should be merged and then you can rebase any additional improvements on top?

@oborichkin
Copy link
Contributor

seems reasonable

gpotter2 pushed a commit that referenced this issue Apr 14, 2024
* rtcp: Add extract_padding to SenderInfo and ReceptionReport

By default when parsing these packets parse everything as their
"payload" but they are in fact fixed-size.

Fixes Issue #4348

* rtcp: Add test demonstrating parsing of SR+RR
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 a pull request may close this issue.

2 participants