[asterisk-dev] [Code Review] Don't re-auth subscriptions that are just re-transmissions (not re-subscriptions)

[reviewboard at asterisk.org]

> On 2010-11-09 12:33:45, Olle E Johansson wrote:
> > I think it's very dangerous to disable the verification of the auth. There has to be another way of fixing this. The retransmit should hit the same dialog and we still have the nonce. Why does the auth fail?
> 
> David Vossel wrote:
>     The auth fails because the nonce can only be responded to once...  When a retransmission comes in check_auth() sees that the nonce has already been responded to and issues a new challenge. I don't think this is correct behavior.  In the case of a retransmit the current nonce probably shouldn't be treated as a stale nonce.
> 
> Terry Wilson wrote:
>     We do the same thing in handle_request_invite.
>     
>         if (!p->lastinvite && !ast_test_flag(req, SIP_PKT_IGNORE) && !p->owner) {
>             /* This is a new invite */
>             /* Handle authentication if this is our first invite */
>             res = check_user(p, req, SIP_INVITE, e, XMIT_RELIABLE, sin);
>     
>     It is only disabling verification for retransmissions (unless I have messed up somewhere) and I checked that retransmissions that weren't authed aren't accepted. 
>     
>     It fails the verification with "Duplicate authentication received from ..." because the auth request has already been responded to and we treat the nonce as stale. We then re-generate a 401, which is what breaks the polycom implementation.
> 
> Klaus Darilion wrote:
>     I agree disabling authentication sounds weird. The problem is on the transaction layer and thus the only clean solution would be to add a transaction layer - and all these problems will disappear.
> 
> Olle E Johansson wrote:
>     Then I think we should fix the way we handle all retransmission of an authenticated message, regardless of type. We should not bypass verification of the auth at all. If we do, we need to consider if that's a security issue.
> 
> Olle E Johansson wrote:
>     Klaus - yes, that is the religous correct issue but that's going to be very hard. We need a simple fix for 1.4 and that's not to rewrite the stack. But it's not to disable authentication either.
> 
> Terry Wilson wrote:
>     Klaus: Yes, having a transaction layer in chan_sip would be nice. Keep dreaming. :-)
>     Olle: Again, the method I used has been used for INVITES (which are far more dangerous than SUBSCRIBEs) for ages. That siad, I can "fix" the auth code by allowing the nonce to be re-used in the case of retransmitted packets, but I worry that it might open us up to some kind of replay attack because of some assumption buried in the code somewhere else. In fact, the patch is fairly simple. The important part is:
>     
>     Index: channels/chan_sip.c
>     ===================================================================
>     --- channels/chan_sip.c	(revision 294499)
>     +++ channels/chan_sip.c	(working copy)
>     @@ -9223,7 +9223,7 @@
>      
>      	/* Verify nonce from request matches our nonce, and the nonce has not already been responded to.
>      	 * If this check fails, send 401 with new nonce */
>     -	if (strcasecmp(p->randdata, keys[K_NONCE].s) || p->stalenonce) { /* XXX it was 'n'casecmp ? */
>     +	if (strcasecmp(p->randdata, keys[K_NONCE].s) || (!ignore && p->stalenonce)) { /* XXX it was 'n'casecmp ? */
>      		wrongnonce = TRUE;
>      		usednonce = keys[K_NONCE].s;
>      	} else {
>     
>     and it solves the issue--it just scares me a bit. Opinions?

As all code properly handles SIP_PKT_IGNOREd packets, this should be safe. I'm not sure how big an IF that is. I've tried breaking it with INVITE and SUBSCRIBE w/o much luck.


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/1005/#review2897
-----------------------------------------------------------


On 2010-11-09 12:21:48, Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1005/
> -----------------------------------------------------------
> 
> (Updated 2010-11-09 12:21:48)
> 
> 
> Review request for Asterisk Developers and David Vossel.
> 
> 
> Summary
> -------
> 
> From the issue:
> 0018075: Asterisk fails to recognize SUBSCRIBE retransmissions and tries to re-authenticate them, which breaks presence on polycom phones
> Description	 Here's what happens from asterisk point of view:
> -> SUBSCRIBE
> <- 401 Unauthorized
> -> SUBSCRIBE with auth
> <- 200 OK (lost)
> <- NOTIFY
> -> 200 OK for NOTIFY
> -> SUBSCRIBE with auth retransmission
> <- 401 Unauthorized
> At this point polycom phone received 2 "401 Unauthorized" in a row and it completely gives up on resubscribing to this hint until reboot. While polycom behavior is questionable, I believe asterisk should recognize retransmission and resubmit 200OK in this case
> 
> 
> This addresses bug 18075.
>     https://issues.asterisk.org/view.php?id=18075
> 
> 
> Diffs
> -----
> 
>   /branches/1.4/channels/chan_sip.c 294120 
> 
> Diff: https://reviewboard.asterisk.org/r/1005/diff
> 
> 
> Testing
> -------
> 
> I created a sipp scenario mimicking the issue, it now re-sends a 200 OK when an authed retransmission occurs. I also tested a scenario that sent a retransmission without authentication to ensure that we didn't accept un-authed retransmissions.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.digium.com/pipermail/asterisk-dev/attachments/20101110/f3d6eb52/attachment-0001.htm