Summary:	ASTERISK-25707: Long contact URIs or hostnames can crash pjproject/Asterisk under certain conditions
Reporter:	George Joseph (gjoseph)	Labels:	Security
Date Opened:	2016-01-19 19:25:21.000-0600	Date Closed:	2016-04-14 13:01:20
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Resources/res_pjsip
Versions:	SVN 13.7.0	Frequency of Occurrence	Constant
Related Issues:
Environment:		Attachments:	( 0) 0001-res_pjsip-Validate-that-URIs-don-t-exceed-pjproject-.patch ( 1) ASTERISK-25707-assertion-bt.txt ( 2) bt_full.txt ( 3) register.xml
Description:	If pjproject is compiled without the -DNDEBUG CFLAG, then it's possible to crash Asterisk by crafting a REGISTER with either a user or hostname greater than pjproject's compiled limits.  If authentication is required, the issue won't happen because we have to add the contact, then try to use it.  If rewrite_contact is set on the endpoint, then you can protect against hostname attacks but if the combined uri is > 128, pjproject triggers an assert/raise which will crash the process.  Worse, the contact will be added to astdb before the crash so when Asterisk restarts, it will just crash again when it tries to send an OPTIONS.  This will continue until the registration times out.
Comments:	By: Asterisk Team (asteriskteam) 2016-01-19 19:25:22.479-0600 Thanks for creating a report! The issue has entered the triage process. That means the issue will wait in this status until a Bug Marshal has an opportunity to review the issue. Once the issue has been reviewed you will receive comments regarding the next steps towards resolution. A good first step is for you to review the [Asterisk Issue Guidelines|https://wiki.asterisk.org/wiki/display/AST/Asterisk+Issue+Guidelines] if you haven't already. The guidelines detail what is expected from an Asterisk issue report. Then, if you are submitting a patch, please review the [Patch Contribution Process|https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process]. By: George Joseph (gjoseph) 2016-01-20 12:04:37.851-0600 This patch should apply cleanly to 13 and to master with a little fuzz. You can apply it with 'git am' By: Mark Michelson (mmichelson) 2016-02-08 14:42:21.033-0600 Hi George, I was having a look at the patch and I have three findings on it: 1) Does {{ast_sip_validate_uri_length()}} have to do its own parsing? Would it be acceptable to just call the built-in PJSIP function {{pjsip_parse_uri()}} instead? Or is {{pjsip_parse_uri()}} actually where the crash happens? 2) In {{permanent_uri_handler()}}, you've added an error message that misspells "Contact" as "Ccontact". 3) The SRV prefix logic is potentially flawed. First, "_sip._udp" is only one possibility for an SRV prefix. "_sips._tcp" is also possible and is one character longer. Also, if the URI has a port present, then there should never be an SRV lookup attempted on the URI, so there is no need to take into account the SRV prefix in that case. Also also, for master, even if SRV/NAPTR lookups are necessary, if using the external resolver (i.e. the SRV/NAPTR stuff we wrote in Asterisk), then is the crash still going to happen? Can I see an example of a backtrace you're seeing from one of these crashes? I'm flying blind on this one since I don't know where the actual crash is or at what point it happens. The fact that PJSIP/PJLIB is including the SRV prefix when comparing against PJ_MAX_HOSTNAME is totally bonkers to me. It makes me wonder if we should point something out to Teluu in addition to patching Asterisk. By: George Joseph (gjoseph) 2016-02-08 17:08:18.727-0600 Well, for some reason this issue has changed.  It now SEGVs with or without the NDEBUG. The problem with pjsip_parse_uri is not only that it may trigger the issue ( i haven't tested it) but it also requires a pj_pool_t which is doable but I really only need the hostname. bt and a sipp scenario will be attached shortly. By: George Joseph (gjoseph) 2016-02-08 19:36:07.158-0600 The issue still happens in master because the overflow happens before pjproject sends the hostname back to Asterisk. I'm not sure the SRV inclusion is unreasonable.  The buffer is fixed at PJ_MAX_HOSTNAME even for pjproject internal use.  Although I guess they could allocate the array at PJ_MAX_HOSTNAME + strlen("_sips._tcp.") so they can try the SRV lookups.  Now why they even HAVE a fixed length array is a whole other question. :) By: George Joseph (gjoseph) 2016-02-08 21:24:26.910-0600 New patch using pjsip_parse_uri.  Verified that it doesn't cause the SEGV itself. Apply with git-am against 13. By: Mark Michelson (mmichelson) 2016-02-09 16:22:34.764-0600 Uploading ASTERISK-25707-assertion-bt.txt . This is a backtrace I was able to produce from performing an outbound registration to an overly-long hostname. The inbound registration backtrace uploaded by [~gtj] is a separate issue but is also security-related and related to long host names. I think the patch uploaded here is the proper fix for the outbound registration issue. For inbound, the fix is simpler since the problem exists due to our ignoring some return values from PJSIP. By: Mark Michelson (mmichelson) 2016-02-10 12:52:13.480-0600 Here's an update from me for the day so far. I am still unable to make the assertion happen in PJLIB in a scenario where the incoming message causes it to happen. I've only been able to have it happen by setting up an outbound registration scenario. As such, I think we can think of this as two separate issues: 1) On an incoming registration, a contact URI that exceeds PJSIP_MAX_URL_SIZE can result in a crash. This is because Asterisk is ignoring the return values of pjsip_uri_print() and pjsip_parse_uri() at key points when handling registrations. 2) On an outgoing registration, if a configured contact URI's hostname exceeds PJ_MAX_HOSTNAME, then PJLib can crash if built in debug mode. Of the two issues, 1 clearly is a security problem. 2...I'm not so sure. It definitely needs to be addressed and is a bug, but can it really be exploited? Now, if the assertion in 2 can be triggered based on receiving an incoming SIP request of some sort, then that's a different story and it's definitely a security problem. But otherwise I'm not convinced. As far as your patch goes, there is only one further concern I have. When calculating the max hostname length, you do not need to subtract out "_sips._tcp" if the parsed URI's port is 0. This is because an explicit port means that no SRV lookup will be performed, so the hostname will only be used as-is. Change that logic and it'll have my +1. I'm going to start drafting a security advisory for the crash caused by issue 1 I listed above. By: Mark Michelson (mmichelson) 2016-02-10 15:13:37.801-0600 After working with George on IRC a bit more, it turns out that (2) can be caused by receving a poisonous REGISTER request. Here is the updated list of issues: 1) On an incoming registration, if a contact URI is larger than PJSIP_MAX_URL_SIZE, then we will crash when attempting to render the 200 OK response to that REGISTER. 2) On an incoming registration, if a contact URI is smaller than PJSIP_MAX_URL_SIZE, but the hostname in that URI is larger than PJ_MAX_HOSTNAME, then the registration will succeed, but any attempts to make an outbound request to that contact will crash due to an assertion in the PJLib resolver. Your patch actually fixes both of these because of the added call to pjsip_uri_print() in registrar_validate_contacts(). We now short-circuit processing the contact as soon as we realize it's too large to fit in a sane buffer. So just fix the finding I mentioned previously and we'll have a patch that's good to go and will fix everything. By: George Joseph (gjoseph) 2016-02-10 17:04:37.406-0600 Updated.