Asterisk
  1. Asterisk
  2. ASTERISK-20658

Blindly doing alloca (or strdupa) on potentially large user input is bad

    Details

    • Type: Bug Bug
    • Status: Closed
    • Severity: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.8.18.0, 10.10.0, 10.10.0-digiumphones, 11.0.1
    • Target Release Version/s: 1.8.19.1, 10.11.1, 10.11.1-digiumphones, 11.1.1
    • Component/s: None
    • Labels:
      None
    • Regression:
      No

      Description

      I recently removed a potential user initiated crash by removing unnecessary ast_strdupa's in this patch.
      http://lists.digium.com/pipermail/asterisk-commits/2012-October/057328.html

      The problem is that alloca() does not check whether things will fit in the stack. If they don't fit, asterisk will simply segfault.
      This wouldn't be so much of a problem, if it weren't for the limited stack size:

      /* default 500kB or 110kB */
      #define AST_STACKSIZE (((sizeof(void *) * 8 * 8) - 16) * 1024)
      #define AST_BACKGROUND_STACKSIZE (((sizeof(void *) * 8 * 2) - 16) * 1024)
      pthread_attr_setstacksize(attr, stacksize ? stacksize : AST_STACKSIZE);
      

      Before the mentioned patch, sending a SIP body that was too large (e.g. 700kB) would trigger the crash. Unfortunately, that isn't the only place where user data is fed to alloca and friends directly.

      Anywhere where one of ast_alloca, ast_str_alloca and ast_strdupa is used, case should be taken that the data isn't too large.

      While checking for issues in the SIP module, I've found at least three, but I'm pretty sure there are more. All of those are only exploitable if you're using TCP (or TLS) because (fragmented) UDP won't allow packet sizes that big.

      • Allow: BIG_STRING
      • Content-Type: multipart/mixed;boundary="BIG_STRING"
      • sdp body: o=BIG_STRING\nm=audio\n

      The easy fix is to cap a SIP packet size to something reasonable like 20K. That's what the attached patch [1] does.

      Patch [2] makes the alloca's visible. It could be used to scan for other potential problems. (I've rewritten the ast_verbose alloca to use C99 variable length arrays. That suffers from the same problem, but it declutters the output.)

      [1] issueA20658_limit_sip_packet_size.patch
      [2] issueA20658_view_allocas.patch

      Regards,
      Walter Doekes
      OSSO B.V.

      1. ASTERISK-20658_res_jabber.c.patch
        0.8 kB
        Mark Michelson
      2. issueA20658_dont_process_overlong_config_lines.patch
        0.9 kB
        Walter Doekes
      3. issueA20658_func_realtime_limit.patch
        1 kB
        Walter Doekes
      4. issueA20658_http_postvars_use_malloc2.patch
        1 kB
        Walter Doekes
      5. issueA20658_limit_sip_packet_size3.patch
        6 kB
        Walter Doekes
      6. issueA20658_sanitize_app_mysql.patch
        5 kB
        Walter Doekes
      7. issueA20658_view_allocas.patch
        3 kB
        Walter Doekes

        Issue Links

          Activity

          Hide
          Mark Michelson added a comment -

          I've been giving looks at this and I'm having difficulty finding anything further that is in need of immediate care.

          I plan on fixing this issue in four separate commits. In one commit, we get the security fixes:

          1) chan_sip.c (except the s/SSL/TLS/ changes)
          2) http.c
          3) res_jabber.c

          In the second, we get the other crashes that are not necessarily security fixes:

          1) func_realtime.c
          2) config.c There is a grammatical error in the error message ("with with") that I will fix when I commit.

          The third will be a trunk-only commit that will sanitize app_mysql.c.

          The fourth will be a trunk-only commit that will replace "SSL" with "TLS" in log messages in chan_sip.c.

          Show
          Mark Michelson added a comment - I've been giving looks at this and I'm having difficulty finding anything further that is in need of immediate care. I plan on fixing this issue in four separate commits. In one commit, we get the security fixes: 1) chan_sip.c (except the s/SSL/TLS/ changes) 2) http.c 3) res_jabber.c In the second, we get the other crashes that are not necessarily security fixes: 1) func_realtime.c 2) config.c There is a grammatical error in the error message ("with with") that I will fix when I commit. The third will be a trunk-only commit that will sanitize app_mysql.c. The fourth will be a trunk-only commit that will replace "SSL" with "TLS" in log messages in chan_sip.c.
          Hide
          Walter Doekes added a comment -

          Sounds good to me.

          Show
          Walter Doekes added a comment - Sounds good to me.
          Hide
          Walter Doekes added a comment -

          Bump.

          I see that we have new releases, but this isn't in it. Status update, anyone?

          /w

          Show
          Walter Doekes added a comment - Bump. I see that we have new releases, but this isn't in it. Status update, anyone? /w
          Hide
          Mark Michelson added a comment -

          We're at a stage right now where test developers at Digium are ensuring that the patches provided are working. They're also working on a more difficult security vulnerability as well.

          I'm not sure when a release will be out with this fix since coordination has become difficult due to lots of people taking vacation this time of year. Don't worry though, this issue hasn't been forgotten about.

          Show
          Mark Michelson added a comment - We're at a stage right now where test developers at Digium are ensuring that the patches provided are working. They're also working on a more difficult security vulnerability as well. I'm not sure when a release will be out with this fix since coordination has become difficult due to lots of people taking vacation this time of year. Don't worry though, this issue hasn't been forgotten about.
          Hide
          Walter Doekes added a comment -

          Oki. Thanks for the update.

          Show
          Walter Doekes added a comment - Oki. Thanks for the update.

            People

            • Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development