[0.x][1.x] SIP Plugin rejects incoming calls with anonymous From header url

[pawnnail]

Version v1.1.2 and v0.13.2. Tested in legacy but the same issue will be evident in multistream.

The issue was introduced with commit 76f33a4 "Fix some clang warnings.", see PR #2556.

Noted that incoming INVITEs are rejected with 400 response if the From header contains url with an empty user part such as:
"From: ;tag=abc123".

The plugin rejects with message "Invalid request (missing From or To)". A From header with an empty user is completely valid so it doesn't look right. The problem is traced to this code:

https://github.com/meetecho/janus-gateway/blob/83245f75986c3a46b13dca0a8939ea12b62afa64/src/plugins/janus_sip.c#L5142-L5147

For the header sample in question the value of "_sip->sip_from->a_url->urluser" is NULL and the value of "_sip->sip_from->aurl" is not.

All is needed to fix this is to relax the check and make it test the presence of "_sip->sip_from->aurl" only, i.e. remove the "_->urluser" part from the test: if(sip->sip_from == NULL || sip->sip_from->a_url == NULL || sip->sip_to == NULL || sip->sip_to->a_url == NULL) {...}

The code doesn't reference "_urluser" from From/To headers so it's safe to update the check. Not sure why it triggered clang warning.

[lminiero]

I don't remember this commit, but if it's a clang related warning fix, then it's very likely something @atoppi did. I'll let him address the specifics of why he thought that fix was necessary.

[atoppi]

The code doesn't reference "url_user" from From/To headers so it's safe to update the check. Not sure why it triggered clang warning.

This is the warning from clang:

warning: comparison of array 'sip->sip_from->a_url' equal to a null pointer is always false [-Wtautological-pointer-compare]
                        if(sip->sip_from == NULL || sip->sip_from->a_url == NULL ||
                                                    ~~~~~~~~~~~~~~~^~~~~    ~~~~

a_url is defined in a stuct as a static array of size 1

struct sip_addr_s
{
  sip_common_t       a_common[1];   /**< Common fragment info */
  sip_error_t       *a_next;
  char const        *a_display;     /**< Display name */
  url_t              a_url[1];      /**< URL */
  msg_param_t const *a_params;      /**< Parameter table  */
  char const        *a_comment;     /**< Comment */

  char const        *a_tag;     /**< Tag parameter */
};

Hence the check of the bare pointer is pointless (as clearly mentioned by clang).

As of the removal, we are not sure what the side effects of removing the check will be, since there are multiple references to sip->sip_from->a_url and sip->sip_from->a_display after that point.

Can you try removing the whole check and make a couple of tests to see that everything works as expected?

[pawnnail]

It does seem to be pointless, didn't think of that.

Have run the following tests with just if(sip->sip_from == NULL || sip->sip_to == NULL) {...} in place to see if a crash or "Invalid request" message is triggered:

1. From/To headers have empty user part in the initial INVITE - no errors
2. From/To headers have empty user part in the mid-dialog REFER request - no errors
3. Malformed initial INVITE has missing From or To header - processing never makes it to janus_sip_sofia_callback(), i.e. sofia drops request as expected
4. Malformed mid-dialog REFER has missing From - processing never makes it to janus_sip_sofia_callback(), i.e. sofia drops request as expected.

There aren't any side effects. I'd leave if(sip->sip_from == NULL || sip->sip_to == NULL) {...} just in case the sofia behaviour changes in the future even though it appears to be redundant at present.

[lminiero]

What is the value of the caller and displayname properties in the incomingcall event in that case? I'm trying to figure out if this may cause broken output in some contexts, even if there is no visible side effect code wise, before committing the fix.

Edit: sorry, meant username: caller is used in the missed_call event.

[pawnnail]

Presuming the context is an empty user part in the From header url, i.e From: "restricted caller" <sip:pstn.parramatta.sydney>;tag=abc123. Confusingly, the username field of the incomingcall event reflects the whole url and not just a user part of it. As a result the username field will never be empty. The displayname is independent of the user part and may or may not be empty.

Both username (sip:pstn.parramatta.sydney) and displayname ("restricted caller") may have a hypothetical value in screening of an incoming call, i.e. originating domain and a friendly category name of the caller. Otherwise, I may have misunderstood the question.

[lminiero]

username is indeed a misleading name, it's always the full SIP URI; displayname is the "vanity" part of it. I was curious to see what values they have in the case of anonymous From headers, after you removed those checks: I want to be sure that we won't introduce issues in the messages because of that. If the example you made above is a real test with an anonymous From and Janus gave you those values, that's good enough for me.

[lminiero]

@pawnnail any update on my comment above? Wondering if I can indeed push that fix with confidence, since I don't have a way to replicate the problem :hand_over_mouth: Thanks!

[pawnnail]

Oh, sorry, for the confusion, I didn't think further commenting was necessary. I can confirm that relaxing the check as shown below fixes the issue without any side effects:

if(sip->sip_from == NULL || sip->sip_from->a_url == NULL || sip->sip_to == NULL || sip->sip_to->a_url == NULL) {...}

Tested ok with empty user part in the From and To headers SIP URI and the call was processed as expected and not rejected. I also tested using malformed INVITEs without From or To header and the processing didn't make as far as sofia simply dropped the malformed requests (whether INVITE or reINVITE).

Would be fantastic if you pushed the fix and thanks for looking into the issue 🙂

[lminiero]

Thanks! I just pushed the fix to both master and 0.x.