[PATCH 03/24] http: Fix overflow on HTTP request buffers
Kevin Cernekee
cernekee at gmail.com
Sat Nov 3 13:22:45 EDT 2012
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
More information about the openconnect-devel
mailing list