From 26f752c3dbf69227679fc6bebb4ae071aecec491 Mon Sep 17 00:00:00 2001 From: Kevin Cernekee Date: Sat, 27 Oct 2012 12:25:50 -0700 Subject: [PATCH] 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 --- http.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 130 insertions(+), 34 deletions(-) diff --git a/http.c b/http.c index 1df6488..d49f26f 100644 --- a/http.c +++ b/http.c @@ -35,6 +35,7 @@ #include #include #include +#include #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"), -- 2.7.4