SRTP: Fix mis-parsing of long input line #822

orgads · 2025-12-06T20:50:41Z

A literal space in sscanf means "0 or more spaces". So a string longer than 40 chars was split between e.g. primary_audio_cryptokeyparams and crypto_audio_sessionparams.

orgads · 2025-12-06T20:51:01Z

@jeannotlanglois Please review.

jeannotlanglois · 2025-12-06T21:08:36Z

I will take a look on Subnday
I've just became a first-time new dad recently, time is a bit scarce but planning some on Sunday for a good check on this. Planning to restore the original code with memsets which had been extensively tested but somehow taken out by soneone. JLSRTP shouldn't have to be modified as by design caller should provide NULL-terminated values and this has been confirmed to work for many years without a hitch before it was merged in.

orgads · 2025-12-07T06:51:40Z

@jeannotlanglois I replaced memset with assignment of null in position 0. There's no reason to fill the entire buffer just for null-termination. It should just be handled correctly.

Regarding JLSRTP, the only change I did there was to reserve buffer size before appending, to avoid repeating reallocations. It's not critical, but can improve performance a bit.

jeannotlanglois

Line #469 changes the return code that used to be -1 when no SDP body is found to 0 - this is a behavior change - please change back to -1.

jeannotlanglois · 2025-12-07T18:18:38Z

Hmm... for some reason I was only able to submit a single comment in my review... so adding a second one here:

Also lines 475-725 involve some important changes in the SDP parsing code - I've spent a lot of hours getting this to work right for many cases involving different kinds of SDP m-lines. I did not see any mention of testcases - which scenarios have you run to validate that the parsing still works as it used to?

As a minimum there should be 4 scenarios per media type:
m=audio:
single m-line: RTP/AVP w/o crypto (aka "no encryption")
single m-line: RTP/SAVP w/crypto (aka "normal encryption")
single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption")
two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to the same port)

m=video:
single m-line: RTP/AVP w/o crypto (aka "no encryption")
single m-line: RTP/SAVP w/crypto (aka "normal encryption")
single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption")
two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to the same port)

There's a lot of effort involved in supporting these scenarios - we can't just change the code w/o retesting and hoping for the best that things will work - they usually don't and then in the future we end up in the same kind of regression bug fixing like we're doing now.

Please explain what was done to test these changes.
Many of the sample xml scenarios provided allow to test these cases, so you should be able to validate semi-manually by taking some time to run the ones that apply and confirm that the parsing happens as expected in each case.

src/call.cpp

orgads · 2025-12-08T20:35:31Z

Line #469 changes the return code that used to be -1 when no SDP body is found to 0 - this is a behavior change - please change back to -1.

It was a preparation for returning -1 on real failures. Not having SDP is not really a failure.

In practice, the return value is never checked, so it doesn't change any actual behavior.

Regarding tests, aren't there unit tests for these files? I ran some manual tests, but only on 2 files.

A literal space in sscanf means "0 or more spaces". So a string longer than 40 chars was split between e.g. primary_audio_cryptokeyparams and crypto_audio_sessionparams. Fix by using "non-space" regex, and explicit 1 space after inline.

jeannotlanglois · 2025-12-14T18:00:15Z

It was a preparation for returning -1 on real failures. Not having SDP is not really a failure.
In practice, the return value is never checked, so it doesn't change any actual behavior.
Regarding tests, aren't there unit tests for these files? I ran some manual tests, but only on 2 files.

Still that is not correct - that function is meant to only be called when SDP has been detected by the caller. Otherwise it's a logic error. That is why the -1 error code is for. So you changed the behavior here. Please fix.

jeannotlanglois · 2025-12-14T18:03:02Z

As a minimum there should be 4 scenarios per media type:
m=audio:
single m-line: RTP/AVP w/o crypto (aka "no encryption")
single m-line: RTP/SAVP w/crypto (aka "normal encryption")
single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption")
two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to >the same port)

m=video:
single m-line: RTP/AVP w/o crypto (aka "no encryption")
single m-line: RTP/SAVP w/crypto (aka "normal encryption")
single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption")
two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to >the same port)

