Asterisk
  1. Asterisk
  2. ASTERISK-26938

Heap overflow in CSEQ header parsing affects Asterisk chan_pjsip and PJSIP

    Details

      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.

      1. core-asterisk-1492008299.27652-brief.txt
        70 kB
        George Joseph
      2. core-asterisk-1492008299.27652-full.txt
        206 kB
        George Joseph
      3. core-asterisk-1492008299.27652-locks.txt
        0.0 kB
        George Joseph
      4. core-asterisk-1492008299.27652-thread1.txt
        6 kB
        George Joseph
      5. options.xml
        2 kB
        George Joseph
      6. options2.raw
        2 kB
        Sandro Gauci
      7. README.md
        7 kB
        Sandro Gauci

        Issue Links

          Activity

          No builds found.
          Sandro Gauci created issue -
          Hide
          Asterisk Team added a comment -

          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 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.

          Show
          Asterisk Team added a comment - 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 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 .
          George Joseph made changes -
          Field Original Value New Value
          Security None [ 10113 ] Reporter, Bug Marshals, and Digium [ 10110 ]
          George Joseph made changes -
          Labels Security
          Hide
          Sandro Gauci added a comment -

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

          Show
          Sandro Gauci added a comment - The markdown file contains our report, the options2.raw is the SIP message to reproduce the issue.
          Sandro Gauci made changes -
          Attachment README.md [ 55362 ]
          Attachment options2.raw [ 55363 ]
          Sandro Gauci made changes -
          Summary Locked Heap overflow in CSEQ header parsing affects Asterisk chan_pjsip and PJSIP
          Description NA 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.
          Component/s pjproject/pjsip [ 12616 ]
          George Joseph made changes -
          Status Triage [ 10000 ] Open [ 1 ]
          Hide
          George Joseph added a comment -

          sipp scenario.

          Show
          George Joseph added a comment - sipp scenario.
          George Joseph made changes -
          Attachment options.xml [ 55365 ]
          George Joseph made changes -
          George Joseph made changes -
          Remote Link This issue links to "SWP-9725 (Digium JIRA Internal)" [ 15808 ]
          Hide
          Mark Michelson added a comment -

          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:

              /* 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);
          

          This is then immediately followed by the following:

              /* 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++ = '$';
              }
          

          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:

              /* 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);
          

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

          Show
          Mark Michelson added a comment - 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: /* 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); This is then immediately followed by the following: /* 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++ = '$'; } 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: /* 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); This way, the method name is taken into account when performing the allocation.
          Hide
          Mark Michelson added a comment -

          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.

          Show
          Mark Michelson added a comment - 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.
          Hide
          Mark Michelson added a comment - - edited

          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.

          Show
          Mark Michelson added a comment - - edited 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.
          Friendly Automation made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Hide
          Friendly Automation added a comment -

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

          https://gerrit.asterisk.org/5658

          Show
          Friendly Automation added a comment - Change 5658 merged by Jenkins2: AST-2017-002: Ensure transaction key buffer is large enough. https://gerrit.asterisk.org/5658
          Hide
          Friendly Automation added a comment -

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

          https://gerrit.asterisk.org/5659

          Show
          Friendly Automation added a comment - Change 5659 merged by Jenkins2: AST-2017-002: Ensure transaction key buffer is large enough. https://gerrit.asterisk.org/5659
          Hide
          Friendly Automation added a comment -

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

          https://gerrit.asterisk.org/5662

          Show
          Friendly Automation added a comment - Change 5662 merged by George Joseph: AST-2017-002: Ensure transaction key buffer is large enough. https://gerrit.asterisk.org/5662
          Hide
          Friendly Automation added a comment -

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

          https://gerrit.asterisk.org/5671

          Show
          Friendly Automation added a comment - Change 5671 merged by Matthew Fredrickson: AST-2017-002: Ensure transaction key buffer is large enough. https://gerrit.asterisk.org/5671
          Hide
          Friendly Automation added a comment -

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

          https://gerrit.asterisk.org/5665

          Show
          Friendly Automation added a comment - Change 5665 merged by Matthew Fredrickson: AST-2017-002: Ensure transaction key buffer is large enough. https://gerrit.asterisk.org/5665
          Hide
          Friendly Automation added a comment -

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

          https://gerrit.asterisk.org/5668

          Show
          Friendly Automation added a comment - Change 5668 merged by Matthew Fredrickson: AST-2017-002: Ensure transaction key buffer is large enough. https://gerrit.asterisk.org/5668
          Richard Mudgett made changes -
          Security Reporter, Bug Marshals, and Digium [ 10110 ]
          Matthew Fredrickson made changes -
          Target Release Version/s 14.4.1 [ 13584 ]
          Target Release Version/s 13.15.1 [ 13585 ]
          Kevin Harwell made changes -
          Target Release Version/s 13.16.0 [ 13583 ]
          Kevin Harwell made changes -
          Target Release Version/s 13.16.0 [ 13583 ]
          Kevin Harwell made changes -
          Target Release Version/s 14.5.0 [ 13582 ]
          Kevin Harwell made changes -
          Target Release Version/s 14.5.0 [ 13582 ]
          George Joseph made changes -
          Target Release Version/s 15.0.0 [ 13592 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Triage Triage Open Open
          5h 44m 1 gtj 12/Apr/17 9:46 AM
          Open Open Closed Closed
          37d 4h 20m 1 Friendly Automation 19/May/17 2:06 PM

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development