Bug#394025: Remote compromise

[Ben Hutchings]

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 -- ben at decadentplace.org.uk shortened to ben at decadent.org.uk
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.alioth.debian.org/pipermail/pkg-voip-maintainers/attachments/20061020/f0d86c30/attachment.pgp