Skip to content

Commit eb86df0

Browse files
committed
libretro-common/net: Priority B hardening in net_http.c
Follow-up to commit 66155ff. Seven additional fixes in net_http.c covering the remaining unchecked-allocation sites and the chunked-encoding body parser. One of them (the Content-Length/chunked collision) is a real OOB read driven by a hostile server response; the rest are OOM hardening and a code-quality cleanup. net_http: reset response->len when entering T_CHUNK body A hostile server that sends BOTH "Content-Length: N" and "Transfer-Encoding: chunked" headers leaves response->len set to N (from the Content-Length pass) while the body parser switches to T_CHUNK mode. The chunked path uses response->len as the *position* of the current chunklen line and reads: memchr(response->data + response->len + 2, ..., response->pos - response->len - 2); With len=N and pos=0 (fresh after header processing), the subtraction "pos - len" is an unsigned wrap to a huge size_t and memchr() does a wild OOB read at "data + N + 2" for roughly SIZE_MAX bytes. Fix: when the blank line ends the header block and we transition to P_BODY_CHUNKLEN, explicitly set response->len = 0 to match the chunked-path contract. net_http: cap chunked chunklen at NET_HTTP_MAX_CONTENT_LENGTH strtoul(hex) accepts arbitrarily large values. A hostile server advertising a chunklen of ffffffffffffffff keeps the client in a receive loop indefinitely (each net_http_update() tick decrements a tiny amount from response->len, never reaching zero). Bandwidth / time DoS. Fix: reuse the existing NET_HTTP_MAX_CONTENT_LENGTH cap (256 MiB) for chunks too; oversized chunklen sets state->err and the dispatch loop tears down the transfer cleanly. net_http: realloc NULL checks in chunked body parser Three realloc() calls in net_http_receive_body (end-of-body shrink, end-of-transfer shrink, mid-parse grow) ignored their return values. On OOM the original buffer leaked AND response->data became NULL, which the rest of the function (and subsequent update ticks) dereference. Fix: tmp pointer pattern + state->err + return false to abort cleanly, matching the fix for the header-side reallocs from commit 66155ff. net_http: redirect path hardening net_http_redirect had four unchecked allocations: * strdup(new_url->domain) and strdup(new_url->path) (absolute redirect path) * strdup(location) (relative redirect, absolute path) * malloc(PATH_MAX_LENGTH) (relative redirect, relative path) * realloc(response->data, 64*1024) (response buffer reset) Each failure left a later strlen()/memcpy()/... target NULL or overwrote state->request.path with NULL. Fix: check each allocation, on failure set state->err and return true so the dispatch loop treats the transfer as finished-with-error and tears it down via the normal path. net_http: connection_done malloc NULL check The urlcopy expansion branch (hit when a URL has query parms but no port and no path separator, e.g. "site.com?x=1") did an unchecked malloc followed by two memcpys. On OOM the memcpys would NULL-deref. Fix: check the malloc and return false; caller treats false as a malformed URL and frees the partially-initialised connection via net_http_connection_free. net_http: connection_set_content malloc NULL check The postdata malloc was unchecked before the memcpy. Fix: if malloc fails, leave postdata NULL and reset contentlength to 0 so net_http_send_request does not advertise a Content-Length it cannot honour. net_http: urlencode malloc NULL check net_http_urlencode's output buffer malloc was unchecked and the encoding loop then wrote to the NULL pointer. Fix: leave *dest NULL and return on malloc failure. Callers who need to distinguish success/failure already check *dest against NULL (the typical URL-builder pattern). net_http: stack buffer for Content-Length digit string net_http_send_request allocated a small buffer via malloc to hold the decimal representation of request->contentlength, then snprintf'd into it. Pre-patch the malloc was unchecked and snprintf-into-NULL would crash. The value is at most 20 digits (UINT64_MAX + NUL), so switch to a 32-byte stack buffer and drop the malloc entirely. Also replace the subsequent strlen() with the snprintf return length. Reachability: * The Content-Length/chunked collision is remotely triggerable by a malicious server response; ~every HTTP/1.1 server distinguishes between the two body framings but a malicious one is free to send both. * The chunklen cap closes a bandwidth/time DoS on the same attack surface. * The remaining OOM fixes are crash avoidance on constrained devices. VC6 compatibility: the file is compiled on all platforms. The fix uses only size_t / ssize_t / int and standard comparisons; no new headers. The 32-byte stack buffer for the Content-Length string replaces the malloc path on both MSVC (_WIN32) and non-MSVC branches. Regression test: NONE, for the same reasons as the parent patch (net_http.c internals are static and exercised by net_http_update which dispatches to sockets). Compile-verified under -Wall -Werror for both -DHAVE_THREADS and no-threads configs. Full libretro-common samples CI dry-run: Built: 14 Ran: 14 Failed: 0 (the existing http_parse_test in that allowlist passes unchanged)
1 parent 8596a03 commit eb86df0

