Summary:ASTERISK-07566: chan_skinny interger overflow (dos/possible remote compromise)
Reporter:Anthony LaMantia (alamantia)Labels:
Date Opened:2006-08-20 17:36:11Date Closed:2006-08-21 02:35:57
Versions:Frequency of
Environment:Attachments:( 0) skinny_patch.diff
Description:there is an interger overflow inside the get_input function inside of chan_skinny that allows anyone client which has established a active tcp connection to the skinny channel the ability to crash asterisk as well as comprimise a few cirtical structures within the channel driver.

the patched posted should repair the issue.

Comments:By: Jason Parker (jparker) 2006-08-20 23:14:14

It may just be that I'm tired, but I don't quite understand the intentions of this patch.  The read() that sets dlen reads exactly 4 bytes from the stream, and converts it to an int.  If almost looks like you're trying to protect from what I would call an "underflow".

Can you explain a little more?

Also, all patches need to be in unified format.  diff -u, or svn diff

By: Anthony LaMantia (alamantia) 2006-08-21 00:52:11

it's quite simple.

03906 static int get_input(struct skinnysession *s)
03907 {
03908    int res;
03909    int dlen = 0;
03910    struct pollfd fds[1];

dlen is a signed 32bit integer
and inbuf is a 1,000 byte char array located inside of a skinnysession struct.

03927       res = read(s->fd, s->inbuf, 4);

when any tcp connection is made to port 2000 get_input will read 4 bytes in to inbuf

dlen = letohl(*(int *)s->inbuf);

dlen is set to the assigned contents of the 32bits sent over the network to s->inbuf

consider for this example the remote client sends this

the value of dlen in memory as a signed integer would be -5


03936       if (dlen+8 > sizeof(s->inbuf)) {
03937          dlen = sizeof(s->inbuf) - 8;
03938       }
03939       *(int *)s->inbuf = htolel(dlen);

the above math is all preformed treating dlen as a signed integer
-5 + 8 = 3  and thats not larger then 1000..so dlen keeps it's retains..

after this the vulnerablity is exposed with this read

03941       res = read(s->fd, s->inbuf+4, dlen+4);

ssize_t read(int fd, void *buf, size_t count);
count, us an unsigned 16bit number (the max being 65535)

0xfffffffb + 0x4 = 0xffffffff

thus read will attempt to read into the 1,000 byte buffer 65535 bytes which would make it possible  for an attacker to  control importent bits of memory inside of the asterisk process...


if an attacker sends chan_skinny a 4byte packet that contains \xfb\xff\xff\xff
with one send() followed directly by  4,000 A's (for example) in another send()

asterisk will crash via chan_skinny  

see destory_session(s); that is called after get_input()  inside of skinny_session..

this may all be sort of hard to read, and im a bit overtired tommrow i can post another patch, and an example file that will cash asterisk when running chan_skinny

good night

By: Anthony LaMantia (alamantia) 2006-08-21 00:54:42

oh also,

the bug should really be classified as a signedness issue rather then an intergeroverflow, sorry about that..

i usually just class all of these sorts of problems as interger overflows..

By: Jason Parker (jparker) 2006-08-21 01:17:06

Made this bug private for now - for obvious reasons

By: Jason Parker (jparker) 2006-08-21 02:35:57

Fix committed in revision 40757.