[Home]

Summary:ASTERISK-16043: [patch] bypass "contactdeny" with nat=yes
Reporter:klaus3000 (klaus3000)Labels:
Date Opened:2010-05-03 09:56:57Date Closed:2010-07-16 12:34:25
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Channels/chan_sip/Registration
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) patch-asterisk-trunk-contactdeny.txt
( 1) testresults.txt
Description:Hi!

chan_sip's "contactdeny" feature screens the "to be registered contact". In case of nat=yes it should not use the address information from the Contact header (which is not used at all for routing), but the source IP address of the request.

Thus, if nat=yes and a client sends a request from a denied IP address (e.g. by spoofing the src-IP address) it can bypass the screening.

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

This is the current code:

       if (ast_apply_ha(sip_cfg.contact_ha, &testsin) != AST_SENSE_ALLOW ||
                       ast_apply_ha(peer->contactha, &testsin) != AST_SENSE_ALLOW) {
               ast_log(LOG_WARNING, "Host '%s' disallowed by contact ACL (violating IP %s)\n", host, ast_inet_ntoa(testsin.sin_addr));
               ast_string_field_set(peer, fullcontact, "");
               ast_string_field_set(pvt, our_contact, "");
               return PARSE_REGISTER_DENIED;
       }

       /*! \todo This could come before the checking of DNS earlier on, to avoid
               DNS lookups where we don't need it... */
       if (!ast_test_flag(&peer->flags[0], SIP_NAT_FORCE_RPORT) && !ast_test_flag(&peer->flags[0], SIP_NAT_RPORT_PRESENT)) {
               peer->addr.sin_family = AF_INET;
               memcpy(&peer->addr.sin_addr, hp->h_addr, sizeof(peer->addr.sin_addr));
               peer->addr.sin_port = htons(port);
       } else {
               /* Don't trust the contact field.  Just use what they came to us
                  with */
               peer->addr = pvt->recv;
       }

What about moving the "set peer->addr" code before the contact check, and then use peer->addr during the contact check?
Comments:By: Paul Belanger (pabelanger) 2010-05-04 13:41:08

You might get some better traction from #asterisk-dev or mailing lists.
By: Leif Madsen (lmadsen) 2010-05-07 10:57:18

Ya chan_sip handling is a contentious issue because someone could potentially be expecting/working around that, and we need to be very careful about these types of changes and to give it lots of thought on what it could potentially break.

That's not to say this may not be a fine change, but we should get some feedback. Thanks!
By: klaus3000 (klaus3000) 2010-05-12 03:48:04

Dicussion is happening on mailing list:
http://lists.digium.com/pipermail/asterisk-dev/2010-May/043924.html
By: Leif Madsen (lmadsen) 2010-05-17 10:50:03

Quick summary thus far:  Olle agreeds, Klaus has a patch to resolve the issue but is waiting for feedback to make sure his approach is correct.
By: klaus3000 (klaus3000) 2010-05-17 12:59:37

There wasn't any feedback to my proposal at: http://lists.digium.com/pipermail/asterisk-dev/2010-May/044065.html

Thus, I write a patch which fits above proposal. Testresults (IMO OK) are attached too.
By: klaus3000 (klaus3000) 2010-05-17 13:01:24

Note: I have not understand the usage of the SIP_NAT_RPORT_PRESENT, thus I did not changed the if-condition.
By: Paul Belanger (pabelanger) 2010-05-17 13:08:19

I would encourage you to write a testcase for this in the asterisk testsuite! :) If your feeling up to it.
By: klaus3000 (klaus3000) 2010-05-17 13:51:00

Is there somewhere a Howto for the testsuite? Or a pointer where to start? Thanks
By: Leif Madsen (lmadsen) 2010-05-17 13:55:40

