http: Fix overflow on HTTP request buffers

A malicious VPN gateway can send a very long hostname/path (for redirects)
or cookie list (in general), which OpenConnect will attempt to sprintf()
into a fixed length buffer.  Each HTTP server response line can add
roughly MAX_BUF_LEN (131072) bytes to the next OpenConnect HTTP request,
but the request buffer (buf) is capped at MAX_BUF_LEN bytes and is
allocated on the stack.

The result of passing a long "Location:" header looks like:

    Attempting to connect to server 127.0.0.1:443
    SSL negotiation with localhost
    Server certificate verify failed: self signed certificate in certificate chain
    Connected to HTTPS on localhost
    GET https://localhost/
    Got HTTP response: HTTP/1.0 301 Moved
    Ignoring unknown HTTP response line 'aaaaaaaaaaaaaaaaaa'
    SSL negotiation with localhost
    Server certificate verify failed: self signed certificate in certificate chain
    Connected to HTTPS on localhost
    *** buffer overflow detected ***: /scr/openconnect2/.libs/lt-openconnect terminated
    ======= Backtrace: =========
    /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7fd62729b82c]
    /lib/x86_64-linux-gnu/libc.so.6(+0x109700)[0x7fd62729a700]
    /lib/x86_64-linux-gnu/libc.so.6(+0x108b69)[0x7fd627299b69]
    /lib/x86_64-linux-gnu/libc.so.6(_IO_default_xsputn+0xdd)[0x7fd62720d13d]
    /lib/x86_64-linux-gnu/libc.so.6(_IO_vfprintf+0x1ae7)[0x7fd6271db4a7]
    /lib/x86_64-linux-gnu/libc.so.6(__vsprintf_chk+0x94)[0x7fd627299c04]
    /lib/x86_64-linux-gnu/libc.so.6(__sprintf_chk+0x7d)[0x7fd627299b4d]
    /scr/openconnect2/.libs/libopenconnect.so.2(openconnect_obtain_cookie+0xc0)[0x7fd62832d210]
    /scr/openconnect2/.libs/lt-openconnect[0x40413f]
    /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x7fd6271b276d]
    /scr/openconnect2/.libs/lt-openconnect[0x404579]

The proposed fix is to use dynamically allocated buffers with overflow
checking.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>

diff --git a/http.c b/http.c
index 1df648873dba30d574963b43895d882f0c146eef..d49f26fac4ac548eacd04fd26d11efd70394285c 100644 (file)
--- a/http.c
+++ b/http.c
@@ -35,6 +35,7 @@
 #include <errno.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <stdarg.h>
 
 #include "openconnect-internal.h"
 
@@ -44,6 +45,85 @@ static int proxy_read(struct openconnect_info *vpninfo, int fd,
                      unsigned char *buf, size_t len);
 
 #define MAX_BUF_LEN 131072
