Content-Length bugs

[Mark Doliner]

Hmm, I think the specific example of http_canread() might not be a
buffer overflow or null pointer dereference. I think the worst that
can happen is the while loop will run for a long time reading 1 byte
at a time. Eventually read() will probably return EAGAIN and we'll
break out of the while loop. But please let me know if anyone
disagrees.

I checked in the attached change to our private 2.x.y branch. My
inclination is to leave it like this until 3.0.0, when we hopefully
use the new unified http parsing code.
-------------- next part --------------
diff -r 82ec5fb22ce9 ChangeLog
--- a/ChangeLog	Sun Jan 12 13:08:28 2014 -0800
+++ b/ChangeLog	Sun Jan 12 14:27:03 2014 -0800
@@ -9,6 +9,8 @@ version 2.10.8:
 	* Fix buffer overflow when parsing a malformed HTTP response with
 	  chunked Transfer-Encoding. (Discovered by Matt Jones, Volvent)
 	  (CVE-2014-NNNN)
+	* Better handling of HTTP proxy responses with negative Content-Lengths.
+	  (Discovered by Matt Jones, Volvent)
 	* Fix handling of SSL certificates without subjects when using libnss.
 	* Fix handling of SSL certificates with timestamps in the distant future
 	  when using libnss. (#15586)
diff -r 82ec5fb22ce9 libpurple/proxy.c
--- a/libpurple/proxy.c	Sun Jan 12 13:08:28 2014 -0800
+++ b/libpurple/proxy.c	Sun Jan 12 14:27:03 2014 -0800
@@ -985,22 +985,34 @@ http_canread(gpointer data, gint source,
 	p = g_strrstr((const gchar *)connect_data->read_buffer, "Content-Length: ");
 	if (p != NULL) {
 		gchar *tmp;
-		int len = 0;
+		gsize content_len;
 		char tmpc;
+
 		p += strlen("Content-Length: ");
 		tmp = strchr(p, '\r');
 		if(tmp)
 			*tmp = '\0';
-		len = atoi(p);
+		if (sscanf(p, "%" G_GSIZE_FORMAT, &content_len) != 1) {
+			/* Couldn't read content length */
+			purple_debug_error("proxy", "Couldn't read content length value "
+					"from %s\n", p);
+			if(tmp)
+				*tmp = '\r';
+			purple_proxy_connect_data_disconnect_formatted(connect_data,
+					_("Unable to parse response from HTTP proxy: %s"),
+					connect_data->read_buffer);
+			return;
+		}
 		if(tmp)
 			*tmp = '\r';
 
 		/* Compensate for what has already been read */
-		len -= connect_data->read_len - headers_len;
+		content_len -= connect_data->read_len - headers_len;
 		/* I'm assuming that we're doing this to prevent the server from
 		   complaining / breaking since we don't read the whole page */
-		while (len--) {
+		while (content_len--) {
 			/* TODO: deal with EAGAIN (and other errors) better */
+			/* TODO: Reading 1 byte at a time is horrible and stupid. */
 			if (read(connect_data->fd, &tmpc, 1) < 0 && errno != EAGAIN)
 				break;
 		}