[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=249341#comment-249341 ] 

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

As for your:
{noformat}
If you have strictrtp turned on, I would expect Asterisk to strictly enforce this.
{noformat}
The strictrtp is there _not_ to disallow the session media to be changed. It is to disallow unrelated stranges from enumerating my Asterisk-ports and stealing the audio stream. (RTPbleed, or however you want to call it.)

And, now, a short git-blame history lesson. _To emphasize that the current codebase was not the result of careful design, but rather grew organically._

{noformat}
commit 6aaa99230115240b6bcf757183d16244865e1232
Author: Russell Bryant <russell at russellbryant.com>
Date:   Wed Jan 16 21:53:10 2008 +0000

    Merge the changes from issue #10665 from the team/group/sip_session_timers branch.

    This set of changes introduces SIP session timers support (RFC 4028).  In short,
    this prevents stuck SIP sessions that were not properly torn down due to network
    or endpoint failures during an established SIP session.
...
+       if (p->sessionversion_remote < 0 || p->sessionversion_remote != rua_version) {
+               p->sessionversion_remote = rua_version;
+               p->session_modify = TRUE;
{noformat}
That used to be a `!=`, not a `>`. And it appears it was added to handle a problem with session (timer) updates, not to avoid the media stream getting updated.

And when {{SIP_PAGE2_IGNORESDPVERSION}} was added in {{91192e30c}}, it still was. That option was _not_ created for the use case described here, but for broken clients which didn't update the session-version _at all_.

First when change {{85e57521a}} was done (when others were facing issues due to broken clients again), the `!=` check was changed to a `>`. At this point, partial RFC reading created the broken situation where we are now. (Explicitly not blaming Kevin or anyone else here.)

{noformat}
commit 85e57521ab6eee3dc866e5882694f054f98b7186
Author: Kevin P. Fleming <kpfleming at digium.com>
Date:   Mon Jun 15 20:42:38 2009 +0000

    Accept T.38 re-INVITE responses with invalid SDP versions.
...
-       if (ast_test_flag(&p->flags[1], SIP_PAGE2_IGNORESDPVERSION)
-               || p->sessionversion_remote < 0
-               || p->sessionversion_remote != rua_version) {
+       if (ast_test_flag(&p->flags[1], SIP_PAGE2_IGNORESDPVERSION) ||
+           (p->sessionversion_remote < 0) ||
+           (p->sessionversion_remote < rua_version)) {
{noformat}

QED

> 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)