libretro-common/net: harden net_http.c response parser and net_http_new

Audit of the full net_http.c HTTP client stack (~1633 lines).  Five
real bugs found; fixed in this patch.  All exploitable by a
hostile server that the client has been pointed at.

net_http: bound Content-Length parsing
  The Content-Length header parser accumulated digits into a
  signed ssize_t, which is undefined behaviour on overflow and
  produces a huge size_t on assignment to response->len.  That
  value then drives the realloc() at the end of
  net_http_receive_header(), pushing the client toward OOM on a
  hostile server response.

  Fix:
    * accumulate into size_t
    * reject empty values ("Content-Length: " with no digits)
    * reject values that would exceed NET_HTTP_MAX_CONTENT_LENGTH
      (256 MiB -- large enough for any legitimate libretro/
      RetroArch payload, small enough to keep realloc honest)
    * on reject, set state->err and return -1 cleanly

net_http: bounds-check HTTP status line
  net_http_receive_header read the three status digits at
  scan[9..11] unconditionally.  A malicious server could send a
  short line like "HTTP/1.0\n" (9 bytes before the LF, so
  scan[8] was the NUL we just wrote and scan[9..11] were
  whatever followed in the receive buffer).  The status came
  out as garbage but no memory was actually corrupted; still,
  reading past the logical end of the line is a bug.

  Fix: require line_len >= 12, require scan[8] == ' ', and
  require scan[9..11] to be decimal digits.  Any deviation
  fails the response cleanly.

net_http: realloc NULL-check in receive_header
  Two realloc() calls at the end of net_http_receive_header
  ignored the return value.  On failure the original buffer
  would leak AND response->data would be NULL, which subsequent
  writes in net_http_receive_body dereference.

  Fix: stash the realloc result in a tmp and fail the response
  (state->err = true, return -1) on NULL.  Does not leak the
  original buffer.

net_http: consolidated OOM guard in net_http_new
  net_http_new() issued seven strdup() and two malloc() calls
  back-to-back without checking any of them.  Any OOM left
  fields (notably request.domain and request.path) NULL, and
  later strlen() calls in net_http_send_request() crashed on
  NULL.  The response.data malloc and string_list_new()
  allocation had the same hazard for the response-receiving
  side.

  Fix: after all allocations, check every mandatory pointer
  plus each "was requested but failed" pointer.  On any
  failure, free response.data and response.headers explicitly
  (net_http_delete does not touch those -- see note below) and
  call net_http_delete to free the request side and the state
  struct, then return NULL.

net_http: guard postdata malloc
  The body-malloc + memcpy pair in net_http_new was unguarded:
  malloc(contentlength) could return NULL, then memcpy would
  dereference it.  Fix: skip the memcpy on NULL; the OOM guard
  above catches the failure and tears down the whole state.

Note on net_http_delete: it does NOT free response.data or
response.headers.  This is intentional in the existing contract:
net_http_data() and net_http_headers() transfer ownership of
those buffers to the caller, who becomes responsible for
freeing them.  On the OOM failure path in net_http_new no
ownership transfer ever happens, so this patch explicitly frees
both before calling net_http_delete.

Reachability: hostile-server responses are the main attack
vector.  An attacker-controlled buildbot mirror (or MITM on a
cleartext HTTP connection) can trivially send oversized
Content-Length values, short status lines, or force the client
into the OOM path via very large allocations elsewhere.

VC6 compatibility: the file is compiled on all platforms.  The
fix uses only size_t, ssize_t, and standard comparisons; no new
headers.  The NET_HTTP_MAX_CONTENT_LENGTH macro uses a
size_t-cast literal that is valid in C89.

