forwarded 394025 http://bugs.digium.com/view.php?id=7770 tags 394025 + patch thanks
I'm adding a reference to the upstream bug report in case you really
want to read further details of this clusterfuck.
The upstream change is simply:
--- asterisk-1.2.12.1/channels/chan_skinny.c
+++ asterisk-1.2.13/channels/chan_skinny.c
@@ -2863,6 +2863,10 @@
return -1;
}
dlen = letohl(*(int *)s->inbuf);
+ if (dlen < 0) {
+ ast_log(LOG_WARNING, "Skinny Client sent invalid
data.\n");
+ return -1;
+ }
if (dlen+8 > sizeof(s->inbuf)) {
dlen = sizeof(s->inbuf) - 8;
}
-- END --
The new test deals with the case where dlen is negative. If dlen is a
large positive value the sum dlen+8 can overflow to become negative, but
thankfully it's compared with an unsigned value (type size_t is always
unsigned) so it will be implicitly converted to a large positive value
and replaced by the maximum acceptable length, as intended.
However, looking over this function, I realised there was another bug
right in front of me (reported upstream as
<http://bugs.digium.com/view.php?id=8186>). The call to
ast_mutex_unlock() is unmatched; there's no way the calling thread can
hold the lock at this point, and nor should it (skinnysession::inbuf
isn't shared between multiple threads). So I believe it should be
deleted.
Patch for sid:
--- asterisk-1.2.12.1.dfsg/channels/chan_skinny.c.orig 2006-10-19
23:05:19.000000000 +0000
+++ asterisk-1.2.12.1.dfsg/channels/chan_skinny.c 2006-10-19
23:43:34.000000000 +0000
@@ -2863,12 +2863,15 @@
return -1;
}
dlen = letohl(*(int *)s->inbuf);
+ if (dlen < 0) {
+ ast_log(LOG_WARNING, "Skinny Client sent invalid
data.\n");
+ return -1;
+ }
if (dlen+8 > sizeof(s->inbuf)) {
dlen = sizeof(s->inbuf) - 8;
}
*(int *)s->inbuf = htolel(dlen);
res = read(s->fd, s->inbuf+4, dlen+4);
- ast_mutex_unlock(&s->lock);
if (res != (dlen+4)) {
ast_log(LOG_WARNING, "Skinny Client sent less data than
expected.\n");
return -1;
-- END --
For sarge, there are two more problems:
1. The length is assumed to be in host byte order.
2. get_input() doesn't write back the modified length, so
skinny_req_parse() uses the original length.
I've combined the above changes with fixes for these:
--- asterisk-1.0.7.dfsg.1/channels/chan_skinny.c.orig 2006-10-20
00:10:49.000000000 +0000
+++ asterisk-1.0.7.dfsg.1/channels/chan_skinny.c 2006-10-20
00:16:37.000000000 +0000
@@ -2304,11 +2304,15 @@
ast_log(LOG_WARNING, "Skinny Client sent less data than
expected.\n");
return -1;
}
- dlen = *(int *)s->inbuf;
+ dlen = letohl(*(int *)s->inbuf);
+ if (dlen < 0) {
+ ast_log(LOG_WARNING, "Skinny Client sent invalid
data.\n");
+ return -1;
+ }
if (dlen+8 > sizeof(s->inbuf))
dlen = sizeof(s->inbuf) - 8;
+ *(int *)s->inbuf = htolel(dlen);
res = read(s->fd, s->inbuf+4, dlen+4);
- ast_mutex_unlock(&s->lock);
if (res != (dlen+4)) {
ast_log(LOG_WARNING, "Skinny Client sent less data than
expected.\n");
return -1;
@@ -2328,7 +2332,7 @@
}
memset(req, 0, sizeof(skinny_req));
/* +8 to account for reserved and length fields */
- memcpy(req, s->inbuf, *(int*)(s->inbuf)+8);
+ memcpy(req, s->inbuf, letohl(*(int*)(s->inbuf))+8);
if (req->e < 0) {
ast_log(LOG_ERROR, "Event Message is NULL from socket %d, This
is bad\n", s->fd);
free(req);
-- END --
These changes are untested, beyond verifying that the packages still
build. I attempted a simple denial of service with the following Python
code:
import socket
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM, 0)
s.connect(('127.0.0.1', 2000))
s.send('\xFA\xFF\xFF\xFF')
junk = '\xDE\xAD\xBE\xEF' * 1024
while True:
s.send(junk)
However, the second read() call in get_input() fails immediately with
with EFAULT, which I should have expected, and the connection is
dropped. Since Linux's read() verifies that the entire length of the
buffer is writeable before modifying any of it, I'm not convinced that
this is actually exploitable on Linux.
Ben.
--
Ben Hutchings -- [EMAIL PROTECTED] shortened to [EMAIL PROTECTED]
If you've signed my GPG key, please send a signature on and to the new uid.
Experience is what causes a person to make new mistakes instead of old ones.
signature.asc
Description: This is a digitally signed message part
