[Home]

Summary:ASTERISK-26938: Heap overflow in CSEQ header parsing affects Asterisk chan_pjsip and PJSIP
Reporter:Sandro Gauci (sandrogauci)Labels:Security
Date Opened:2017-04-12 04:01:48Date Closed:2017-05-19 14:06:46
Priority:CriticalRegression?
Status:Closed/CompleteComponents:pjproject/pjsip
Versions:14.4.0 Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) core-asterisk-1492008299.27652-brief.txt
( 1) core-asterisk-1492008299.27652-full.txt
( 2) core-asterisk-1492008299.27652-locks.txt
( 3) core-asterisk-1492008299.27652-thread1.txt
( 4) options.xml
( 5) options2.raw
( 6) README.md
Description:I have included our draft report as an attachment. Please do let me know if this is uncomfortable for you and would like details here.
Comments:By: Asterisk Team (asteriskteam) 2017-04-12 04:01:50.880-0500

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: Sandro Gauci (sandrogauci) 2017-04-12 08:55:19.729-0500

The markdown file contains our report, the options2.raw is the SIP message to reproduce the issue.

By: George Joseph (gjoseph) 2017-04-12 09:47:32.624-0500

sipp scenario.


By: Mark Michelson (mmichelson) 2017-04-12 14:55:16.997-0500

I have determined the cause of the crash seen here. When an incoming SIP request arrives, PJSIP needs to determine if the incoming message matches a SIP transaction that is currently underway. To do this, it uses the message contents to create a key and then looks up that key in a hashtable of SIP transactions that are currently in progress. PJSIP has two algorithms for calculating a transaction key. The more common algorithm is based on RFC 3261's (SIP 2.0) requirement of a unique branch parameter in the top-most Via header. The less common algorithm is intended to be used when communicating with endpoints that are following RFC 2543 (SIP 1.0).

In the crashing scenario, there is no branch parameter in the Via header, so the RFC 2543 algorithm is used when creating the transaction key. Space for the transaction key is done as follows:
{code}
   /* Calculate length required. */
   len_required = 9 +                      /* CSeq number */
                  rdata->msg_info.from->tag.slen +   /* From tag. */
                  rdata->msg_info.cid->id.slen +    /* Call-ID */
                  host->slen +             /* Via host. */
                  9 +                      /* Via port. */
                  16;                      /* Separator+Allowance. */
   key = p = (char*) pj_pool_alloc(pool, len_required);
{code}

This is then immediately followed by the following:
{code}
   /* Add method, except when method is INVITE or ACK. */
   if (method->id != PJSIP_INVITE_METHOD && method->id != PJSIP_ACK_METHOD) {
       pj_memcpy(p, method->name.ptr, method->name.slen);
       p += method->name.slen;
       *p++ = '$';
   }
{code}

As you can see, the allocation length does not take into account the length of the CSeq method name. In normal circumstances, this is not an issue since the allocation is slightly larger than necessary and SIP method names are short. However, when a long CSeq method is used, the {{pjmemcpy()}} results in the CSeq method being written past the end of the allocation. A small overrun could still not cause a crash since PJLIB uses a block allocation scheme. However, making the CSeq ridiculously long results in the {{pjmemcpy()}} writing past the length of the allocation, plus the boundaries of the memory pool. In this particular case, when a later {{free()}} was performed, the malloc'd block was corrupted and resulted in a crash.

We will be submitting this report to the PJProject maintainers and suggesting the following change be made to the allocation:
{code}
   /* Calculate length required. */
   len_required = method->name.slen +      /* Method */
                  9 +                      /* CSeq number */
                  rdata->msg_info.from->tag.slen +   /* From tag. */
                  rdata->msg_info.cid->id.slen +    /* Call-ID */
                  host->slen +             /* Via host. */
                  9 +                      /* Via port. */
                  16;                      /* Separator+Allowance. */
   key = p = (char*) pj_pool_alloc(pool, len_required);
{code}

This way, the method name is taken into account when performing the allocation.

By: Mark Michelson (mmichelson) 2017-04-12 15:10:45.131-0500

Actually, I will wait to report this to the PJProject maintainers for the time being. I understand that you have a couple more Asterisk security issues to report, and so we will wait to make sure that those do not require PJProject fixes as well. This may require coordination between us, PJProject, and some of their stakeholders to make sure we do not expose anyone to attacks if possible.

By: Mark Michelson (mmichelson) 2017-04-17 12:59:25.928-0500

Update: We've begun a dialog with the PJProject maintainers. They are reviewing the patches and will get back to us as soon as they can. This goes for ASTERISK-26939 as well.

By: Friendly Automation (friendly-automation) 2017-05-19 14:06:48.008-0500

Change 5658 merged by Jenkins2:
AST-2017-002: Ensure transaction key buffer is large enough.

[https://gerrit.asterisk.org/5658|https://gerrit.asterisk.org/5658]

By: Friendly Automation (friendly-automation) 2017-05-19 14:16:06.368-0500

Change 5659 merged by Jenkins2:
AST-2017-002: Ensure transaction key buffer is large enough.

[https://gerrit.asterisk.org/5659|https://gerrit.asterisk.org/5659]

By: Friendly Automation (friendly-automation) 2017-05-19 14:22:18.343-0500

Change 5662 merged by George Joseph:
AST-2017-002: Ensure transaction key buffer is large enough.

[https://gerrit.asterisk.org/5662|https://gerrit.asterisk.org/5662]

By: Friendly Automation (friendly-automation) 2017-05-19 15:11:07.531-0500

Change 5671 merged by Matthew Fredrickson:
AST-2017-002: Ensure transaction key buffer is large enough.

[https://gerrit.asterisk.org/5671|https://gerrit.asterisk.org/5671]

By: Friendly Automation (friendly-automation) 2017-05-19 15:11:21.962-0500

Change 5665 merged by Matthew Fredrickson:
AST-2017-002: Ensure transaction key buffer is large enough.

[https://gerrit.asterisk.org/5665|https://gerrit.asterisk.org/5665]

By: Friendly Automation (friendly-automation) 2017-05-19 15:12:34.885-0500

Change 5668 merged by Matthew Fredrickson:
AST-2017-002: Ensure transaction key buffer is large enough.

[https://gerrit.asterisk.org/5668|https://gerrit.asterisk.org/5668]