[Home]

Summary:ASTERISK-04991: [patch] dialog matching bug (pedantic mode)
Reporter:klaus3000 (klaus3000)Labels:
Date Opened:2005-09-05 11:03:14Date Closed:2008-01-15 15:50:50.000-0600
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) asterisk-log.txt
( 1) asterisk-shell.txt
( 2) dialog.txt
( 3) pedantic_reject.txt
( 4) pedantictags.txt
( 5) pedantictags2.txt
( 6) pedantictags3.txt
( 7) pedantictags4.txt
( 8) pedantictags5.txt
Description:Asterisk still suffers an RFC conform dialog matching.
RFC3261:
 "...A dialog is identified at each UA with a dialog ID,
  which consists of a Call-ID value, a local tag and
  a remote tag...."

Even if pedantic=yes, Asterisk does not care about to-tags. This is a security issue and should be fixed.

Furthermore, if Asterisk does not find a call to an incoming request, it automatically generates a new dialog, even if the incoming request already contains a to-tag (this means that it is an in-dialog request).

If you want an example why this is a bad behaviour, here we go:
Many VoIP setups use "ser" in front of asterisk for user authentication and call routing because of the higher troughput (ser can be much faster than asterisk as it is only transaction statefull). ser will authenticate the SIP users and Asterisk will trust all packets received from ser.

As ser is only call statefull, in-dialog requests can't be associated to the inital request. Thus, ser just "loose routes" the in-dialog requests as they can't do any harm if the UAS (asterisk) handles them correctly. Thus, incoming requests with a to-tag which can't be matched to an existing call should be rejected with "481 Call/Transaction Does Not Exist".


****** ADDITIONAL INFORMATION ******

I often read the argument that RFC conform dialog matching is bad as it does not work with all clients. If this is the case, the clients should be fixed.
Comments:By: Olle Johansson (oej) 2005-09-05 11:05:34

I agree that we need to improve pedantic mode (and remove it in the future). As per the bug guidelines, can you please upload a SIP debug that shows a case where you have these problems?
By: klaus3000 (klaus3000) 2005-09-05 11:46:03

I uploaded an ngrep which shows both problems (in the following scenario I use sipsak to send an INVITE, and after the call is established a BYE to asterisk. pedantic=yes).

1. Asterisk accepts the INVITE from SIPSAK, although there is already a to-tag

2. Asterisk accepts the BYE (and hang up the previously established call), although the to-tag is wrong.
By: Olle Johansson (oej) 2005-09-05 12:51:35

The reason that we ask for a SIP debug output from Asterisk is that we want to see the logging from Asterisk in addition to the packets. Please produce a SIP debug output with debug and verbose set to 4. make sure that the debug log channel is activated.

Thank you.
By: klaus3000 (klaus3000) 2005-09-06 04:13:24

Files added:
 dialog.txt          ngrep capture of the SIP packets
 asterisk-log.txt    debug log from /var/log/asterisk
 asterisk-shell.txt  Asterisk prompt output
By: Olle Johansson (oej) 2005-09-06 04:28:24

Thanks Klaus, I'll look at this during the afternoon.
By: Olle Johansson (oej) 2005-09-06 11:26:49

Klaus, please try this patch. If you set debug to 4 it will start talking quite a lot. I have tried it in normal transactions, but could not force your type of error. Please see if this makes it better or worse :-)
By: klaus3000 (klaus3000) 2005-09-06 17:18:30

Hi Olle!

I have not tested your patch yet - I will do it tomorrow. But I reviewed the caode and have some questions:

+if (req->method == SIP_RESPONSE && !totag) { /* Responses need to-tags also */
+ badsyntax = 1;

Some responses need not to have a to-tag (e.g. 100 Trying)

Further, if there is an to-tag, but no existing call, the call is not rejected with "481 ...".
By: Olle Johansson (oej) 2005-09-07 01:01:37

I was trying to find info on the requirement of the to-tag, thanks.

We can't really send a 481 in this routine, it has to be handled in other places later in the code. So we have to create a pvt and let the handle_request function do it's magic. That's why I wanted the test.
By: klaus3000 (klaus3000) 2005-09-07 04:36:30

For incoming REQUESTS I think the dialog detection works. Although there is a funny thing at the moment: Asterisk accepts the INVITE with to-tag and creats a new sipcall (sip_alloc). But in sip_alloc, the local tag is generated randomly, although there is already a to-tag in the request. This has to be considerd when generating the 481 response.

For incoming responses, I'm not sure how asterisk handle these at the moment. What if Asterisk receives multiple responses (e.g a forked call) with different to-tags? Does it create an early dialog for each call leg as suggessted in RFC3261? Can it handle multiple 200 OK in case of both phones are picked up at the same moment? All this issues has to be considered when verifying the to-tag of incoming responses.
By: Olle Johansson (oej) 2005-09-07 04:46:17

The pedantic mode is not very well thought of yet. We will have to rewrite all of that soon... I'll look at this soon. I don't think we support forking very well today, but I have to test it with my SER.
By: klaus3000 (klaus3000) 2005-09-07 05:05:58

Hi Olle!

I modified your patch with your idea of adding a marker to the pvt structure which triggers the 481. At first glance it works. But I'm not sure if it is correct to set the SIP_NEEDDESTROY after replying,
   transmit_response(p, "481 Call/Transaction Does Not Exist", req);
   ast_set_flag(p, SIP_NEEDDESTROY);
as there will be an ACK to the 481 which must be handled.
By: Olle Johansson (oej) 2005-09-07 05:10:41

We don't like expanding sip_pvt, we use the flags. But I see what you're doing and will fix that. Thanks! Pleasure to be working with someone that knows SIP :-)
By: Olle Johansson (oej) 2005-09-07 12:12:53

Try this, I hope I haven't missed stuff... At least it makes it a bit better, though I can't try sending broken packets...
By: klaus3000 (klaus3000) 2005-09-08 06:42:25

Hi Olle!

Comments inline!

+if (pedanticsipchecking) {
+  /* If this is a request packet without a from tag, it's not
+    correct according to RFC 3261  */
+  if (!ast_test_flag(req, SIP_PKT_WITH_FROMTAG)) {
+    /* No from tag on request? Don't bother with it */
+    if (!ignore && req->method == SIP_INVITE) {
+      transmit_response_reliable(p, "400 Bad Request", req, 1);
+      /* Will cease to exist after ACK */
+    } else {
+      transmit_response(p, "400 Bad Request", req);
+      ast_set_flag(p, SIP_NEEDDESTROY);
+    }

return res; ??????????

+  }

I think this is a little bit to strict! It also rejects REGISTER messages from Cisco Phones.


+
+ /* Check if this a new request in a new dialog with a totag already attached to it,
+ if it is, and it is not matched to a current call, forget it.
+ RFC 3261 - section 12.2 - and we don't want to mess with recovery  */
+ if (!p->initreq.headers && ast_test_flag(req, SIP_PKT_WITH_TOTAG)) {
+ /* If this is a first request and it got a to-tag, it is not for us */
+ if (!ignore && req->method == SIP_INVITE) {
+ transmit_response_reliable(p, "481 Call/Transaction Does Not Exist", req, 1);
+ /* Will cease to exist after ACK */
+ } else {
+ transmit_response(p, "481 Call/Transaction Does Not Exist", req);
+ ast_set_flag(p, SIP_NEEDDESTROY);
+ }

return res; ??????????

+ }
By: Olle Johansson (oej) 2005-09-08 07:45:59

So REGISTER can work without from-tag? Or is it just Cisco? What kind of rule do we need to implement here to be enough strict but not wide open?

Yeah, return res. :-)
By: klaus3000 (klaus3000) 2005-09-08 08:34:36

RFC2543 (old):
from-tag: the UAC may add a from-tag
to-tag: the UAS must add a to-tag if the request contained more than one Via header field.

RFC3261:
from-tag: UAC: The From field MUST contain a new "tag" parameter
to-tag: must

Thus, to be compatible with RFC2543 phones (e.g Cisco), we should allow incoming requests without from-tag
By: Olle Johansson (oej) 2005-09-08 10:49:05

So that check is not really needed then I guess... It's not easy with all these RFCs... :-)
By: klaus3000 (klaus3000) 2005-09-08 11:37:34

I agree - we should remove the check for the from-tag
By: Olle Johansson (oej) 2005-09-08 14:20:21

Try this version. I removed a lot of the debugging stuff.
By: Olle Johansson (oej) 2005-09-09 08:27:20

Klaus - any test results? Do you agree with the latest patch?
By: klaus3000 (klaus3000) 2005-09-09 08:36:50

Hi Olle!

I didn't tested it as I thought you might release another one with a bugfix for vonage.

I'm leaving now and will return after weekend. Thus, tests ahve to wait till monday.

I think the "return res;" after sending 481 is missing again.
By: Olle Johansson (oej) 2005-09-09 09:52:54

Sure was. Bad. And yes, I just now decided to update to a combined patch.
By: Olle Johansson (oej) 2005-09-09 09:57:30

Arrrggghhh. Now our INVITE matching does not match anymore, which is correct but breaks our auth scheme... Need a thinking cap.
By: Olle Johansson (oej) 2005-09-09 10:46:10

Latest patch also tries to solve ASTERISK-5030
By: Kevin P. Fleming (kpfleming) 2005-09-13 18:24:09

This looks like a reasonable patch, except the weird stuff in get_tag when 'tagbuf' is not supplied. As best I can tell, it allocates some space on the stack, copies the tag into it, trims off the end, then returns a pointer to the original string (ignoring the copy it just made).

If it's supposed to work this way (no existing callers use this mode), it will need to be fixed to do something useful... and keep in mind that you cannot return a pointer to alloca() memory space anyway (and you should use ast_strdupa() instead, if possible).

Once this patch is in I'm going to change the ->tag member of sip_pvt to be an already-formatted string instead of having to format it every place we use it, since we _never_ need it in its numeric form, and we never parse any incoming tags into any sort of numeric form.
By: Olle Johansson (oej) 2005-09-14 01:38:46

I'll look into your issues. Also noted the stupid tag but did not want to change that already, but since you insist...
By: Kevin P. Fleming (kpfleming) 2005-09-29 00:18:24

I've already done the conversion to store the Asterisk-end tag in text form in the private structure, so that should make things ever-so-slightly easier for you :-)
By: Olle Johansson (oej) 2005-10-11 23:23:41

Olle, please wake up!
By: Olle Johansson (oej) 2005-10-13 17:58:34

New patch, removed the alloc in gettag()
By: Kevin P. Fleming (kpfleming) 2005-10-13 18:09:25

Committed to CVS HEAD, thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:50:50.000-0600

Repository: asterisk
Revision: 6767

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r6767 | kpfleming | 2008-01-15 15:50:50 -0600 (Tue, 15 Jan 2008) | 2 lines

clean up pedantic mode tag handling (issue ASTERISK-4991)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6767