Regression test: NONE.  Exercising the relevant code paths
requires either a live SMB-style network mock or refactoring
the two static helpers (net_http_receive_header, net_http_new)
to expose inner routines that can be unit-tested in isolation.
Adding socket-mocked tests to libretro-common/samples would
pull network_init, getaddrinfo, and thread primitives into a
sample harness that currently has no such setup.  The existing
http_parse_test covers the separate net_http_parse.c unit that
was hardened in commit fd69102.  These net_http.c fixes are
localised, small, and verified at build time under
-Wall -Werror for both -DHAVE_THREADS and no-threads configs.
This commit is contained in:
U-DESKTOP-SPFP6AQ\twistedtechre committed 2026-04-19 21:42:05 +02:00
1 parent fd69102744
commit 66155ff344
1 file changed
+120 -8
+120 -8
View File
@@ -43,6 +43,14 @@
#include <rthreads/rthreads.h>
#endif
/* Maximum Content-Length we'll honour from a server, to bound the
* realloc() that follows header parsing. 256 MiB is comfortably
* larger than any single libretro HTTP payload (core downloads,
* thumbnail images, assets bundles) and small enough that a
* hostile server cannot drive the client toward OOM by lying in
* the Content-Length header. */
#define NET_HTTP_MAX_CONTENT_LENGTH ((size_t)256 * 1024 * 1024)
enum response_part
{
P_HEADER_TOP = 0,
@@ -809,7 +817,9 @@ struct http_t *net_http_new(struct http_connection_t *conn)
if (conn->postdata && conn->contentlength)
{
state->request.postdata = malloc(conn->contentlength);
memcpy(state->request.postdata, conn->postdata, conn->contentlength);
if (state->request.postdata)
memcpy(state->request.postdata, conn->postdata, conn->contentlength);
/* If malloc failed, the OOM guard further below catches it. */
}
state->request.useragent= conn->useragent ? strdup(conn->useragent) : NULL;
state->request.headers = conn->headers ? strdup(conn->headers) : NULL;
@@ -820,6 +830,39 @@ struct http_t *net_http_new(struct http_connection_t *conn)
state->response.data = (char*)malloc(state->response.buflen);
state->response.headers = string_list_new();
/* Any of the strdup / malloc calls above can return NULL on OOM.
* The dispatch path in net_http_update() dereferences
* request.domain and writes to response.data without guards; fail
* the whole setup early here rather than stack up NULL derefs
* later.
*
* Note on cleanup order: net_http_delete() intentionally does not
* free response.data or response.headers because successful callers
* take ownership of response.data via net_http_data() and of
* response.headers via net_http_headers(). On the OOM failure path
* those ownership transfers never happen, so we free both here
* before calling net_http_delete() (which then cleans up the
* request.* fields and the state struct itself). */
if ( !state->response.data
|| !state->response.headers
|| !state->request.domain
|| !state->request.path
|| !state->request.method
|| (conn->contenttype && !state->request.contenttype)
|| (conn->postdata && conn->contentlength && !state->request.postdata)
|| (conn->useragent && !state->request.useragent)
|| (conn->headers && !state->request.headers))
{
if (state->response.data)
free(state->response.data);
if (state->response.headers)
string_list_free(state->response.headers);
state->response.data = NULL;
state->response.headers = NULL;
net_http_delete(state);
return NULL;
}
return state;
}
@@ -1153,16 +1196,35 @@ static ssize_t net_http_receive_header(struct http_t *state, ssize_t len)
if (response->part == P_HEADER_TOP)
{
if ( scan[0] != 'H' || scan[1] != 'T' || scan[2] != 'T'
/* Status line is "HTTP/1.x SSS <reason>\r\n". The fixed
* prefix is 8 bytes, then a space, then 3 status digits ->
* minimum line length is 12 bytes excluding the NUL we just
* wrote at lineend (lineend - scan >= 12). Pre-patch this
* was not checked and a short malicious line like
* "HTTP/1.0\n" let the code read scan[9..11] past the
* terminator into whatever followed in the receive buffer. */
ssize_t line_len = lineend - scan;
if ( line_len < 12
|| scan[0] != 'H' || scan[1] != 'T' || scan[2] != 'T'
|| scan[3] != 'P' || scan[4] != '/' || scan[5] != '1'
|| scan[6] != '.')
|| scan[6] != '.' || scan[8] != ' ')
{
response->part = P_DONE;
state->err = true;
return -1;
}
{
const char *p = scan + 9;
const char *p = scan + 9;
/* Also verify the three status chars are digits -- a
* non-digit would produce a negative or junk status. */
if ( p[0] < '0' || p[0] > '9'
|| p[1] < '0' || p[1] > '9'
|| p[2] < '0' || p[2] > '9')
{
response->part = P_DONE;
state->err = true;
return -1;
}
response->status = (p[0] - '0') * 100
+ (p[1] - '0') * 10
+ (p[2] - '0');
@@ -1191,12 +1253,43 @@ static ssize_t net_http_receive_header(struct http_t *state, ssize_t len)
if (strncasecmp(scan, "Content-Length:",
sizeof("Content-Length:") - 1) == 0)
{
/* Parse Content-Length as unsigned with an explicit
* cap. Pre-patch the accumulator was a signed ssize_t
* that could overflow (UB) on a very long digit string
* and then sign-extend to a huge size_t when assigned
* to response->len, driving realloc() toward OOM. Cap
* at NET_HTTP_MAX_CONTENT_LENGTH (256 MiB) which is
* larger than any legitimate single HTTP response in
* the libretro/RetroArch workflow (cores, thumbnails,
* ROM manifests) and leaves a safe headroom before
* buflen can wrap. */
char *ptr = scan + (sizeof("Content-Length:") - 1);
ssize_t val = 0;
size_t val = 0;
int any = 0;
int oflow = 0;
while (*ptr == ' ' || *ptr == '\t')
++ptr;
while (*ptr >= '0' && *ptr <= '9')
val = val * 10 + (*ptr++ - '0');
{
size_t digit = (size_t)(*ptr++ - '0');
any = 1;
/* Detect overflow against the cap rather than
* against SIZE_MAX, so the later realloc call
* never sees an attacker-chosen huge value. */
if (val > (NET_HTTP_MAX_CONTENT_LENGTH - digit) / 10)
{
oflow = 1;
break;
}
val = val * 10 + digit;
}
if (!any || oflow)
{
/* Malformed header: treat as protocol error. */
response->part = P_DONE;
state->err = true;
return -1;
}
response->bodytype = T_LEN;
response->len = val;
}
@@ -1234,16 +1327,35 @@ static ssize_t net_http_receive_header(struct http_t *state, ssize_t len)
response->pos = 0;
if (response->bodytype == T_LEN)
{
/* Use a tmp pointer so a realloc failure does not leak the
* original buffer AND leave response->data NULL for later
* writes to dereference. */
char *tmp;
response->buflen = response->len;
response->data = (char*)realloc(response->data, response->buflen);
tmp = (char*)realloc(response->data, response->buflen);
if (!tmp)
{
response->part = P_DONE;
state->err = true;
return -1;
}
response->data = tmp;
}
}
else
{
if (response->pos >= response->buflen - 64)
{
char *tmp;
response->buflen *= 2;
response->data = (char*)realloc(response->data, response->buflen);
tmp = (char*)realloc(response->data, response->buflen);
if (!tmp)
{
response->part = P_DONE;
state->err = true;
return -1;
}
response->data = tmp;
}
}
return len;