[Home]

Summary:ASTERISK-18389: non-compliant code in chan_sip could be removed for asterisk10 release
Reporter:Walter Doekes (wdoekes)Labels:
Date Opened:2011-08-31 02:53:38Date Closed:2011-12-04 04:03:15.000-0600
Priority:TrivialRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Registration
Versions:10 Frequency of
Occurrence		Constant
Related
Issues:
causesASTERISK-18572 SIP REGISTER fails if :port appears in the To: header
Environment:Attachments:( 0) issueA18389.patch
Description:You could still remove this before the official asterisk10 release:

{code}
       if (ast_strlen_zero(of)) {
               /* XXX note: the original code considered a missing @host
                * as a username-only URI. The SIP RFC (19.1.1) says that
                * this is wrong, and it should be considered as a domain-only URI.
                * For backward compatibility, we keep this block, but it is
                * really a mistake and should go away.
                */
               of = domain;
{code}

And replace this with a if (ast_strlen_zero(domain)) return AUTH_SOME_FAILURE:

{code}
       /*! \todo XXX here too we interpret a missing @domain as a name-only
        * URI, whereas the RFC says this is a domain-only uri.
        */
       if (!ast_strlen_zero(domain) && !AST_LIST_EMPTY(&domain_list)) {
               if (!check_sip_domain(domain, NULL, 0)) {
                       transmit_response(p, "404 Not found (unknown domain)", &p->initreq);
                       return AUTH_UNKNOWN_DOMAIN;
               }
       }
{code}

And while looking at the XXX'es, I see this:

{code}
       buf[0] = '\0';
       /*XXX isn't strlen(buf) going to always be 0? */
       y = len - strlen(buf) - 5;
{code}

Yes, it's going to be 0.
Comments:By: Paul Belanger (pabelanger) 2011-10-18 12:24:56.794-0500

Patch?
By: Walter Doekes (wdoekes) 2011-10-18 14:57:51.423-0500

Here you go, Paul.

(1) The register_verify now enforces a proper user@domain.
(2) The check_user_full still doesn't *require* a name, but it won't match users by domain anymore.
(3) There was some freaky logic going on in get_msg_text: I think you could overflow buf if you put more than 4 lines in req->lines. Should be fixed now. (buf += linelen so strncat doesn't have to redo the strlen, strncat instead of strncpy for obvious reasons)
(4) In the reqresp parser there were lots of if (params) inside a big if (params) block.
By: Walter Doekes (wdoekes) 2011-10-18 15:01:51.946-0500

> I think you could overflow buf if you put more than 4 lines in req->lines

Blah. The unconditional +1 took care of that (even when addnewline was false). Oh well..
By: Paul Belanger (pabelanger) 2011-10-19 10:03:51.781-0500

Might be worth getting this up to reviewboard and starting some discussions about it.
By: Walter Doekes (wdoekes) 2011-10-19 12:31:48.331-0500

Ok. https://reviewboard.asterisk.org/r/1533/
By: Leif Madsen (lmadsen) 2011-11-07 10:24:08.111-0600

After a discussion with some developers, it was essentially agreed upon that this isn't really hurting anything, and could potentially break things. I've been asked to pass that along. Thanks!