From 5574b45296e26ebdb25a8824f003d69a75b6f609 Mon Sep 17 00:00:00 2001
From: Dimitri Papadopoulos
<3234522+DimitriPapadopoulos@users.noreply.github.com>
Date: Tue, 14 Apr 2020 12:01:14 +0200
Subject: [PATCH] Code review and cleanup of http_receive()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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:
if (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.
---
src/http.c | 186 +++++++++++++++++++++++++++++++----------------------
src/http.h | 2 +
src/ssl.h | 3 +-
3 files changed, 112 insertions(+), 79 deletions(-)
diff --git a/src/http.c b/src/http.c
index 3420d5e..e32ecf9 100644
--- a/src/http.c
+++ b/src/http.c
@@ -22,6 +22,7 @@
#include "userinput.h"
#include "log.h"
+#include <assert.h>
#include <ctype.h>
#include <string.h>
#include <stdarg.h>
@@ -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.
@@ -58,6 +60,7 @@ static void url_encode(char *dest, const char *str)
*dest = '\0';
}
+
/*
* Sends data to the HTTP server.
*
@@ -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') {
@@ -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);
@@ -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,
@@ -135,6 +139,7 @@ static const char *find_header(
return NULL;
}
+
/*
* Receives data from the HTTP server.
*
@@ -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,
@@ -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,
@@ -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.
*
@@ -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 "=".
@@ -351,6 +374,7 @@ end:
return ret;
}
+
static int get_auth_cookie(
struct tunnel *tunnel,
char *buf,
@@ -390,6 +414,7 @@ static int get_auth_cookie(
return ret;
}
+
static void delay_otp(struct tunnel *tunnel)
{
if (tunnel->config->otp_delay > 0) {
@@ -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,
@@ -555,6 +580,7 @@ int try_otp_auth(
#undef SPACE_AVAILABLE
}
+
/*
* Authenticates to gateway by sending username and password.
*
@@ -684,11 +710,13 @@ end:
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);
@@ -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;
@@ -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;
@@ -828,6 +857,7 @@ int parse_config(struct tunnel *tunnel, const char *buffer)
return 1;
}
+
int auth_get_config(struct tunnel *tunnel)
{
char *buffer;
diff --git a/src/http.h b/src/http.h
index e1e1710..ec124e9 100644
--- a/src/http.h
+++ b/src/http.h
@@ -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
diff --git a/src/ssl.h b/src/ssl.h
index 879b9d4..c0fe623 100644
--- a/src/ssl.h
+++ b/src/ssl.h
@@ -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>
@@ -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";
}
--
GitLab