Fix unauth-SSRC memory grow

[sirzooro]

Fix unauthenticated-SSRC memory exhaustion and add security-warning logging

Summary

This change addresses two classes of vulnerability where unauthenticated data in RTP/RTCP headers could be used to allocate unbounded per-SSRC state before the auth tag is verified.

Security Fixes

Unauthenticated SSRC state allocation (memory exhaustion)

srtp.go / srtcp.go — getSRTPSSRCState and getSRTCPSSRCState now accept a keepNew bool parameter. During decryption the SSRC is read from the unauthenticated RTP/RTCP header, so these functions are called in read-only mode (keepNew = false): a temporary state object is created but not inserted into the map. The state is committed to the map by the new setSRTPSSRCState / setSRTCPSSRCState helpers only after markAsValid() succeeds (i.e. authentication is confirmed).

session_srtp.go — decryptRTP is now called before getOrCreateReadStream. Previously an unauthenticated peer could cause getOrCreateReadStream to allocate a per-SSRC stream for an arbitrary spoofed SSRC, exhausting memory without ever providing a valid packet.

Both fixes include inline comments explaining the security rationale and the intentional ordering of the replay check relative to authentication.

What Changed

context.go

• Added setSRTPSSRCState and setSRTCPSSRCState helpers for explicit post-auth state commitment.
• getSRTPSSRCState and getSRTCPSSRCState return a second bool indicating whether the state was pre-existing.
• SetROC and SetIndex updated to pass keepNew = true (they always want to create state).

session_srtp.go

• Moved decryptRTP call above getOrCreateReadStream to prevent unauthenticated stream allocation (see Security Fixes above).

srtcp.go / srtp.go

• Decryption paths updated to use getSRTCPSSRCState(ssrc, false) / getSRTPSSRCState(ssrc, false) with deferred setSRTCPSSRCState / setSRTPSSRCState after authentication.
• Encryption paths updated to use getSRTCPSSRCState(ssrc, true) / getSRTPSSRCState(ssrc, true) (no behaviour change for encrypt).

API Notes

• getSRTPSSRCState / getSRTCPSSRCState signatures changed (internal); callers outside the package are unaffected.

Tests

• All existing SRTP/SRTCP unit tests pass with go test ..

Reviewer Notes

• The core security invariant is: no per-SSRC state (map entry, replay detector, or stream) is committed until the packet's auth tag has been successfully verified.
• The replay check remains intentionally before authentication to avoid wasting CPU on AES/HMAC for flooded duplicate packets; safety is preserved because the replay detector only records an index as "seen" when markAsValid() is explicitly called.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.54%. Comparing base (51804b0) to head (02154e9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   82.42%   82.54%   +0.11%     
==========================================
  Files          19       19              
  Lines        1462     1472      +10     
==========================================
+ Hits         1205     1215      +10     
  Misses        142      142              
  Partials      115      115              

Flag	Coverage Δ
go	82.54% <100.00%> (+0.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Sean-Der]

Nice find!

@sirzooro would you mind adding a test to assert no allocations on auth failure.

IMO the warning on NULL crypto isn’t needed. If we add to much noise then maybe something real would get ignored.

[Sean-Der]

Does doing replay detection without authentication allow a DoS? I can spam RTP packets and break sessions now without keying materials

[sirzooro]

Nice find!

@sirzooro would you mind adding a test to assert no allocations on auth failure.

IMO the warning on NULL crypto isn’t needed. If we add to much noise then maybe something real would get ignored.

Added new tests. Is there some valid production use of NULL cipher beside debugging? If yes, I would add another option to silence these warnings. I would like to have at least warning if someone would accidentally use insecure configuration in production environment.

Does doing replay detection without authentication allow a DoS? I can spam RTP packets and break sessions now without keying materials

I do not see anything obvious in the code, AI scan also did not reveal anything related to this. Maybe this is caused by increased CPU usage (higher jitter) or receive UDP buffer was overflown and some frames were dropped?

[sirzooro]

@Sean-Der @JoTurk is something more needed here?

[JoTurk]

i looked yesterday and it looks good to me, but i didn't really review it, I can review it tonight, @Sean-Der what do you think?

[Sean-Der]

@sirzooro Can we reduce it down to just fix unauth-SSRC memory grow

I would like to still keep single purpose commits (even if AI can do big changes easily) especially in these security changes. Sorry for the push back :/

I think we should merge/tag that right away!

[sirzooro]

@Sean-Der I have removed logging changes. PR contains main fix for unauth-SSRC memory grow, and two other small ones in context.go (first two points in description above). I also updated PR's description to match actual changes.

[sirzooro]

@Sean-Der small reminder :)

[JoTurk]

@sirzooro I think we can just break the changes into separate commits in this PR, the idea is that if someone is looking at blame or reviewing the changes that happens to a repo it's a lot easier if commit = single change, also if we want to revert stuff for whatever reason.

[sirzooro]

I already removed logging changes from this PR, what is left is fix for issue in the title plus two minor fixes in CreateContext. I also had to do small refactoring (renamed s variable to state), lint complained about this.

[JoTurk]

@sirzooro I'm sorry can we rebase these minor fixes to a separate commit, it also makes more sense for security fixes in case we want to release a CVE or just link the patch / commit.

[sirzooro]

@JoTurk done, removed it from this PR.

[JoTurk]

Thank you.