1 file changed

Lines changed: 141 additions & 26 deletions

File tree

libretro-common/net/net_http.c

Lines changed: 141 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,13 @@ void net_http_urlencode(char **dest, const char *source)
250250
enc = (char*)malloc(len + 1);
251251
*dest = enc;
252252

253+
/* Malloc failure: leave *dest NULL and bail. Callers that
254+
* dereference the result (common in URL-builder flows) will
255+
* then hit a single deliberate NULL check instead of a random
256+
* crash in the encoding loop below. */
257+
if (!enc)
258+
return;
259+
253260
/* Second pass: encode */
254261
for (s = source; *s; s++)
255262
{
@@ -453,6 +460,12 @@ bool net_http_connection_done(struct http_connection_t *conn)
453460
size_t domain_len = strlen(conn->domain);
454461
size_t path_len = strlen(conn->scan);
455462
char* urlcopy = (char*)malloc(domain_len + path_len + 2);
463+
/* Malloc failure: leave conn untouched and return false
464+
* so the caller does not use a partially-initialised
465+
* connection. Without this check the following memcpy
466+
* would NULL-deref. */
467+
if (!urlcopy)
468+
return false;
456469
memcpy(urlcopy, conn->domain, domain_len);
457470
urlcopy[domain_len] = '\0';
458471
memcpy(urlcopy + domain_len + 1, conn->scan, path_len + 1);
@@ -530,7 +543,15 @@ void net_http_connection_set_content(
530543
if (content_length)
531544
{
532545
conn->postdata = malloc(content_length);
533-
memcpy(conn->postdata, content, content_length);
546+
if (conn->postdata)
547+
memcpy(conn->postdata, content, content_length);
548+
else
549+
{
550+
/* Malloc failure: leave postdata NULL and reset
551+
* contentlength so net_http_send_request does not
552+
* advertise a Content-Length it cannot honour. */
553+
conn->contentlength = 0;
554+
}
534555
}
535556
}
536557

@@ -1114,8 +1135,8 @@ static bool net_http_send_request(struct http_t *state)
11141135
}
11151136
if (request->method && request->method[0] == 'P')
11161137
{
1117-
size_t _len, len;
1118-
char *len_str = NULL;
1138+
size_t _len;
1139+
int len;
11191140
if ( !request->postdata
11201141
&& request->method[1] == 'O' /* POST, not PUT */
11211142
&& request->contentlength > 0)
@@ -1130,19 +1151,27 @@ static bool net_http_send_request(struct http_t *state)
11301151
sizeof("Content-Type: application/x-www-form-urlencoded\r\n")-1);
11311152
net_http_send_str(state, "Content-Length: ", sizeof("Content-Length: ")-1);
11321153
_len = request->contentlength;
1154+
/* Use a stack buffer -- the maximum decimal representation
1155+
* of a size_t is 20 digits (UINT64_MAX) + NUL, well within
1156+
* 32 bytes. Pre-patch this was a malloc/snprintf pair
1157+
* whose malloc was never NULL-checked; a snprintf-into-NULL
1158+
* crash on OOM was the tail end of that sequence. */
1159+
{
1160+
char len_buf[32];
11331161
#ifdef _WIN32
1134-
len = snprintf(NULL, 0, "%" PRIuPTR, _len);
1135-
len_str = (char*)malloc(len + 1);
1136-
snprintf(len_str, len + 1, "%" PRIuPTR, _len);
1162+
len = snprintf(len_buf, sizeof(len_buf), "%" PRIuPTR, _len);
11371163
#else
1138-
len = snprintf(NULL, 0, "%llu", (long long unsigned)_len);
1139-
len_str = (char*)malloc(len + 1);
1140-
snprintf(len_str, len + 1, "%llu", (long long unsigned)_len);
1164+
len = snprintf(len_buf, sizeof(len_buf), "%llu",
1165+
(long long unsigned)_len);
11411166
#endif
1142-
len_str[len] = '\0';
1143-
net_http_send_str(state, len_str, strlen(len_str));
1167+
if (len < 0)
1168+
len = 0;
1169+
else if ((size_t)len >= sizeof(len_buf))
1170+
len = sizeof(len_buf) - 1;
1171+
len_buf[len] = '\0';
1172+
net_http_send_str(state, len_buf, (size_t)len);
1173+
}
11441174
net_http_send_str(state, "\r\n", sizeof("\r\n")-1);
1145-
free(len_str);
11461175
}
11471176
net_http_send_str(state, "User-Agent: ", sizeof("User-Agent: ")-1);
11481177
if (request->useragent)
@@ -1241,7 +1270,21 @@ static ssize_t net_http_receive_header(struct http_t *state, ssize_t len)
12411270
{
12421271
response->part = P_BODY;
12431272
if (response->bodytype == T_CHUNK)
1273+
{
12441274
response->part = P_BODY_CHUNKLEN;
1275+
/* The chunked body parser uses response->len as
1276+
* the position of the current chunklen line --
1277+
* must start at 0. A hostile server that sent
1278+
* both "Content-Length: N" and
1279+
* "Transfer-Encoding: chunked" would otherwise
1280+
* leave response->len set to N from the
1281+
* Content-Length pass, and the first chunked
1282+
* parse step computed "response->pos -
1283+
* response->len" as an unsigned wrap to a huge
1284+
* value. memchr() at that offset is a wild
1285+
* OOB read. */
1286+
response->len = 0;
1287+
}
12451288
}
12461289
scan = lineend + 1;
12471290
continue;
@@ -1396,6 +1439,18 @@ static bool net_http_receive_body(struct http_t *state, ssize_t newlen)
13961439
if (end)
13971440
{
13981441
size_t chunklen = strtoul(response->data + response->len, NULL, 16);
1442+
/* Cap the chunk length at the same Content-Length
1443+
* ceiling. A hostile server sending a chunklen
1444+
* like ffffffffffffffff drives the client into
1445+
* an effectively unbounded receive loop (each
1446+
* net_http_update tick nibbles at response->len
1447+
* and response->len never reaches zero). */
1448+
if (chunklen > NET_HTTP_MAX_CONTENT_LENGTH)
1449+
{
1450+
response->part = P_DONE;
1451+
state->err = true;
1452+
return false;
1453+
}
13991454
response->pos = response->len;
14001455
end++;
14011456

@@ -1413,9 +1468,17 @@ static bool net_http_receive_body(struct http_t *state, ssize_t newlen)
14131468
response->part = P_BODY;
14141469
if (response->len == 0)
14151470
{
1471+
char *tmp;
14161472
response->part = P_DONE;
14171473
response->len = response->pos;
1418-
response->data = (char*)realloc(response->data, response->len);
1474+
tmp = (char*)realloc(response->data,
1475+
response->len);
1476+
if (!tmp)
1477+
{
1478+
state->err = true;
1479+
return false;
1480+
}
1481+
response->data = tmp;
14191482
return true;
14201483
}
14211484
goto parse_again;
@@ -1446,24 +1509,46 @@ static bool net_http_receive_body(struct http_t *state, ssize_t newlen)
14461509
{
14471510
response->part = P_DONE;
14481511
if (response->buflen != response->len)
1449-
response->data = (char*)realloc(response->data, response->len);
1512+
{
1513+
char *tmp = (char*)realloc(response->data, response->len);
1514+
if (!tmp)
1515+
{
1516+
state->err = true;
1517+
return false;
1518+
}
1519+
response->data = tmp;
1520+
}
14501521
return true;
14511522
}
14521523
}
14531524

