[PATCH 03/24] http: Fix overflow on HTTP request buffers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 at gmail.com>
---
 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 <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"),
-- 
1.7.10.4




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux