[Home]

Summary:ASTERISK-03583: [patch - bugfix] False REGISTER denials with unreliable transport.
Reporter:constfilin (constfilin)Labels:
Date Opened:2005-02-24 23:51:49.000-0600Date Closed:2008-01-15 15:42:45.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) astlog.log
( 1) chan_sip.c
( 2) regpedantic.txt
( 3) sipsak.log
( 4) regped0610.txt
( 5) regped0721.txt
Description:Greetings -

I have seen this on the chan_sip.c

1. A telephone sends a REGISTER request with correct
  authentication
2. * receives it and sends a successful reply
3. The reply gets lost and Telephone re-sends the
  REGISTER request
4. * receives the second request, finds out that it
  already replied on it (using the incoming sequence
  number) and function "check_auth" sends a "401
  Unauthorized" reply without bothering to look into it
  again. The code excerpt is below

/* Resend message if this was NOT a reliable delivery.   Otherwise the retransmission should get it */
transmit_response_with_auth(p, response, req, randdata, reliable, respheader);

I can go ahead and fix the problem but I need to know
why sending of "410 Unauthorized" on ignored request was
there in the first place.

Does anybody understand this piece of code?
Comments:By: Clod Patry (junky) 2005-02-24 23:54:31.000-0600

can you attach a sip debug?
Thanks.
By: Mark Spencer (markster) 2005-02-25 12:40:04.000-0600

That line is being executed only when either randdata is empty (we haven't produced random data for authentication yet) or authtoken is empty (they aren't sending us valid authentication info).  Have you confirmed that's the path that is being executed?
By: constfilin (constfilin) 2005-02-25 12:54:57.000-0600

Mark, there is this code in check_auth:

if (ignore && !ast_strlen_zero(randdata) && ast_strlen_zero(authtoken)) {
 /* This is a retransmitted invite/register/etc, don't reconstruct
    authentication information */
 if (!ast_strlen_zero(randdata)) {
    if (!reliable) {
       /* Resend message if this was NOT a reliable delivery.   Otherwise the
          retransmission should get it */
       transmit_response_with_auth(p, response, req, randdata, reliable,
       respheader);
       /* Schedule auto destroy in 15 seconds */
       sip_scheddestroy(p, 15000);
    }
    res = 1;
 }
}

As you can see the line is executed when radndata is not empty *AND* authtoken is empty. Moreover, the next "if" in the code does not make sense because it
is always true.

Is it just a bug in the code?

Thanks

-c
By: Mark Spencer (markster) 2005-02-25 22:32:18.000-0600

If it's a bug, then I don't see it yet.  If this is a duplicate message and (a) We have already sent random data and (b) this duplicate message does not include an authtoken, then we send the authtoken again.  Otherwise, if we have no data, and no auth token was specified (ignore is not important in this case), then we create one.  Otherwise, we take the provided authtoken and check it against our expectation.  I think we need more information on the actual path being taken when this fails in your environment.
By: Olle Johansson (oej) 2005-02-26 01:37:52.000-0600

constfilin, I would really appreciate a SIP debug with high verbosity in order to dig down into this. I think I've seen similar things happen, but can't repeat it. If you can, I would like to see what it is.
By: constfilin (constfilin) 2005-02-26 12:30:08.000-0600

I will try to produce a SIP debug output, but - meanwhile - can
you just take a look at the code and make sense of it. For example,

WHY DOES CODE HAVE THE SECOND if (!ast_strlen_zero(randdata)) {
CONDITION IF THE EXACT SAME CONDITION IS CHECKED IN THE LINE JUST
ABOVE IT?

In other words - there is certainly something not right about the code,
and I will produce the SIP trace when I can.
By: constfilin (constfilin) 2005-02-27 12:08:48.000-0600

Hello, I uploaded 2 files - one is the log of what asterisk sent to sipsak testing tool and the other what sipsak saw. The asterisk log also has the line number of verbose file, please ignore those. I do not understand why in astlog.log there are no incoming packets from sipsak - probably because of NAT and I am working with sysadmins to figrue this out.

astlog.log also contains traces of where the code have gone. The traces are based on the attached customer version of chan_sip.c.

I hope this helps.
By: Olle Johansson (oej) 2005-02-27 12:21:22.000-0600

Ok, will take a look soon, thanks.
By: constfilin (constfilin) 2005-03-03 21:02:52.000-0600

any news on this?

Thank you

-c
By: Olle Johansson (oej) 2005-03-19 16:02:21.000-0600

Have the same problem now with a WiSIP phone :-(
By: Olle Johansson (oej) 2005-03-19 16:14:44.000-0600

Agree, the code in that part is strange, maybe it should be an || instead of an &&.

Anyway, are you sure you have a packet marked "ignore"?
By: Olle Johansson (oej) 2005-03-19 16:17:51.000-0600

According to your trace, that I now found in your modified chan_sip, you are not within that part of the code, your in the code below that like me. The randdata is gone for some reason.
By: Olle Johansson (oej) 2005-03-19 16:32:57.000-0600

Do you have "pedantic" turned on?
By: Olle Johansson (oej) 2005-03-19 16:36:30.000-0600

Turns out that my WiSIP changes the From: tag on the second REGISTER, the one with the auth. With pedantic turned on, we match on tags as well and did not match with the first one, thus no auth randdata being found - every REGISTER was a *new* one.

Back to the RFC to check how this should work, if the WISIP is doing the right thing.
By: khb (khb) 2005-03-19 17:30:44.000-0600

If I recall this, we should not match on tags on the second register,
I added a fix along time ago to prevent that by resetting "theirtag" and generating a new local tag on every register.
Since there is no dialog here, this seem still the correct behavior.
I can't find the issue number.
By: Olle Johansson (oej) 2005-03-19 18:14:53.000-0600

Test if this patch to CVS head will solve it for you.
It solves my problem - regardless of the setting of pedantic.

Basically, I move the parsing of the headers and the method to earlier in the process of receiving a SIP packet. We will parse it anyway. With this data, I can do matching of request to existing dialogs in different ways depending on the dialog method.

Also, changed the name of function "parse" to "parse_request" to simplify searching through the source code for it. parse_request is a better name.

Disclaimer on file.

Constfilin: Please test and confirm if this helps you. This will require CVS head, not your old custom chan_sip, sorry. I do hope you can test. It took quite a while to figure this one out.
By: Olle Johansson (oej) 2005-03-19 18:17:04.000-0600

khb: Right, the new REGISTER is a non-related transaction, so there should be no matching really... We should propably look into handling the challenges in another way with timeouts and being able to re-authenticate with a current set of credentials and re-use the previous challenge for a while. But that is another patch, another day.
By: Olle Johansson (oej) 2005-03-20 14:49:36.000-0600

Constfilin: Please test this patch.
By: Olle Johansson (oej) 2005-03-22 01:09:54.000-0600

Hello??? Did the patch solve your problem?
By: Olle Johansson (oej) 2005-03-23 10:53:28.000-0600

kpfleming found a bug, will resubmit patch later.
By: Olle Johansson (oej) 2005-03-23 13:58:44.000-0600

Updated and fixed.
By: Olle Johansson (oej) 2005-03-31 13:29:11.000-0600

I still think this is a good patch, even though the bug report submitter has lost interest in it.
By: Mark Spencer (markster) 2005-04-07 13:38:38

It sounds like this is ready for code review and then merger, can we get anthm and/or kpfleming to do code review and get this merged?
By: Leif Madsen (lmadsen) 2005-04-07 13:42:17

oej: Does this patch fix 3923? When you were talking, you said you had a fix for when pedantic=yes, but the latest date is for after I reported the bug. If you've got a fix for it which was updated in this patch and can upload it, I can test it. If you've fixed my issue in 3923 with this bug, then let me know and I'll close the other one out as a duplicate and refer to this one.
Thanks.
By: Olle Johansson (oej) 2005-05-01 15:21:46

No, this doesn't solve your issue blitzrage. One small step at a time to make it a bit more simple for code reviewers... :-)

kpfleming: Please look at this, it will help in fixing the RTP allocation issue in another bug report properly.
By: Olle Johansson (oej) 2005-06-04 16:22:18

oej: Take another look at this now that chan_sip changed a lot :-)
By: Olle Johansson (oej) 2005-06-10 15:54:06

New patch uploaded. We need this in CVS to move along. Disclaimer on file as always.
By: Michael Jerris (mikej) 2005-06-23 06:14:13

Can we get a review for this one please.
By: adomjan (adomjan) 2005-06-27 05:59:31

Thanks the patch!
It fixed the pedantic=yes + sjphone registration problem.
By: Olle Johansson (oej) 2005-07-20 12:45:41

Kevin, if I update this patch again, will you consider it for CVS? CVS is broken for registrations with pedantic=yes ...
By: Olle Johansson (oej) 2005-07-21 15:33:21

Updated the patch, still waiting for CVS. Four months old, this patch was the other day!
By: Kevin P. Fleming (kpfleming) 2005-07-25 19:20:22

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

Repository: asterisk
Revision: 6218

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r6218 | kpfleming | 2008-01-15 15:42:44 -0600 (Tue, 15 Jan 2008) | 2 lines

clean up SIP response and pedantic REGISTER matching (bug ASTERISK-3583)

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

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