pidgin/pidgin

Improve how our HTTP proxy code reads the content-length header.
release-2.x.y
2014-01-12, Mark Doliner
dd8ee564e065
Parents 82ec5fb22ce9
Children 98f3f47f8382
Improve how our HTTP proxy code reads the content-length header.

It now reads the length into an unsigned variable. This was reported
to us by Matt Jones of Volvent. I think there's no potential for
remote code execution or remote crash--we don't allocate a buffer
for the content or anything. I think it's just ugly code. I didn't
impose a max size for the content-length. I feel like it's ok for the
while loop to loop many times then get EAGAIN and break out.

I'm looking forward to unified http parsing in 3.0.0.
2 files changed, 18 insertions(+), 4 deletions(-)
  • +2 -0
    ChangeLog
  • +16 -4
    libpurple/proxy.c
    • --- a/ChangeLog Sun Jan 12 13:08:28 2014 -0800
    +++ b/ChangeLog Sun Jan 12 14:32:15 2014 -0800
    @@ -9,6 +9,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)
    --- a/libpurple/proxy.c Sun Jan 12 13:08:28 2014 -0800
    +++ b/libpurple/proxy.c Sun Jan 12 14:32:15 2014 -0800
    @@ -985,22 +985,34 @@
    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;
    }