My blog post (http://blogs.asterisk.org/2010/04/29/installing-the-asterisk-test-suite/ ) at least starts introducing it by getting it installed and running. The README.txt should have some introductions to getting started on building a test, but the best information probably stems from looking at an existing test in the testsuite to determine how to build one.

I'm sure any questions asked on the asterisk-dev mailing list would be enthusiastically replied to as we're trying to get more contributions to the testsuite.


By: Digium Subversion (svnbot) 2010-06-15 15:18:05

Repository: asterisk
Revision: 270658

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r270658 | twilson | 2010-06-15 15:18:04 -0500 (Tue, 15 Jun 2010) | 20 lines

Make contactdeny apply to src ip when nat=yes

chan_sip's "contactdeny" feature screens the "to be registered contact".
In case of nat=yes it should not use the address information from the
Contact header (which is not used at all for routing), but the source
IP address of the request.

Thus, if nat=yes and a client sends a request from a denied IP address
(e.g. by spoofing the src-IP address) it can bypass the screening.

This commit makes contactdeny apply to the src ip when nat=yes instead.

(closes issue ASTERISK-16043)
Reported by: klaus3000
Patches:
     patch-asterisk-trunk-contactdeny.txt uploaded by klaus3000 (license 65)
Tested by: klaus3000

Review: [full review board URL with trailing slash]

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

http://svn.digium.com/view/asterisk?view=rev&revision=270658
By: Digium Subversion (svnbot) 2010-06-15 17:16:52

Repository: asterisk
Revision: 270693

_U  branches/1.6.2/
U   branches/1.6.2/channels/chan_sip.c

------------------------------------------------------------------------
r270693 | twilson | 2010-06-15 17:16:52 -0500 (Tue, 15 Jun 2010) | 27 lines

Merged revisions 270658 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r270658 | twilson | 2010-06-15 15:18:04 -0500 (Tue, 15 Jun 2010) | 20 lines
 
 Make contactdeny apply to src ip when nat=yes
 
 chan_sip's "contactdeny" feature screens the "to be registered contact".
 In case of nat=yes it should not use the address information from the
 Contact header (which is not used at all for routing), but the source
 IP address of the request.
 
 Thus, if nat=yes and a client sends a request from a denied IP address
 (e.g. by spoofing the src-IP address) it can bypass the screening.
 
 This commit makes contactdeny apply to the src ip when nat=yes instead.
 
 (closes issue ASTERISK-16043)
 Reported by: klaus3000
 Patches:
       patch-asterisk-trunk-contactdeny.txt uploaded by klaus3000 (license 65)
 Tested by: klaus3000
 
 Review: [full review board URL with trailing slash]
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=270693
By: Digium Subversion (svnbot) 2010-06-15 17:34:30

Repository: asterisk
Revision: 270724

U   branches/1.4/channels/chan_sip.c

------------------------------------------------------------------------
r270724 | twilson | 2010-06-15 17:34:30 -0500 (Tue, 15 Jun 2010) | 27 lines

Merged revisions 270658 via svnmerge from
https://origsvn.digium.com/svn/asterisk/trunk

........
 r270658 | twilson | 2010-06-15 15:18:04 -0500 (Tue, 15 Jun 2010) | 20 lines
 
 Make contactdeny apply to src ip when nat=yes
 
 chan_sip's "contactdeny" feature screens the "to be registered contact".
 In case of nat=yes it should not use the address information from the
 Contact header (which is not used at all for routing), but the source
 IP address of the request.
 
 Thus, if nat=yes and a client sends a request from a denied IP address
 (e.g. by spoofing the src-IP address) it can bypass the screening.
 
 This commit makes contactdeny apply to the src ip when nat=yes instead.
 
 (closes issue ASTERISK-16043)
 Reported by: klaus3000
 Patches:
       patch-asterisk-trunk-contactdeny.txt uploaded by klaus3000 (license 65)
 Tested by: klaus3000
 
 Review: [full review board URL with trailing slash]
........

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

http://svn.digium.com/view/asterisk?view=rev&revision=270724
By: Digium Subversion (svnbot) 2010-07-16 12:34:24

Repository: asterisk
Revision: 270658

U   trunk/channels/chan_sip.c

------------------------------------------------------------------------
r270658 | twilson | 2010-06-15 15:18:04 -0500 (Tue, 15 Jun 2010) | 18 lines

Make contactdeny apply to src ip when nat=yes

chan_sip's "contactdeny" feature screens the "to be registered contact".
In case of nat=yes it should not use the address information from the
Contact header (which is not used at all for routing), but the source
IP address of the request.

Thus, if nat=yes and a client sends a request from a denied IP address
(e.g. by spoofing the src-IP address) it can bypass the screening.

This commit makes contactdeny apply to the src ip when nat=yes instead.

(closes issue ASTERISK-16043)
Reported by: klaus3000
Patches:
     patch-asterisk-trunk-contactdeny.txt uploaded by klaus3000 (license 65)
Tested by: klaus3000

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

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