Asterisk
  1. Asterisk
  2. ASTERISK-19905

Security Vulnerability: remotely exploitable crash in chan_skinny if client is disconnected when client is not in on-hook state

    Details

    • Type: Bug Bug
    • Status: Closed
    • Severity: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.8.12.0, 10.4.0
    • Target Release Version/s: 1.8.12.1, 10.4.1
    • Component/s: Channels/chan_skinny
    • Security Level: None
    • Labels:
      None
    • Regression:
      No

      Description

      Reported by Christoph Hebeisen of Telus Labs.

      Quoting from the vulnerability report:

      "A Null-pointer dereference has been identified in the SCCP (Skinny) channel driver of Asterisk. When an SCCP client closes its connection to the server, a pointer in a structure is set to Null. If the client was not in the on-hook state at the time the connection was closed, this pointer is later dereferenced.

      A remote attacker with a valid SCCP ID can can use this vulnerability by closing a connection to the Asterisk server in certain call states (e.g. "Off hook") to crash the server. Successful exploitation of this vulnerability would result in termination of the server, causing denial of service to legitimate users.

      The following code snippets have been taken from channels/chan_skinny.c as shipped in the source archive of Asterisk 10.5.0-rc1. Comments added by TELUS Security Labs start with the string TSL."

       
      chan_skinny.c:6798
      ------------------
      static int get_input(struct skinnysession *s)
      {
      [ ... Truncated for vulnerability ... ]
       	 } else if (res != 4) {
                       // *TSL* This section is executed when a skinny client closes its connection.
       	 	 ast_log(LOG_WARNING, "Skinny Client sent less data than expected. �Expected 4 but got %d.\n", res);
       	 	 ast_mutex_unlock(&s->lock);
       	 	 if (res == 0) {
       	 	 	 if (skinnydebug)
       	 	 	 	 ast_verb(1, "Skinny Client was lost, unregistering\n");
       	 	 	 skinny_unregister(NULL, s); � // *TSL* See below
       	 	 }
       	 	 return -1;
       	 }
       
       
      chan_skinny.c:2126
      ------------------
      static int skinny_unregister(struct skinny_req *req, struct skinnysession *s)
      {
       struct skinny_device *d;
       struct skinny_line *l;
       struct skinny_speeddial *sd;
       d = s->device;
      [ ... Truncated for readability ... ]
       	 	 if (l->device == d) {
       	 	 	 l->device = NULL; � // *TSL* Set device pointer to NULL
       	 	 	 ast_format_cap_remove_all(l->cap);
       	 	 	 ast_parse_allow_disallow(&l->prefs, l->cap, "all", 0);
       	 	 	 l->instance = 0;
       	 	 	 manager_event(EVENT_FLAG_SYSTEM, "PeerStatus", "ChannelType: Skinny\r\nPeer:
                                  Skinny/%s@%s\r\nPeerStatus: Unregistered\r\n", l->name, d->name);
       	 	 	 unregister_exten(l);
       	 	 	 ast_devstate_changed(AST_DEVICE_UNAVAILABLE, "Skinny/%s", l->name);
       	 	 }
       
      chan_skinny.c:2528
      ------------------
      static void transmit_stop_tone(struct skinny_device *d, int instance, int reference)
      {
       struct skinny_req *req;
       if (!(req = req_alloc(sizeof(struct stop_tone_message), STOP_TONE_MESSAGE)))
       	 return;
       req->data.stoptone.instance = htolel(instance);
       req->data.stoptone.reference = htolel(reference);
       transmit_response(d, req); � // *TSL* If device is unregistered, d is Null
      }
       
      chan_skinny.c:2363
      ------------------
      static void transmit_response(struct skinny_device *d, struct skinny_req *req)
      {
       transmit_response_bysession(d->session, req); � // *TSL* Dereference (crash, first vector)
      }
       
      chan_skinny.c:4729
      ------------------
      static int skinny_indicate(struct ast_channel *ast, int ind, const void *data, size_t datalen)
      {
       struct skinny_subchannel *sub = ast->tech_pvt;
       struct skinny_line *l = sub->line;
       struct skinny_device *d = l->device;
       struct skinnysession *s = d->session; � // *TSL* Dereference (crash, second vector)
       if (!s) {
       	 ast_log(LOG_NOTICE, "Asked to indicate '%s' condition on channel %s, but session does not exist.\n",
                 control2str(ind), ast->name);
       	 return -1;
       }
      
      
      1. AST-19905-1.8.diff
        4 kB
        Damien Wedhorn
      2. AST-19905-10.diff
        4 kB
        Damien Wedhorn
      3. AST-19905-trunk.diff
        4 kB
        Damien Wedhorn
      4. AST-2012-008-1.8.diff
        3 kB
        Matt Jordan
      5. AST-2012-008-10.diff
        2 kB
        Matt Jordan
      6. AST-2012-009-10.diff
        0.9 kB
        Matt Jordan
      7. skinny.ldevice-deref.patch
        3 kB
        Damien Wedhorn
      8. skinny.unregister.patch
        2 kB
        Damien Wedhorn
      9. vendor disclosure notes.txt
        13 kB
        Matt Jordan

        Issue Links

          Activity

          Hide
          Damien Wedhorn added a comment - - edited

          Bonus, the patches (for 10 and trunk) fix up the last bits of ASTERISK-16610 and so it can be tagged resolved once this is committed.

          Show
          Damien Wedhorn added a comment - - edited Bonus, the patches (for 10 and trunk) fix up the last bits of ASTERISK-16610 and so it can be tagged resolved once this is committed.
          Hide
          Matt Jordan added a comment -

          Thanks a lot for the patches. I'm going to go ahead and remove the two that I had previously written - I think your approach is a better solution.

          Show
          Matt Jordan added a comment - Thanks a lot for the patches. I'm going to go ahead and remove the two that I had previously written - I think your approach is a better solution.
          Hide
          Matt Jordan added a comment - - edited

          Damien:

          There's still a small problem with the patch (1.8 is what I'm currently testing). When an unregister occurs, the channel's tech_pvt is set to NULL. If you run the 1.8 attack vector script twice (using vector 0), this causes a crash in skinny_ss - it tries to create a thread for the channel (that for some reason is still hanging around), but has a tech_pvt of NULL.

          Matt

          Show
          Matt Jordan added a comment - - edited Damien: There's still a small problem with the patch (1.8 is what I'm currently testing). When an unregister occurs, the channel's tech_pvt is set to NULL. If you run the 1.8 attack vector script twice (using vector 0), this causes a crash in skinny_ss - it tries to create a thread for the channel (that for some reason is still hanging around), but has a tech_pvt of NULL. Matt
          Hide
          Matt Jordan added a comment -

          Damien:

          On closer inspection, skinny_ss looks to be inherently dangerous. It holds no locks on the channel, does not increase the reference count on the channel, and - since it uses the pvt data at will - it can, at any point in time, have the resources its using free'd out from underneath it.

          Not only that, but it can operate for an incredibly long period of time (3 seconds).

          I'd think about re-doing that entire method at a latter date.

          Matt

          Show
          Matt Jordan added a comment - Damien: On closer inspection, skinny_ss looks to be inherently dangerous. It holds no locks on the channel, does not increase the reference count on the channel, and - since it uses the pvt data at will - it can, at any point in time, have the resources its using free'd out from underneath it. Not only that, but it can operate for an incredibly long period of time (3 seconds). I'd think about re-doing that entire method at a latter date. Matt
          Hide
          Matt Jordan added a comment -

          Damien:

          So, nuts.

          The 1.8 patch, with some tweaks, could be made to work. Essentially, skinny_ss has to lock the sub_channel object while it does its work - otherwise, the unregister will nuke the sub_channel and the skinny_ss thread will inevitably crash.

          The 10 patch, on the other hand, has multiple issues that are rather too difficult to resolve at this point. These include (but are not limited to):

          • Race conditions in who destroys the sub_channel between skinny_hangup and skinny_unregister
          • Race conditions between in the destruction of the channel between skinny_ss and skinny_hangup/skinny_unregister

          Both of these are difficult to solve; the channel ref counting more so due to nothing using the ref counted nature of the channels throughout chan_skinny.

          At this time, I think the best approach will be to not use the patches for the security vulnerability. I think we'll have to go with the original approach of just preventing the immediate seg faults, and leave the architectural problems for another day.

          I would encourage, however, you to continue to get these patches to a point where they could go in, as I think they solve a number of problems that should be solved.

          Matt

          Show
          Matt Jordan added a comment - Damien: So, nuts. The 1.8 patch, with some tweaks, could be made to work. Essentially, skinny_ss has to lock the sub_channel object while it does its work - otherwise, the unregister will nuke the sub_channel and the skinny_ss thread will inevitably crash. The 10 patch, on the other hand, has multiple issues that are rather too difficult to resolve at this point. These include (but are not limited to): Race conditions in who destroys the sub_channel between skinny_hangup and skinny_unregister Race conditions between in the destruction of the channel between skinny_ss and skinny_hangup/skinny_unregister Both of these are difficult to solve; the channel ref counting more so due to nothing using the ref counted nature of the channels throughout chan_skinny. At this time, I think the best approach will be to not use the patches for the security vulnerability. I think we'll have to go with the original approach of just preventing the immediate seg faults, and leave the architectural problems for another day. I would encourage, however, you to continue to get these patches to a point where they could go in, as I think they solve a number of problems that should be solved. Matt

            People

            • Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development