[core] Exact cipher matching by string splitting - #709
Open
zandeez wants to merge 1 commit into
Open
Conversation
zandeez
changed the title
Contributor
Author
|
Rebased on current master head. |
Contributor
|
We know sometimes it may be too much to ask, but adding unit-tests to this sort of core patches would speed-up merging considerably. |
zandeez
force-pushed
the
master
branch
2 times, most recently
from
August 15, 2026 08:57
dfa15a1 to
0f8cfe9
Compare
Incoming SDP crypto attributes were matched against known suites with switch_stristr(), a substring search over the whole crypto line. This made matching sensitive to suite table ordering and vulnerable to collisions (eg "AEAD_AES_256_GCM" matching inside "AEAD_AES_256_GCM_8"), breaking negotiation with some Cisco phones. switch_core_session_check_incoming_crypto() now splits the crypto attribute into fields and compares the suite name/alias field with strcmp() for an exact match, independent of ordering. The SUITES[] table and switch_rtp_crypto_key_type_t enum are reordered to group GCM/GCM_8 variants together. switch_core_media_crypto_str2type() had the same collision problem (prefix matching, relying on the old table ordering to stay safe) and is fixed the same way: a match now requires a delimiter (space or end of string) immediately after the suite name/alias. Also: - check the crypto_cpy malloc() return before writing into it - pull in signalwire#951: also accept a suite's alias, not just its name, when parsing crypto_suite_order preferences (fixes SRTP media crypto keys not working for suites offered by their alias) - now that str2type() is collision-safe, switch_core_session_parse_crypto_prefs() calls it directly instead of duplicating the name/alias matching loop (per review feedback on signalwire#951 asking why the block wasn't just calling switch_core_media_crypto_str2type()) - add unit tests covering switch_core_media_crypto_str2type(), including the GCM/GCM_8 collision regression Co-authored-by: Ajay Sabat <ajay.sabat@five9.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Rebased on master, Claude generated unit tests (visually look fine) also pulled in semi-related PR #951 including the suggested use of the preexisting function. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This request build on the previous one as it was discovered that it didn't fully resolve the issues with the Cisco phones. I believe that the Cisco devices are also relying on substring matches, meaning the shorter ciphers need to be placed first, This causes issues where FreeSWITCH behaves in the same way and I originally resolved by adding a space into the string.
This pull request splits the string and checks for an exact match in the second field, eliminating the need for the space in the string and meaning that as far as FreeSWITCH is concerned, the cipher order is now less relevant. In my tests, calls in both directions now function correctly using the AEAD_AES_265_GCM codec.
Resolves #714