[asterisk-dev] dialog matching

[Klaus Darilion]

Hi Steve!

pedantic in Asterisk is buggy - regardless if it is turned on or off.

Thus, please do not just reimplement the old behavior but fix it.

AFAIK pedantic controls 2 things:
  1. accept short header names and header which are wrapped around 
multiple lines if pedantic=on
  2. use to-tag for dialog matching if pedantic=on.

IMO part 2 should be removed from pedantic setting and dialog matching 
should be done standard conform.

Current problems with dialog matching with pedantic=off: Asterisk 
accepts an to-tag. Thus for example a reINVITe will cause a new call if 
Asterisk can not find an existing call -> bad security

Current problems with dialog matching with pedantic=on: Asterisk learns 
the totag from the first provisional response and ignores responses with 
different to-tag although they belong to the same transaction -> causes 
problems if the outgoing SIP call is forked and causes multiple 
early-dialog.

The fix would be to always use RFC conform dialog matching - meaning 
call-id/fromtag/totag for indialog requests and accept multiple 
provisional responses (I know asterisk can not handle multiple early 
dialogs but a reasonable workaround would be to learn the totag only 
from the final response, not from the provisional response and treat all 
early dialog as one early dialog).

regards
klaus




Steve Murphy schrieb:
> Olle--
> 
> I thought to double check with you... the price of missing something is 
> pretty high.
> 
> Put on your thinking cap, pull out some paper and a pencil, turn off the 
> cell phone... this is going to require
> some thought!
> 
> 
> For reference, here's the search loop in find_call:
> 
> 
>         dialoglist_lock();
>         for (p = dialoglist; p; p = p->next) {
>                 /* In pedantic, we do not want packets with bad syntax 
> to be connected to a PVT */
>                 int found = FALSE;
>                 if (ast_strlen_zero(p->callid))
>                         continue;
>                 if (req->method == SIP_REGISTER)
>                         found = (!strcmp(p->callid, callid));
>                 else
>                         found = (!strcmp(p->callid, callid) &&
>                            (!pedanticsipchecking || !tag || 
> ast_strlen_zero(p->theirtag) || !strcmp(p->theirtag, tag))) ;
> 
>                 ast_debug(5, "= %s Their Call ID: %s Their Tag %s Our 
> tag: %s\n",
>                                                  found ? &qu ot;Found" : 
> "No match", p->callid, p->theirtag, p->tag);
> 
>                 /* If we get a new request within an existing to-tag - 
> check the to tag as well */
>                 if (pedanticsipchecking && found  && req->method != 
> SIP_RESPONSE) {     /* SIP Request */
>                         if (p->tag[0] == '\0' && totag[0]) {
>                                 /* We have no to tag, but they have. 
> Wrong dialog */
>                                 found = FALSE;
>                         } else if (totag[0]) {                  /* Both 
> have tags, compare them */
>                                 if (strcmp(totag, p->tag)) {
>                                         found = FALSE;          /* This 
> is not our packet */
>                                 }
>                         }
>                         if (!found)
>                                 ast_debug(5, "= Being pedantic: This ... 
> request: Call ID: %s Ourtag <null> Totag %s Method %s\n",
>                                        p->callid, totag, 
> sip_methods[req->method].text);
>                 }
> 
> 
>                 if (found) {
>                         /* Found the call */
>                         sip_pvt_lock(p);
>                         dialoglist_unlock();
>                         return p;
>                 }
>         }
>         dialoglist_unlock();
> 
> The object: remove the loop, and replace it with astobj2 hash table lookups.
> 
> I checked Luigi's astobj2 branch, where he put dialogs in a table, but 
> he implements the above as a linear traversal, which is not my goal. I 
> want to improve performance, not just convert the code to be astobj2 
> based...
> 
> There seem to be two main modes: with pedanticsipchecking ON, and 
> pedanticsipchecking OFF.
> ------------------------------------------------------------------------
> 
> 
> 
> Things are simple in OFF mode:
> 
> fundamentally, whether req->method is SIP_REGISTER or not, found is true 
> when a dialog's callid matches.
> No code after setting found could disqualify this choice.
> 
> ------------------------------------------------------------------------
> 
> 
> 
> Things get more complex in pedanticsipchecking ON mode:
> 
> It seems split into whether req->method is SIP_REGISTER/SIP_RESPONSE  or 
> not:
> 
> If req->method is SIP_REGISTER:
> 
>    if match(p->callid,callid)  AND (!totag[0] ||  (p->tag[0] && 
> match(p->tag,totag))
> 
> else
> 
>     If req->method is SIP_RESPONSE:
> 
>             If callid matches AND ( empty(p->theirtag)  || 
> match(p->theirtag,tag))
> 
>     else
> 
>             If callid matches AND  (!totag[0] ||  (p->tag[0] && 
> match(p->tag,totag))
>                                         AND ( empty(p->theirtag)  || 
> match(p->theirtag,tag)) 
> 
> (I notice in the code that there is :     ... ||  !tag  || ...   which 
> will never be true, and can be dropped)
> 
> ------------------------------------------------------------------------
> 
> 
> I'm finding that coding the above via hashtable lookups to be 
> mind-bending. I've got ideas as how to handle it, and I'm "hashing" it 
> out, (pardon the really, really bad pun there).
> 
> What I'd like to do, is verify that my refactoring of the logic  above 
> is indeed correct, and some explanation as to how dialogs can be created 
> without unique callid's and when such are created, how the other fields 
> are affected, namely, the from/to tag/headers as they are recorded in 
> the dialog structures... Can you help me? I'm hoping there may be a 
> simplifying assumption or two that will make things easier.
> 
> Many thanks,
> 
> murf
> 
> 
> -- 
> Steve Murphy
> Software Developer
> Digium
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> --Bandwidth and Colocation Provided by http://www.api-digital.com--
> 
> asterisk-dev mailing list
> To UNSUBSCRIBE or update options visit:
>    http://lists.digium.com/mailman/listinfo/asterisk-dev