Content-Length bugs

[Matt Jones]

Hi there,

There are a number of instances in libpurple where Content-Length is
parsed from user-controlled data in dangerous or sloppy ways.  This is
done inconsistently throughout the code-base, primarily in protocol
plugins.

I am listing out the dangerous ways this http header is being parsed,
primarily related to the use of treating this as a signed integer via
things like %d's via sscanf, etc. or grabbing this string and running
the value through atoi().

It would be nice to see this standardized in some way, where this
header is treated as an unsigned integer, and properly validated to
avoid abuse cases - or potentailly security problems - like how you
have parse_content_len() in Util.c

Here is an example in proxy.c that shows one way this is being parsed.

/**
 * We're using an HTTP proxy for a non-port 80 tunnel.  Read the
 * response to the CONNECT request.
 */
static void
http_canread(gpointer data, gint source, PurpleInputCondition cond)
{
        int len, headers_len, status = 0; [1]
        gboolean error;
        PurpleProxyConnectData *connect_data = data;
        char *p;
        gsize max_read;

        [...]

        p = g_strrstr((const gchar *)connect_data->read_buffer,
"Content-Length: "); [2]
        if (p != NULL) {
                gchar *tmp;
                int len = 0;
                char tmpc;
                p += strlen("Content-Length: ");
                tmp = strchr(p, '\r');
                if(tmp)
                        *tmp = '\0';
                len = atoi(p); [3]
                if(tmp)
                        *tmp = '\r';

                /* Compensate for what has already been read */
                len -= connect_data->read_len - headers_len; [4]
                /* 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--) { [5]
                        /* TODO: deal with EAGAIN (and other errors) better */
                        if (read(connect_data->fd, &tmpc, 1) < 0 &&
errno != EAGAIN)
                                break;
                }
        }

[1] len is set as a signed integer
[2] The Content-Length value is grabbed from the header
[3] The value is set as a signed integer using atoi()
[4] The header is subtracted from the content-length
[5] A loop starts decrementing len each iteration

Here are all protocols that are independently parsing Content-Length,
most often than not, in dangerous or simply messy ways.

protocols/yahoo/libymsg.c:              "Content-Length: 0\r\n\r\n",
protocols/yahoo/libymsg.c:              "Content-Length: %"
G_GSIZE_FORMAT "\r\n"
protocols/yahoo/yahoo_picture.c:                "Content-Length: %"
G_GSIZE_FORMAT "\r\n"
protocols/yahoo/yahoo_aliases.c:
"Content-Length: %" G_GSIZE_FORMAT "\r\n"
protocols/yahoo/yahoo_aliases.c:
"Content-Length: %d\r\n"
protocols/yahoo/yahoo_filexfer.c:                       length =
g_strstr_len(xd->rxqueue, len, "Content-Length:");
protocols/yahoo/yahoo_filexfer.c:       tx = g_strdup_printf("HTTP/1.1
200 OK\r\nContent-Length: 0\r\nContent-Type:
application/octet-stream\r\nConnection: close\r\n\r\n");
protocols/yahoo/yahoo_filexfer.c:
         "Content-Length: %ld\r\n"
protocols/yahoo/yahoo_filexfer.c:
         "Content-Length: %ld\r\n"
protocols/yahoo/yahoo_filexfer.c:
         "Content-Length: 0\r\n"
protocols/yahoo/yahoo_filexfer.c:
         "Content-Length: 0\r\n"
protocols/jabber/bosh.c:                const char *content_length =
purple_strcasestr(cursor, "\r\nContent-Length:");
protocols/jabber/bosh.c:                /* Make sure Content-Length is
in headers, not body */
protocols/jabber/bosh.c:                                 * The packet
ends in the middle of the Content-Length line.
protocols/jabber/bosh.c:                        len =
atoi(content_length + strlen("\r\nContent-Length:"));
protocols/jabber/bosh.c:
purple_debug_warning("jabber", "Found mangled Content-Length header,
or server returned 0-length response.\n");
protocols/jabber/bosh.c:        /* Have we read all that the
Content-Length promised us? */
protocols/jabber/bosh.c:
"Content-Length: %" G_GSIZE_FORMAT "\r\n\r\n"
protocols/jabber/oob.c:                 lenstr =
strstr(jox->headers->str, "Content-Length: ");
protocols/jabber/oob.c:                         sscanf(lenstr,
"Content-Length: %d", &size);
protocols/simple/simple.c:      sipmsg_remove_header(msg, "Content-Length");
protocols/simple/simple.c:              sipmsg_add_header(msg,
"Content-Length", len);
protocols/simple/simple.c:              sipmsg_add_header(msg,
"Content-Length", "0");
protocols/simple/simple.c:                      "Content-Length: %"
G_GSIZE_FORMAT "\r\n\r\n%s",
protocols/simple/sipmsg.c:      tmp2 = sipmsg_find_header(msg,
"Content-Length");
protocols/gg/lib/pubdir.c:              "Content-Length: %d\r\n"
protocols/gg/lib/pubdir.c:              "Content-Length: %d\r\n"
protocols/gg/lib/pubdir.c:              "Content-Length: %d\r\n"
protocols/gg/lib/pubdir.c:              "Content-Length: %d\r\n"
protocols/gg/lib/pubdir.c:              "Content-Length: 0\r\n"
protocols/mxit/protocol.c:
"Content-Length: %d\r\n"
protocols/mxit/http.c:#define           HTTP_CONTENT_LEN
"Content-Length: "
protocols/oscar/clientlogin.c:  g_string_append_printf(request,
"Content-Length: %" G_GSIZE_FORMAT "\r\n\r\n", body->len);
protocols/msn/httpconn.c:       if ((s = purple_strcasestr(header,
"Content-Length: ")) != NULL)
protocols/msn/httpconn.c:               s += strlen("Content-Length: ");
protocols/msn/httpconn.c:               "Content-Length: 0\r\n\r\n",
protocols/msn/httpconn.c:               "Content-Length: %d\r\n\r\n",
protocols/msn/slpmsg.c:         "Content-Length: %" G_GSIZE_FORMAT "\r\n"
protocols/msn/slpcall.c:                temp = get_token(body,
"Content-Length: ", "\r\n");
protocols/msn/soap.c:                           } else if (strcmp(key,
"Content-Length") == 0) {
protocols/msn/soap.c:                           "Content-Length: %d\r\n"


I understand these are all except 1 instance in protocols/*.c files,
so it's a bit tedious to go changing this, however I think it's an
unnecessary attack surface and it would be helpful to properly
standardize this.

Thanks for your time,

Matt