Skip to content

Commit

Permalink
Code review and cleanup of http_receive()
Browse files Browse the repository at this point in the history 
Improve readabilty by moving conditionals that operate on 'n'
right after assignment of 'n'. The conditional at the end of
the loop:
	while (n > = 0)
has been replaced by this conditional:
	if (n < = 0)
and this one that triggers a tight 2nd loop:
	(n == ERR_SSL_AGAIN) ⇔ if (n == 0)

The new codes *returns from the function* with ERR_HTTP_SSL upon:
	if (n < 0)
The previous code would only *leave the loop* upon:
	if (n < 0)
and then return from the function with ERR_HTTP_SSL only upon:
	if (!header)
Therefore SSL violations were silently ignored after reading
the header and while reading the body of the HTTP response.

Increase the HTTP buffer capacity when needed. You never know.
The previous size of 32 KB used to work well from 2015 to 2020.
In 2020 a case was reported where 32 KB were not enough anymore
and we increased the buffer size to 64 KB. Someday 64 KB might
not be enough either. Yet in most cases 32 KB are more than
enough. So start with 32 KB and increase well beyond 64 KB if
needed, instead of bailing out with ERR_HTTP_SSL.

These changes will help introduce 3rd party HTTP parsing code
if we decide to go that way.
  • Loading branch information
@DimitriPapadopoulos
DimitriPapadopoulos committed Apr 15, 2020
1 parent 8b0c235 commit f608fce
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 79 deletions.
186 changes: 108 additions & 78 deletions src/http.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "userinput.h"
#include "log.h"

#include <assert.h>
#include <ctype.h>
#include <string.h>
#include <stdarg.h>
Expand All @@ -30,7 +31,8 @@
#include <unistd.h>
#include <arpa/inet.h>

#define BUFSZ 0x10000
static const uint32_t HTTP_BUFFER_SIZE = 0x8000;


