Skip to content

Commit

Permalink
http: Fix overflow on HTTP request buffers
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@cernekee
cernekee committed Oct 28, 2012
1 parent 3edcc28 commit 26f752c
Showing 1 changed file with 130 additions and 34 deletions.
164 changes: 130 additions & 34 deletions http.c
View file
Expand Up @@ -35,6 +35,7 @@
#include <errno.h>
#include <stdlib.h>
#include <stdio.h>
#include <stdarg.h>

#include "openconnect-internal.h"

Expand All @@ -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
Expand Down Expand Up @@ -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];
Expand All @@ -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) {
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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",
Expand All @@ -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;

Expand Down Expand Up @@ -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"),
Expand Down

0 comments on commit 26f752c

Please sign in to comment.