[Home]

Summary:ASTERISK-19905: Security Vulnerability: remotely exploitable crash in chan_skinny if client is disconnected when client is not in on-hook state
Reporter:Matt Jordan (mjordan)Labels:
Date Opened:2012-05-23 11:31:03Date Closed:2012-05-29 13:37:52
Priority:BlockerRegression?No
Status:Closed/CompleteComponents:Channels/chan_skinny
Versions:1.8.12.0 10.4.0 Frequency of
Occurrence
Related
Issues:
must be completed before resolvingASTERISK-19901 Asterisk 1.8.13.0 Blockers
must be completed before resolvingASTERISK-19902 Asterisk 10.5.0 Blockers
must be completed before resolvingASTERISK-20019 Asterisk 10.6.0 Blockers
is related toASTERISK-20015 Device handling issues in skinny
Environment:Attachments:( 0) AST-19905-1.8.diff
( 1) AST-19905-10.diff
( 2) AST-19905-trunk.diff
( 3) AST-2012-008-1.8.diff
( 4) AST-2012-008-10.diff
( 5) AST-2012-009-10.diff
( 6) skinny.ldevice-deref.patch
( 7) skinny.unregister.patch
( 8) vendor_disclosure_notes.txt
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*."

{noformat}

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;
}

{noformat}
Comments:By: Damien Wedhorn (wedhorn) 2012-05-26 18:17:59.441-0500

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.
By: Matt Jordan (mjordan) 2012-05-29 07:19:30.430-0500

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.
By: Matt Jordan (mjordan) 2012-05-29 08:40:38.175-0500

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
By: Matt Jordan (mjordan) 2012-05-29 08:58:30.105-0500

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
By: Matt Jordan (mjordan) 2012-05-29 10:58:36.240-0500

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