There's a lot of effort involved in supporting these scenarios - we can't just change the code w/o retesting and hoping for the >best that things will work - they usually don't and then in the future we end up in the same kind of regression bug fixing like >we're doing now.

Please explain what was done to test these changes.
Many of the sample xml scenarios provided allow to test these cases, so you should be able to validate semi-manually by taking >some time to run the ones that apply and confirm that the parsing happens as expected in each case.

I still did not see evidence of these testcases being revalidated to work - please test. This work cannot be considered completed until those are verified to pass. The provided XML files allow these to be easily re-run and validated. Using wireshark in between also allows to perform additional validation.

If you think these testcases might be a bit confusing ( I understand ) I can probably reserve some time to help with this revalidation next sunday. I sill have my setup at home.

Essentially I use two machines with two distinct IPs which makes validation a LOT easier. One acts as the SIPP-UAC and the other acts as SIPP-UAS.

Then running these scenarios can be done within a matter of minutes ( assuming nothing is broken ).

orgads · 2025-12-14T18:05:07Z

It was a preparation for returning -1 on real failures. Not having SDP is not really a failure.
In practice, the return value is never checked, so it doesn't change any actual behavior.
Regarding tests, aren't there unit tests for these files? I ran some manual tests, but only on 2 files.

Still that is not correct - that function is meant to only be called when SDP has been detected by the caller. Otherwise it's a logic error. That is why the -1 error code is for. So you changed the behavior here. Please fix.

But the function is called for every message, and its return value is never checked.

How do you suggest to treat real failures like bad sdp format?

jeannotlanglois · 2025-12-14T18:07:46Z

It was a preparation for returning -1 on real failures. Not having SDP is not really a failure.
In practice, the return value is never checked, so it doesn't change any actual behavior.
Regarding tests, aren't there unit tests for these files? I ran some manual tests, but only on 2 files.

Still that is not correct - that function is meant to only be called when SDP has been detected by the caller. Otherwise it's a logic error. That is why the -1 error code is for. So you changed the behavior here. Please fix.

But the function is called for every message, and its return value is never checked.

How do you suggest to treat real failures like bad sdp format?

The caller of the function are expected to call the function only when SDP is present. I seem to remember that this was the case.

The return code is expected to indicate the various possible errors in fine-grained fashion to help with debugging/troubleshooting:

If SDP is missing the return code is expected to be -1.
If other SDP errors are encountered there's a ton of other exit codes available (-2, etc).

orgads · 2025-12-14T18:37:05Z

Oh, now I see that extract_srtp_remote_info is only called if Content-Type is application/sdp, so indeed having an empty body is probably invalid.

orgads · 2025-12-14T18:39:01Z

Done.

jeannotlanglois · 2025-12-21T23:01:33Z

Running these testcases now on the latest master branch as I've got a little time to check your changes which were merged on master too early before they got confirmed to work again on a PR branch:



    As a minimum there should be 4 scenarios per media type:
    m=audio:
    single m-line: RTP/AVP w/o crypto (aka "no encryption")
    single m-line: RTP/SAVP w/crypto (aka "normal encryption")
    single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption")
    two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to >the same port)

    m=video:
    single m-line: RTP/AVP w/o crypto (aka "no encryption")
    single m-line: RTP/SAVP w/crypto (aka "normal encryption")
    single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption")
    two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to >the same port)

    There's a lot of effort involved in supporting these scenarios - we can't just change the code w/o retesting and hoping for the >best that things will work - they usually don't and then in the future we end up in the same kind of regression bug fixing like >we're doing now.

    Please explain what was done to test these changes.
    Many of the sample xml scenarios provided allow to test these cases, so you should be able to validate semi-manually by taking >some time to run the ones that apply and confirm that the parsing happens as expected in each case.

jeannotlanglois · 2025-12-21T23:29:51Z

After reviewing the list of testcases I remembered now that I only have the "N", "M" and "O1" provided with SIPP -- I did not create XML scenarios for the "O2" ones (so cannot run them right now).

Also for the context of what is being tested here "O1" is nearly equivalent to "M" (e.g. in the former "RTP/AVP" is used in the SDP, while in the latter "RTP/SAVP" is used in the SDP) -- there's no internal differences to SIPP with regards to parsing either of these two profiles so testing one is pretty much equivalent to the other.

