[Home]

Summary:ASTERISK-21013: Security Vulnerability: sip username disclosure
Reporter:Walter Doekes (wdoekes)Labels:
Date Opened:2013-01-30 06:00:58.000-0600Date Closed:2013-03-27 10:54:22
Priority:MajorRegression?
Status:Closed/CompleteComponents:Channels/chan_sip/General
Versions:11.2.1 Frequency of
Occurrence
Related
Issues:
must be completed before resolvingASTERISK-21004 Open Blockers for 1.8.21.0
must be completed before resolvingASTERISK-21005 Open Blockers for 11.3.0
Environment:Attachments:( 0) AST-2013-003-1.8.diff
( 1) AST-2013-003-10.diff
( 2) AST-2013-003-11.diff
( 3) ASTERISK-21013.diff
( 4) ASTERISK-21013.diff
( 5) ASTERISK-21013.diff
( 6) invite-username-disclosure-1.xml
( 7) invite-username-disclosure-1.xml
( 8) invite-username-disclosure-2.xml
( 9) invite-username-disclosure-2.xml
(10) invite-username-disclosure-3.xml
(11) invite-username-disclosure-3.xml
(12) issueA21013_better_but_not_there_yet.patch
(13) issueA21013_bogopeer_still_needs_alwaysauthreject_cleanup.patch
(14) issueA21013_more_cleanup_more_fixes.patch
(15) issueA21013_with_null_check.patch
(16) register-username-disclosure.xml
(17) register-username-disclosure.xml
(18) register-username-disclosure-2.xml
Description:So.. I was trying if I could alter the SIP security framework messages to differentiate between auth failures for any UDP packet and those with a valid nonce. Those with a valid nonce would probably not have a spoofed IP, so I can use fail2ban on them with more peace of mind.

But, then I saw the different handling of the alwaysauthreject-challenge and the "normal" challenge code. These differences can be observed by an attacker sniffing for valid usernames.

{noformat}
VICTIM$ sudo asterisk -nrx 'sip show peers' | head -n4
Name/username...
100...
101...
102...

VICTIM$ sudo asterisk -nrx 'core show version'
Asterisk SVN-branch-11-r380384M
{noformat}

{noformat}
ATTACKER$ sipp -m 1 -sf register-username-disclosure.xml VICTIM -s 000 -ap badpass >/dev/null
000 is NOT a valid username
ATTACKER$ sipp -m 1 -sf register-username-disclosure.xml VICTIM -s 001 -ap badpass >/dev/null
001 is NOT a valid username
ATTACKER$ sipp -m 1 -sf register-username-disclosure.xml VICTIM -s 100 -ap badpass >/dev/null
100 is a valid username
ATTACKER$ sipp -m 1 -sf register-username-disclosure.xml VICTIM -s 101 -ap badpass >/dev/null
101 is a valid username
{noformat}

I haven't done any work on fixing the issue. But it's likely that the right fix would be to follow the normal challenge code path as much as possible.

