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

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 Jordan
added a comment - 29/May/12 8:40 AM - 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

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

Matt Jordan
added a comment - 29/May/12 8:58 AM 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

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 Jordan
added a comment - 29/May/12 10:58 AM 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