/*
* URL-encodes a string for HTTP requests.
Expand Down Expand Up @@ -58,6 +60,7 @@ static void url_encode(char *dest, const char *str)
*dest = '\0';
}


/*
* Sends data to the HTTP server.
*
Expand All @@ -68,13 +71,13 @@ static void url_encode(char *dest, const char *str)
int http_send(struct tunnel *tunnel, const char *request, ...)
{
va_list args;
char buffer[BUFSZ];
char logbuffer[BUFSZ];
char buffer[HTTP_BUFFER_SIZE];
char logbuffer[HTTP_BUFFER_SIZE];
int length;
int n = 0;

va_start(args, request);
length = vsnprintf(buffer, BUFSZ, request, args);
length = vsnprintf(buffer, HTTP_BUFFER_SIZE, request, args);
va_end(args);
strcpy(logbuffer, buffer);
if (loglevel <= OFV_LOG_DEBUG_DETAILS && tunnel->config->password[0] != '\0') {
Expand All @@ -96,7 +99,7 @@ int http_send(struct tunnel *tunnel, const char *request, ...)

if (length < 0)
return ERR_HTTP_INVALID;
else if (length >= BUFSZ)
else if (length >= HTTP_BUFFER_SIZE)
return ERR_HTTP_TOO_LONG;

log_debug_details("%s:\n%s\n", __func__, logbuffer);
Expand All @@ -113,6 +116,7 @@ int http_send(struct tunnel *tunnel, const char *request, ...)
return 1;
}


static const char *find_header(
const char *res,
const char *header,
Expand All @@ -135,6 +139,7 @@ static const char *find_header(
return NULL;
}


/*
* Receives data from the HTTP server.
*
Expand All @@ -150,75 +155,87 @@ int http_receive(
uint32_t *response_size
)
{
uint32_t res_size = BUFSZ;
char *buffer, *res;
int n = 0;
int bytes_read = 0;
int header_size = 0;
int content_size = 0;
uint32_t capacity = HTTP_BUFFER_SIZE;
char *buffer;
uint32_t bytes_read = 0;
uint32_t header_size = 0;
uint32_t content_size = 0;
int chunked = 0;

buffer = malloc(res_size);
buffer = malloc(capacity);
if (buffer == NULL)
return ERR_HTTP_NO_MEM;

do {
n = safe_ssl_read(tunnel->ssl_handle,
(uint8_t *) buffer + bytes_read,
BUFSZ - 1 - bytes_read);
if (n > 0) {
log_debug_details("%s:\n%s\n", __func__, buffer);
const char *eoh;

bytes_read += n;

if (!header_size) {
/* Did we see the header end? Then get the body size. */
eoh = memmem(buffer, bytes_read, "\r\n\r\n", 4);
if (eoh) {
const char *header;

header = find_header(
buffer,
"Content-Length: ", BUFSZ);
header_size = eoh - buffer + 4;
if (header)
content_size = atoi(header);

if (find_header(buffer,
"Transfer-Encoding: chunked",
BUFSZ))
chunked = 1;
}
while (1) {
int n;

while ((n = safe_ssl_read(tunnel->ssl_handle,
(uint8_t *) buffer + bytes_read,
capacity - bytes_read)) == ERR_SSL_AGAIN)
;
if (n < 0) {
log_error("Error reading from SSL connection (%s).\n",
err_ssl_str(n));
free(buffer);
return ERR_HTTP_SSL;
}
bytes_read += n;

log_debug_details("%s:\n%s\n", __func__, buffer);

if (!header_size) {
/* Have we reached the end of the HTTP header? */
static const char EOH[4] = "\r\n\r\n";
const char *eoh = memmem(buffer, bytes_read,
EOH, sizeof(EOH));

if (eoh) {
header_size = eoh - buffer + sizeof(EOH);

/* Get the body size. */
const char *header = find_header(buffer,
"Content-Length: ",
header_size);

if (header)
content_size = atoi(header);

if (find_header(buffer,
"Transfer-Encoding: chunked",
header_size))
chunked = 1;
}
}

if (header_size) {
/* We saw the whole header, is the body done as well? */
if (chunked) {
/* Last chunk terminator. Done naively. */
if (bytes_read >= 7 &&
!memcmp(&buffer[bytes_read - 7],
"\r\n0\r\n\r\n", 7))
break;
} else {
if (bytes_read >= header_size + content_size)
break;
}
if (header_size) {
/* Have we reached the end of the HTTP body? */
if (chunked) {
static const char EOB[7] = "\r\n0\r\n\r\n";

/* Last chunk terminator. Done naively. */
if (bytes_read >= sizeof(EOB) &&
!memcmp(&buffer[bytes_read - sizeof(EOB)],
EOB, sizeof(EOB)))
break;
} else {
if (bytes_read >= header_size + content_size)
break;
}
}

if (bytes_read == BUFSZ - 1) {
log_warn("Response too big\n");
/* expand the buffer if necessary */
if (bytes_read == capacity) {
char *new_buffer;

assert(UINT32_MAX / capacity >= 2);
capacity *= 2;
new_buffer = realloc(buffer, capacity);
if (new_buffer == NULL) {
free(buffer);
return ERR_HTTP_SSL;
return ERR_HTTP_NO_MEM;
}
buffer = new_buffer;
}
} while (n >= 0);

if (!header_size) {
log_debug("Error reading from SSL connection (%s).\n",
err_ssl_str(n));
free(buffer);
return ERR_HTTP_SSL;
}

if (memmem(&buffer[header_size], bytes_read - header_size,
Expand All @@ -231,23 +248,26 @@ int http_receive(

if (response == NULL) {
free(buffer);
return 1;
}
} else {
char *tmp;

assert(capacity < UINT32_MAX);
capacity = bytes_read + 1;
tmp = realloc(buffer, capacity);
if (tmp == NULL) {
free(buffer);
return ERR_HTTP_NO_MEM;
}
tmp[bytes_read] = '\0';
*response = tmp;

res_size = bytes_read + 1;
res = realloc(buffer, res_size);
if (res == NULL) {
free(buffer);
return ERR_HTTP_NO_MEM;
if (response_size != NULL)
*response_size = capacity;
}
res[bytes_read] = '\0';

*response = res;
if (response_size != NULL)
*response_size = res_size;
return 1;
}


static int do_http_request(struct tunnel *tunnel,
const char *method,
const char *uri,
Expand Down Expand Up @@ -276,6 +296,8 @@ static int do_http_request(struct tunnel *tunnel,

return http_receive(tunnel, response, response_size);
}


/*
* Sends and receives data from the HTTP server.
*
Expand Down Expand Up @@ -307,6 +329,7 @@ static int http_request(struct tunnel *tunnel, const char *method,
return ret;
}


/*
* Read value for key from a string like "key1=value1&key2=value2".
* The `key` arg is supposed to contains the final "=".
Expand Down Expand Up @@ -351,6 +374,7 @@ static int get_value_from_response(const char *buf, const char *key,
return ret;
}


static int get_auth_cookie(
struct tunnel *tunnel,
char *buf,
Expand Down Expand Up @@ -390,6 +414,7 @@ static int get_auth_cookie(
return ret;
}


static void delay_otp(struct tunnel *tunnel)
{
if (tunnel->config->otp_delay > 0) {
Expand All @@ -398,8 +423,8 @@ static void delay_otp(struct tunnel *tunnel)
}
}

static
int try_otp_auth(

static int try_otp_auth(
struct tunnel *tunnel,
const char *buffer,
char **res,
Expand Down Expand Up @@ -555,6 +580,7 @@ int try_otp_auth(
#undef SPACE_AVAILABLE
}


/*
* Authenticates to gateway by sending username and password.
*
Expand Down Expand Up @@ -684,11 +710,13 @@ int auth_log_in(struct tunnel *tunnel)
return ret;
}


int auth_log_out(struct tunnel *tunnel)
{
return http_request(tunnel, "GET", "/remote/logout", "", NULL, NULL);
}


int auth_request_vpn_allocation(struct tunnel *tunnel)
{
int ret = http_request(tunnel, "GET", "/remote/index", "", NULL, NULL);
Expand All @@ -699,6 +727,7 @@ int auth_request_vpn_allocation(struct tunnel *tunnel)
return http_request(tunnel, "GET", "/remote/fortisslvpn", "", NULL, NULL);
}


static int parse_xml_config(struct tunnel *tunnel, const char *buffer)
{
const char *val;
Expand Down Expand Up @@ -778,8 +807,8 @@ static int parse_xml_config(struct tunnel *tunnel, const char *buffer)
return 1;
}

static
int parse_config(struct tunnel *tunnel, const char *buffer)

static int parse_config(struct tunnel *tunnel, const char *buffer)
{
const char *c, *end;

Expand Down Expand Up @@ -828,6 +857,7 @@ int parse_config(struct tunnel *tunnel, const char *buffer)
return 1;
}


int auth_get_config(struct tunnel *tunnel)
{
char *buffer;
Expand Down
2 changes: 2 additions & 0 deletions src/http.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

#include "tunnel.h"

#include <stdint.h>

#define ERR_HTTP_INVALID -1
#define ERR_HTTP_TOO_LONG -2
#define ERR_HTTP_NO_MEM -3
Expand Down
3 changes: 2 additions & 1 deletion src/ssl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#define OPENFORTIVPN_SSL_H

#include <errno.h>
#include <stdint.h>
#include <string.h>
#include <openssl/err.h>
#include <openssl/ssl.h>
Expand Down Expand Up @@ -76,7 +77,7 @@ static inline const char *err_ssl_str(int code)
else if (code == ERR_SSL_SEE_ERRNO)
return strerror(errno);
else if (code == ERR_SSL_SEE_SSLERR)
return ERR_error_string(ERR_peek_last_error(), NULL);
return ERR_reason_error_string(ERR_peek_last_error());
return "unknown";
}

Expand Down

0 comments on commit f608fce

Please sign in to comment.