Regards,
Walter Doekes
OSSO B.V.
(my employer wouldn't mind if OSSO B.V. is mentioned in a security bulletin if that were to be produced)
Comments:By: Kinsey Moore (kmoore) 2013-02-04 14:34:51.498-0600

Would you mind attaching the sip.conf you used with this SIPp scenario?  I know it's likely to be trivial, but I want to ensure I have exactly the same setup for testing.

By: Walter Doekes (wdoekes) 2013-02-04 14:52:35.313-0600

{noformat}
$ cat /etc/asterisk/sip.conf
[user](!)
type=friend
host=dynamic
secret=notemptysecret
[alice](user)
[bob](user)

$ sipp -m 1 -sf register-username-disclosure.xml THE_HOST -s alice >/dev/null
alice is a valid username

$ sipp -m 1 -sf register-username-disclosure.xml THE_HOST -s kirk >/dev/null
kirk is NOT a valid username
{noformat}

By: Walter Doekes (wdoekes) 2013-02-04 15:12:52.032-0600

P.S. I retract my "making the fake path follow the normal one". A 403 should be the right response after the retransmission of the 2nd CSeq 2 REGISTER, not a 401 like the valid user now gets.

By: Kinsey Moore (kmoore) 2013-02-05 09:56:22.297-0600

Could you give the attached patch a spin? It corrects the issue demonstrated by the test provided.

By: Kinsey Moore (kmoore) 2013-02-05 16:05:30.848-0600

Uploaded new version of the diff.  This is a little more narrow in scope and showed no issues in the SIP tests I ran on it from the testsuite while still passing the test case you provided.

By: Walter Doekes (wdoekes) 2013-02-06 02:33:02.769-0600

Your patch fixes the issue for the register. But continuing, I see issues for the invite.

* register-username-disclosure.xml different handling for destruction of authenticated register transaction tuple (retransmission of 2nd register yields 401 when user exists instead of 403)
* invite-username-disclosure-1.xml different www response code (401 when user exists, 407 when user doesn't)
* invite-username-disclosure-2.xml different 403 message (Forbidden when user exists, Forbidden Bad Auth when user doesn't)
* invite-username-disclosure-3.xml different handling of destruction again, but this time for invites

Let's allowguest=no, so we can check.
{noformat}
VICTIM# cat /etc/asterisk/sip.conf
[general]
allowguest=no

[user](!)
type=friend
host=dynamic
secret=notemptysecret

[alice](user)
[bob](user)
{noformat}

Before patch:
{noformat}
$ for scenario in register-username-disclosure.xml invite-username-disclosure-{1,2,3}.xml
   do for username in alice bob kirk alice
   do sipp -m 1 -sf $scenario $VICTIM -s $username 3>&1 >/dev/null 2>&1 | sed -e s/^/$scenario:\ /
 done; echo; done
register-username-disclosure.xml: alice is a valid username
register-username-disclosure.xml: bob is a valid username
register-username-disclosure.xml: kirk is NOT a valid username
register-username-disclosure.xml: alice is a valid username

invite-username-disclosure-1.xml: alice is a valid username
invite-username-disclosure-1.xml: bob is a valid username
invite-username-disclosure-1.xml: kirk is NOT a valid username
invite-username-disclosure-1.xml: alice is a valid username

invite-username-disclosure-2.xml: alice is a valid username
invite-username-disclosure-2.xml: bob is a valid username
invite-username-disclosure-2.xml: kirk is NOT a valid username
invite-username-disclosure-2.xml: alice is a valid username

invite-username-disclosure-3.xml: alice is a valid username
invite-username-disclosure-3.xml: bob is a valid username
invite-username-disclosure-3.xml: kirk is NOT a valid username
invite-username-disclosure-3.xml: alice is a valid username
{noformat}
After patch, the first issue is fixed indeed:
{noformat}
$ for..loop..like..above
register-username-disclosure.xml: alice is NOT a valid username
register-username-disclosure.xml: bob is NOT a valid username
register-username-disclosure.xml: kirk is NOT a valid username
register-username-disclosure.xml: alice is NOT a valid username

...unchanged...
{noformat}

I'm guessing we can adapt the scenarios to use MESSAGE and SUBSCRIBE requests as well.

By: Walter Doekes (wdoekes) 2013-02-06 03:31:40.247-0600

In conclusion: the fact that there is a "fake" response, makes this happen. Perhaps a quickfix could make use of a "bogopeer" which always has the wrong password/acl. Then we would never hit the fake response generation and the code paths for existing and nonexistent users would always be equal.

By: Kinsey Moore (kmoore) 2013-02-06 14:14:48.375-0600

The version of the patch I just uploaded seems to handle the additional test cases with the one change.  I'm still looking into your suggestion on an alternative fix.

By: Walter Doekes (wdoekes) 2013-02-07 02:12:36.072-0600

Yes the addional tests. But the register-username-disclosure now only shows kirk. (Because the others get a 407 instead of a 401.) Same for a SUBSCRIBE.

By: Walter Doekes (wdoekes) 2013-02-07 02:45:35.775-0600

In fact, only test invite-username test 1 and 2 were fixed. Test 3 gives errors which we don't see. It only appears fixed.

If I change this:
{noformat}
/* We have to emulate EXACTLY what we'd get with a good peer
* and a bad password, or else we leak information. */
- const char *response = "407 Proxy Authentication Required";
- const char *reqheader = "Proxy-Authorization";
- const char *respheader = "Proxy-Authenticate";
+ const char *response = "401 Unauthorized";
+ const char *reqheader = "Authorization";
+ const char *respheader = "WWW-Authenticate";
{noformat}

Things get better, but now invite tests 2 fails too. It gets "(Bad auth)" for the failed user only.

So.. start removing the alternative headers.
{noformat}
- transmit_response(p, "403 Forbidden (Bad auth)", &p->initreq);
+ transmit_response(p, "403 Forbidden", &p->initreq);
{noformat}

(You can now remove all optional 407 handling from the *xml.)

Now I get success on the first three, but now I still have failure on invite-username-disclosure-3.xml and a subscribe*.xml test shows me that unknown users get a 401 instead of a 403 after failed auth.

By: Walter Doekes (wdoekes) 2013-02-11 09:59:55.590-0600

Ok. I uploaded something with a bogus peer which should never authenticate:

issueA21013_bogopeer_still_needs_alwaysauthreject_cleanup.patch

I updated the XML scenarios that go with it. I left the old ones still available to test against unpatched asterisken (note that invite-username-disclosure-{2,3}.xml had bad CSeqs in the ACKs / was missing a final ACK).

(And yes.. some say now say that the user is or is not valid even when it is/not. The point is that the answers are the same.)


The patch could use some review as to wether it's 100% safe ;)
Further, we can now remove lots of if(alwaysauthreject) blocks and do that check in check_peer_ok only and return AUTH_DONT_KNOW if !alwaysauthreject.

By: Kinsey Moore (kmoore) 2013-02-11 14:13:36.144-0600

Hi Walter,
I just wanted to let you know that I'm still working on this.  I'm finding other information leaks along the way to reviewing your scenarios and patches mostly in the way of transmitting things reliably when they shouldn't be.

By: Kinsey Moore (kmoore) 2013-02-12 14:56:42.007-0600

Updated ASTERISK-21013.diff to merge your bogopeer changes and added some other changes that I noticed along the way.  I was seeing some weird behavior on some "not a real user" transactions vs "real user" transactions with regards to retransmission which could have given away details about users.

By: Walter Doekes (wdoekes) 2013-02-13 04:00:00.002-0600

I take your {{ASTERiSK-21013.diff}} and I raise you {{issueA21013_more_cleanup_more_fixes.patch}}.

(a) Don't rely on the MD5 hash not matching. You could use it to bypass auth. (Yes.. not timing attack safe.. but don't think the rest of the code qualifies either.)

(b) Fix sip_cfg.allowguest usage.

(c) Fix minor verbose log.

(d) Remove some sip_cfg.alwaysauthreject code which we won't reach anymore.

(e) Remove all AUTH_FAKE_AUTH occurrences.

(f) Make sure a global insecure=invite doesn't open up everything for everyone. NOTE: a sip reload where someone switches the default NAT type would now leave the bogopeer in the previous state. I didn't feel like fixing this.

(g) Fix so we're safe against {{register-username-disclosure-2.xml}}: using CSeq 3 now yielded different results for REGISTER.

Further:

(h) The CHANGES should probably mention that all auth requests now do WWW-Auth and never Proxy-Auth.

(i) The fact that there still exists calls to {{transmit_fake_auth_response}} suggests that not all potential avenues are closed.

(j) Someone should probably test that the guests (for invites) and temppeers (for registers) still work like they should.

By: Kinsey Moore (kmoore) 2013-02-13 15:00:01.680-0600

(f) Fixed by refreshing the bogopeer in sip_reload.

(h) This was apparently removed in 1.8 in relation to AST-2011-011 in r325279. It was not applied to trunk at that time.  It will have to go into 10+.

(i) This is now contained to register_verify which seems to be a combination of registration business logic and auth code.  I primarily just undid the changes to check_auth() and retested with the scenarios provided for register, invite, and subscribe.  They all passed as far as I could tell.  Removing this from register_verify is going to be too invasive for existing branches, so I'm leaving it as is since it is currently working and may attempt a refactor for trunk.

Did some cleanup in transmit_fake_auth_response since sipmethod is no longer necessary.  Changed bogopeer to bogus_peer.

By: Walter Doekes (wdoekes) 2013-02-13 16:14:27.750-0600

Yuck at reload() calling the cli sip_reload() instead of the other way around. But you didn't do that ;)

Ok. Your patch looks sane. I added the mandatory null-check for the sip reload in {{issueA21013_with_null_check.patch}}.

I quickly ran the tests against 11.x, and it all seems fixed. Better get a 3rd set of eyes on it and run tests of regular behaviour. Did you run any allowguest and autocreatepeer checks?

By: Kinsey Moore (kmoore) 2013-02-14 12:34:28.082-0600

The existing tests in the integration testsuite report no problems.  allowguest and autocreatepeer look good code-wise after a double check.  I have yet to run manual tests on those two settings, but I don't expect any issues to come up for them.  I'll get to those tests when I can.

By: Kinsey Moore (kmoore) 2013-02-20 09:18:06.286-0600

Testing with allowguest and autocreatepeer looks good with alwaysauthreject enabled.

By: Walter Doekes (wdoekes) 2013-03-04 09:11:42.516-0600

Good. I've been running the patch on a couple of 10.x production machines a couple of days now. Haven't noticed any odd behaviour or customer complaints.

By: Matt Jordan (mjordan) 2013-03-04 11:15:32.505-0600

FYI: The planned security patch date is Thursday, March 7th.