14541525
if (response->pos >= response->buflen)
14551526
{
1527+
char *tmp;
14561528
response->buflen *= 2;
1457-
response->data = (char*)realloc(response->data, response->buflen);
1529+
tmp = (char*)realloc(response->data, response->buflen);
1530+
if (!tmp)
1531+
{
1532+
state->err = true;
1533+
return false;
1534+
}
1535+
response->data = tmp;
14581536
}
14591537
return true;
14601538
}
14611539

14621540
static bool net_http_redirect(struct http_t *state, const char *location)
14631541
{
1464-
/* This reinitializes state based on the new location */
1542+
/* This reinitializes state based on the new location. Every
1543+
* allocation below is checked; on any failure state->err is
1544+
* set and we return true (the dispatch loop reads that as
1545+
* "transfer finished with an error"), leaving the state in a
1546+
* safe-to-delete shape. */
14651547

14661548
/* URL may be absolute or relative to the current URL */
1549+
char *new_domain = NULL;
1550+
char *new_path = NULL;
1551+
char *tmp;
14671552
bool absolute = (!strncmp(location, "http://", sizeof("http://")-1)
14681553
|| !strncmp(location, "https://", sizeof("https://")-1));
14691554

@@ -1476,42 +1561,72 @@ static bool net_http_redirect(struct http_t *state, const char *location)
14761561
if (!net_http_connection_done(new_url))
14771562
{
14781563
net_http_connection_free(new_url);
1564+
state->err = true;
14791565
return true;
14801566
}
1481-
state->ssl = new_url->ssl;
1567+
new_domain = strdup(new_url->domain);
1568+
new_path = strdup(new_url->path);
1569+
if (!new_domain || !new_path)
1570+
{
1571+
free(new_domain);
1572+
free(new_path);
1573+
net_http_connection_free(new_url);
1574+
state->err = true;
1575+
return true;
1576+
}
1577+
state->ssl = new_url->ssl;
1578+
state->request.port = new_url->port;
14821579
if (state->request.domain)
14831580
free(state->request.domain);
1484-
state->request.domain = strdup(new_url->domain);
1485-
state->request.port = new_url->port;
1581+
state->request.domain = new_domain;
14861582
if (state->request.path)
14871583
free(state->request.path);
1488-
state->request.path = strdup(new_url->path);
1584+
state->request.path = new_path;
14891585
net_http_connection_free(new_url);
14901586
}
14911587
else
14921588
{
14931589
if (*location == '/')
14941590
{
1591+
new_path = strdup(location);
1592+
if (!new_path)
1593+
{
1594+
state->err = true;
1595+
return true;
1596+
}
14951597
if (state->request.path)
14961598
free(state->request.path);
1497-
state->request.path = strdup(location);
1599+
state->request.path = new_path;
14981600
}
14991601
else
15001602
{
1501-
char *path = (char*)malloc(PATH_MAX_LENGTH);
1502-
fill_pathname_resolve_relative(path, state->request.path,
1603+
new_path = (char*)malloc(PATH_MAX_LENGTH);
1604+
if (!new_path)
1605+
{
1606+
state->err = true;
1607+
return true;
1608+
}
1609+
fill_pathname_resolve_relative(new_path, state->request.path,
15031610
location, PATH_MAX_LENGTH);
15041611
free(state->request.path);
1505-
state->request.path = path;
1612+
state->request.path = new_path;
15061613
}
15071614
}
15081615
state->request_sent = false;
15091616
state->response.part = P_HEADER_TOP;
15101617
state->response.status = -1;
15111618
/* Start with larger buffer to reduce reallocations */
15121619
state->response.buflen = 64 * 1024;
1513-
state->response.data = (char*)realloc(state->response.data,
1514-
state->response.buflen);
1620+
tmp = (char*)realloc(state->response.data, state->response.buflen);
1621+
if (!tmp)
1622+
{
1623+
/* Keep the existing buffer; state->data is still valid.
1624+
* Mark the transfer as errored so the dispatch loop tears
1625+
* it down. */
1626+
state->err = true;
1627+
return true;
1628+
}
1629+
state->response.data = tmp;
15151630
state->response.pos = 0;
15161631
state->response.len = 0;
15171632
state->response.bodytype = T_FULL;

0 commit comments

Comments
 (0)