RESULTS:

    m=audio -- e.g. single AUDIO stream:
    single m-line: RTP/AVP w/o crypto (aka "no encryption"):  PASSED (single offer/answer only).
    single m-line: RTP/SAVP w/crypto (aka "normal encryption"):  NOT RUN
    single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption"): PASSED (first+second offer/answers)
    two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to >the same port):  SCENARIO NOT AVAILABLE

    m=video -- e.g. single VIDEO stream:
    single m-line: RTP/AVP w/o crypto (aka "no encryption"):  PASSED (single offer/answer only)
    single m-line: RTP/SAVP w/crypto (aka "normal encryption"):  NOT RUN
    single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption"): PASSED (first+second offer/answers)
    two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to >the same port):  SCENARIO NOT AVAILABLE

    m=audio AND m=video -- e.g. one AUDIO stream + one VIDEO stream:
    single m-line: RTP/AVP w/o crypto (aka "no encryption"):  PASSED (single offer/answer only)
    single m-line: RTP/SAVP w/crypto (aka "normal encryption"):  NOT RUN
    single m-line: RTP/AVP w/crypto (aka "best effort/optional type 1 encryption"): PASSED (first+second offer/answers)
    two m-lines: RTP/SAVP w/crypto + RTP/AVP w/o crypto (aka "best effort/optional type 2 encryption - with both m-lines set to >the same port):  SCENARIO NOT AVAILABLE

GOOD NEWS:
With the testcases I've run everything looks fine.
This PR change can finally be considered completed as far as I'm concerned.

orgads · 2025-12-22T05:02:03Z

Thank you!

orgads force-pushed the srtp-long branch from 09d9261 to d4e3f2f Compare December 7, 2025 06:47

jeannotlanglois suggested changes Dec 7, 2025

View reviewed changes

jeannotlanglois reviewed Dec 7, 2025

View reviewed changes

src/call.cpp Show resolved Hide resolved

src/call.cpp Show resolved Hide resolved

src/call.cpp Show resolved Hide resolved

src/call.cpp Show resolved Hide resolved

orgads force-pushed the srtp-long branch from d4e3f2f to 782cf20 Compare December 8, 2025 20:35

orgads requested a review from jeannotlanglois December 8, 2025 20:35

SRTP: Fix mis-parsing of long input line

32e6c95

A literal space in sscanf means "0 or more spaces". So a string longer than 40 chars was split between e.g. primary_audio_cryptokeyparams and crypto_audio_sessionparams. Fix by using "non-space" regex, and explicit 1 space after inline.

orgads force-pushed the srtp-long branch from 782cf20 to 32e6c95 Compare December 9, 2025 11:51

orgads merged commit 1b954c4 into master Dec 14, 2025
14 checks passed

orgads deleted the srtp-long branch December 14, 2025 05:00

SRTP: Fix mis-parsing of long input line #822

SRTP: Fix mis-parsing of long input line #822

Uh oh!

Conversation

orgads commented Dec 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orgads commented Dec 6, 2025

Uh oh!

jeannotlanglois commented Dec 6, 2025

Uh oh!

orgads commented Dec 7, 2025

Uh oh!

jeannotlanglois left a comment

Choose a reason for hiding this comment

Uh oh!

jeannotlanglois commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orgads commented Dec 8, 2025

Uh oh!

Uh oh!

jeannotlanglois commented Dec 14, 2025

Uh oh!

jeannotlanglois commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orgads commented Dec 14, 2025

Uh oh!

jeannotlanglois commented Dec 14, 2025

Uh oh!

orgads commented Dec 14, 2025

Uh oh!

orgads commented Dec 14, 2025

Uh oh!

jeannotlanglois commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeannotlanglois commented Dec 21, 2025

Uh oh!

orgads commented Dec 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

orgads commented Dec 6, 2025 •

edited

Loading

jeannotlanglois commented Dec 7, 2025 •

edited

Loading

jeannotlanglois commented Dec 14, 2025 •

edited

Loading

jeannotlanglois commented Dec 21, 2025 •

edited

Loading