[asterisk-bugs] [JIRA] (ASTERISK-28686) chan_sip strictrtp=yes fails when media source is changed: no audio

[Walter Doekes (JIRA)]

    [ https://issues.asterisk.org/jira/browse/ASTERISK-28686?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=249365#comment-249365 ] 

Walter Doekes commented on ASTERISK-28686:
------------------------------------------

{quote}
The stance has always been that you can't have a 200 and then later on something up and changes the SDP on you without some form of checking or without you expressly saying that is okay with you by ignoring the sdp versioning
{quote}
Yes. While I can see where that is coming from, from the RFC 3264 point of view, I have still yet to find a use-case where doing that fixes a problem in SIP. Remember that SDP is encapsulated in SIP, which also keeps packet identification and order.

If/when the remote device decides to let someone else handle the media (establish an entirely new media session if you will), we have very little to gain by not trusting the remote device. It has the SIP identifiers needed to identify itself (from/to/callid). If an attacker has those identifiers it will most likely have the o= line as well. So if it wanted to abuse anything, it would just increment the sess-version and not be bothered by this check.

As for multiple simultaneous 200s (from different to-tags): Asterisk will choose the first one and BYE the others. At this point, the to-tag would be locked, and other 200s (with different to-tags) wouldn't influence the SDP-session state.

My concerns are:
- the `!=` check was added when session timers were added: there was likely a reason to ignore the there-are-no-updates SDP in those case; so I don't just want to ignoresdpversion=yes (it might open up strictrtp learning at every session-timer interval?)
- the current check is broken; so it only makes sense to fix it (in any direction, in this case the one that suits me best)

> chan_sip strictrtp=yes fails when media source is changed: no audio
> -------------------------------------------------------------------
>
>                 Key: ASTERISK-28686
>                 URL: https://issues.asterisk.org/jira/browse/ASTERISK-28686
>             Project: Asterisk
>          Issue Type: Bug
>      Security Level: None
>          Components: Channels/chan_sip/Interoperability
>    Affects Versions: 13.30.0
>            Reporter: Walter Doekes
>            Assignee: Walter Doekes
>
> Hi!
> When the SDP origin changes, the SDP session version from the new media is not necessarily higher than the existing session version.
> chan_sip contains this bit:
> {code}
>         if (ast_test_flag(&p->flags[1], SIP_PAGE2_IGNORESDPVERSION) ||
>             (p->sessionversion_remote < 0) ||
>             (p->sessionversion_remote != rua_version)) {
>                 p->sessionversion_remote = rua_version;
>         } else {
> ...
>                         ast_debug(2, "Call %s responded to our reinvite without changing SDP version; ignoring SDP.\n", p->callid);
>                         return FALSE;
> {code}
> That means that the updated SDP will be ignored for a change like this:
> - Initial o= line (for example in 183):
> {noformat}
> o=root 1845921041 1845921041 IN IP4 193.x.x.215
> {noformat}
> - Updated o= line (for example in 200):
> {noformat}
> o=- 1126144851 1126144853 IN IP4 87.x.x.235
> {noformat}
> In this example, the new SDP sess_version 1126144851 is lower than 1845921041, causing the SDP to be ignored. And because the SDP is not processed, {{ast_rtp_instance_set_remote_address}} is never called, and the new RTP source is discarded.
> The fix: do not only check sess_version, but also check the "unique parts" from the SDP. And if they change, then also update the SDP.
> Basically this:
> {noformat}
>         if (ast_test_flag(&p->flags[1], SIP_PAGE2_IGNORESDPVERSION) ||
> -           (p->sessionversion_remote < 0) ||
> -           (p->sessionversion_remote < rua_version)) {
> -               p->sessionversion_remote = rua_version;
> +                       sess_version > p->sessionversion_remote ||
> +                       strcmp(unique, S_OR(p->sessionunique_remote, ""))) {
> +               p->sessionversion_remote = sess_version;
> +               ast_string_field_set(p, sessionunique_remote, unique);
>         } else {
> {noformat}
> Along with a marginally bigger patch (to be put on gerrit), we then get these:
> {noformat}
> [2020-01-13 10:31:20] VERBOSE[10720][C-00000001] chan_sip.c:
>   Got SDP version 203027438 and unique parts [- 203027438 IN IP4 10.x.x.161]
> [2020-01-13 10:31:20] VERBOSE[10720][C-00000001] chan_sip.c:
>   Got SDP version 1845921041 and unique parts [root 1845921041 IN IP4 193.x.x.215]
> [2020-01-13 10:31:31] VERBOSE[10720][C-00000001] chan_sip.c: 
>   Comparing SDP version 1845921041 -> 1126144853 and unique parts [root 1845921041 IN IP4 193.x.x.215] -> [- 1126144851 IN IP4 87.x.x.235]
> {noformat}
> And then everything works as intended.



--
This message was sent by Atlassian JIRA
(v6.2#6252)