+#define BUF_CHUNK_SIZE 4096
+
+struct oc_text_buf {
+       char *data;
+       int pos;
+       int buf_len;
+       int error;
+};
+
+static struct oc_text_buf *buf_alloc(void)
+{
+       return calloc(1, sizeof(struct oc_text_buf));
+}
+
+static void buf_append(struct oc_text_buf *buf, const char *fmt, ...)
+{
+       va_list ap;
+
+       if (!buf || buf->error)
+               return;
+
+       if (!buf->data) {
+               buf->data = malloc(BUF_CHUNK_SIZE);
+               if (!buf->data) {
+                       buf->error = -ENOMEM;
+                       return;
+               }
+               buf->buf_len = BUF_CHUNK_SIZE;
+       }
+
+       while (1) {
+               int max_len = buf->buf_len - buf->pos, ret;
+
+               va_start(ap, fmt);
+               ret = vsnprintf(buf->data + buf->pos, max_len, fmt, ap);
+               va_end(ap);
+               if (ret < 0) {
+                       buf->error = -EIO;
+                       break;
+               } else if (ret < max_len) {
+                       buf->pos += ret;
+                       break;
+               } else {
+                       int new_buf_len = buf->buf_len + BUF_CHUNK_SIZE;
+
+                       if (new_buf_len > MAX_BUF_LEN) {
+                               /* probably means somebody is messing with us */
+                               buf->error = -E2BIG;
+                               break;
+                       }
+
+                       buf->data = realloc(buf->data, new_buf_len);
+                       if (!buf->data) {
+                               buf->error = -ENOMEM;
+                               break;
+                       }
+                       buf->buf_len = new_buf_len;
+               }
+       }
+}
+
+static int buf_error(struct oc_text_buf *buf)
+{
+       return buf ? buf->error : -ENOMEM;
+}
+
+static int buf_free(struct oc_text_buf *buf)
+{
+       int error = buf_error(buf);
+
+       if (buf) {
+               if (buf->data)
+                       free(buf->data);
+               free(buf);
+       }
+
+       return error;
+}
+
 /*
  * We didn't really want to have to do this for ourselves -- one might have
  * thought that it would be available in a library somewhere. But neither
@@ -347,7 +427,7 @@ static int fetch_config(struct openconnect_info *vpninfo, char *fu, char *bu,
                        char *server_sha1)
 {
        struct vpn_option *opt;
-       char buf[MAX_BUF_LEN];
+       struct oc_text_buf *buf;
        char *config_buf = NULL;
        int result, buflen;
        unsigned char local_sha1_bin[SHA1_SIZE];
@@ -361,25 +441,31 @@ static int fetch_config(struct openconnect_info *vpninfo, char *fu, char *bu,
                return -EINVAL;
        }
 
-       sprintf(buf, "GET %s%s HTTP/1.1\r\n", fu, bu);
-       sprintf(buf + strlen(buf), "Host: %s\r\n", vpninfo->hostname);
-       sprintf(buf + strlen(buf),  "User-Agent: %s\r\n", vpninfo->useragent);
-       sprintf(buf + strlen(buf),  "Accept: */*\r\n");
-       sprintf(buf + strlen(buf),  "Accept-Encoding: identity\r\n");
+       buf = buf_alloc();
+       buf_append(buf, "GET %s%s HTTP/1.1\r\n", fu, bu);
+       buf_append(buf, "Host: %s\r\n", vpninfo->hostname);
+       buf_append(buf, "User-Agent: %s\r\n", vpninfo->useragent);
+       buf_append(buf, "Accept: */*\r\n");
+       buf_append(buf, "Accept-Encoding: identity\r\n");
 
        if (vpninfo->cookies) {
-               sprintf(buf + strlen(buf),  "Cookie: ");
+               buf_append(buf, "Cookie: ");
                for (opt = vpninfo->cookies; opt; opt = opt->next)
-                       sprintf(buf + strlen(buf),  "%s=%s%s", opt->option,
+                       buf_append(buf, "%s=%s%s", opt->option,
                                      opt->value, opt->next ? "; " : "\r\n");
        }
-       sprintf(buf + strlen(buf),  "X-Transcend-Version: 1\r\n\r\n");
+       buf_append(buf, "X-Transcend-Version: 1\r\n\r\n");
+
+       if (buf_error(buf))
+               return buf_free(buf);
 
-       if (openconnect_SSL_write(vpninfo, buf, strlen(buf)) != strlen(buf)) {
+       if (openconnect_SSL_write(vpninfo, buf->data, buf->pos) != buf->pos) {
                vpn_progress(vpninfo, PRG_ERR,
                             _("Failed to send GET request for new config\n"));
+               buf_free(buf);
                return -EIO;
        }
+       buf_free(buf);
 
        buflen = process_http_response(vpninfo, &result, NULL, &config_buf);
        if (buflen < 0) {
@@ -697,7 +783,7 @@ static int handle_redirect(struct openconnect_info *vpninfo)
 int openconnect_obtain_cookie(struct openconnect_info *vpninfo)
 {
        struct vpn_option *opt;
-       char buf[MAX_BUF_LEN];
+       struct oc_text_buf *buf;
        char *form_buf = NULL;
        int result, buflen;
        char request_body[2048];
@@ -731,27 +817,26 @@ int openconnect_obtain_cookie(struct openconnect_info *vpninfo)
         *
         * So we process the HTTP for ourselves...
         */
-       sprintf(buf, "%s /%s HTTP/1.1\r\n", method, vpninfo->urlpath ?: "");
-       sprintf(buf + strlen(buf), "Host: %s\r\n", vpninfo->hostname);
-       sprintf(buf + strlen(buf),  "User-Agent: %s\r\n", vpninfo->useragent);
-       sprintf(buf + strlen(buf),  "Accept: */*\r\n");
-       sprintf(buf + strlen(buf),  "Accept-Encoding: identity\r\n");
+       buf = buf_alloc();
+       buf_append(buf, "%s /%s HTTP/1.1\r\n", method, vpninfo->urlpath ?: "");
+       buf_append(buf, "Host: %s\r\n", vpninfo->hostname);
+       buf_append(buf, "User-Agent: %s\r\n", vpninfo->useragent);
+       buf_append(buf, "Accept: */*\r\n");
+       buf_append(buf, "Accept-Encoding: identity\r\n");
 
        if (vpninfo->cookies) {
-               sprintf(buf + strlen(buf),  "Cookie: ");
+               buf_append(buf, "Cookie: ");
                for (opt = vpninfo->cookies; opt; opt = opt->next)
-                       sprintf(buf + strlen(buf),  "%s=%s%s", opt->option,
-                                     opt->value, opt->next ? "; " : "\r\n");
+                       buf_append(buf, "%s=%s%s", opt->option,
+                                  opt->value, opt->next ? "; " : "\r\n");
        }
        if (request_body_type) {
-               sprintf(buf + strlen(buf),  "Content-Type: %s\r\n",
-                             request_body_type);
-               sprintf(buf + strlen(buf),  "Content-Length: %zd\r\n",
-                             strlen(request_body));
+               buf_append(buf, "Content-Type: %s\r\n", request_body_type);
+               buf_append(buf, "Content-Length: %zd\r\n", strlen(request_body));
        }
-       sprintf(buf + strlen(buf),  "X-Transcend-Version: 1\r\n\r\n");
+       buf_append(buf, "X-Transcend-Version: 1\r\n\r\n");
        if (request_body_type)
-               sprintf(buf + strlen(buf), "%s", request_body);
+               buf_append(buf, "%s", request_body);
 
        if (vpninfo->port == 443)
                vpn_progress(vpninfo, PRG_INFO, "%s https://%s/%s\n",
@@ -762,7 +847,11 @@ int openconnect_obtain_cookie(struct openconnect_info *vpninfo)
                             method, vpninfo->hostname, vpninfo->port,
                             vpninfo->urlpath ?: "");
 
-       result = openconnect_SSL_write(vpninfo, buf, strlen(buf));
+       if (buf_error(buf))
+               return buf_free(buf);
+
+       result = openconnect_SSL_write(vpninfo, buf->data, buf->pos);
+       buf_free(buf);
        if (result < 0)
                return result;
 
@@ -1095,21 +1184,28 @@ static int process_socks_proxy(struct openconnect_info *vpninfo, int ssl_sock)
 static int process_http_proxy(struct openconnect_info *vpninfo, int ssl_sock)
 {
        char buf[MAX_BUF_LEN];
+       struct oc_text_buf *reqbuf;
        int buflen, result;
 
-       sprintf(buf, "CONNECT %s:%d HTTP/1.1\r\n", vpninfo->hostname, vpninfo->port);
-       sprintf(buf + strlen(buf), "Host: %s\r\n", vpninfo->hostname);
-       sprintf(buf + strlen(buf), "User-Agent: %s\r\n", vpninfo->useragent);
-       sprintf(buf + strlen(buf), "Proxy-Connection: keep-alive\r\n");
-       sprintf(buf + strlen(buf), "Connection: keep-alive\r\n");
-       sprintf(buf + strlen(buf), "Accept-Encoding: identity\r\n");
-       sprintf(buf + strlen(buf), "\r\n");
+       reqbuf = buf_alloc();
+       buf_append(reqbuf, "CONNECT %s:%d HTTP/1.1\r\n", vpninfo->hostname, vpninfo->port);
+       buf_append(reqbuf, "Host: %s\r\n", vpninfo->hostname);
+       buf_append(reqbuf, "User-Agent: %s\r\n", vpninfo->useragent);
+       buf_append(reqbuf, "Proxy-Connection: keep-alive\r\n");
+       buf_append(reqbuf, "Connection: keep-alive\r\n");
+       buf_append(reqbuf, "Accept-Encoding: identity\r\n");
+       buf_append(reqbuf, "\r\n");
+
+       if (buf_error(reqbuf))
+               return buf_free(reqbuf);
 
        vpn_progress(vpninfo, PRG_INFO,
                     _("Requesting HTTP proxy connection to %s:%d\n"),
                     vpninfo->hostname, vpninfo->port);
 
-       result = proxy_write(vpninfo, ssl_sock, (unsigned char *)buf, strlen(buf));
+       result = proxy_write(vpninfo, ssl_sock, (unsigned char *)reqbuf->data, reqbuf->pos);
+       buf_free(reqbuf);
+
        if (result) {
                vpn_progress(vpninfo, PRG_ERR,
                             _("Sending proxy request failed: %s\n"),