Skip to content

Commit ae3292f

Browse files
committed
task_http: fix heap-buffer-overflow in GET fast-path + regression test
task_push_http_transfer_generic() decided whether a request is a "plain" GET (and therefore subject to dedup via task_http_finder) by doing: method = net_http_connection_method(conn); if (memcmp(method, "GET", 3) == 0 && method[3] == '\0') `method` is a pointer into a buffer that net_http_connection_new() strdup'd from caller-supplied data; its allocation is exactly strlen(method) + 1 bytes. Two cases overrun that buffer: 1) method == NULL. task_push_http_transfer_with_content() forwards the caller-supplied `method` argument straight to net_http_connection_new() without a NULL check; net_http_connection_new() itself permits a NULL method (conn->method stays NULL). net_http_connection_method() then returns NULL and the memcmp dereferences it. 2) method shorter than 3 chars (e.g. "" or a 1-char custom verb). strdup allocates 1 or 2 bytes; memcmp reads 3 bytes regardless, overrunning the heap allocation. AddressSanitizer reports: ==N==ERROR: AddressSanitizer: heap-buffer-overflow READ of size 3 at <addr> thread T0 #0 ... memcmp #1 task_push_http_transfer_generic tasks/task_http.c:274 <addr> is located 0 bytes after 2-byte region [...,...] allocated by thread T0 here: #0 ... strdup #1 net_http_connection_new The trailing `method[3] == '\0'` guard happens *after* memcmp's read, so it cannot prevent the overflow. Replace the unsafe pair with string_is_equal(), which is bounded by the buffer's own NUL terminator and is already used elsewhere in this file (see task_http_finder at line 242 and the existing #include of <string/stdstring.h>). Add an explicit NULL guard for the method that takes the existing error path; this also frees `conn` via the goto so the existing ownership invariant on the error label is preserved. Add a standalone regression test under samples/tasks/http/ and wire it into Linux-samples-tasks CI. The test mirrors samples/tasks/decompress/archive_name_safety_test.c: it keeps a verbatim copy of the predicate (the dispatch decision is local to task_push_http_transfer_generic and not in a public header) and exercises it against the truth-table plus the bounds-safety regression cases ("", "X", "GE", NULL). Inputs are strdup'd so each buffer's allocation is exactly strlen+1, matching what net_http_connection_new() does and making the pre-fix expression trip ASan in CI. The Makefile honours SANITIZER= the same way samples/tasks/database/ does, and the new CI step builds with SANITIZER=address so a regression to any unsafe expression is caught at the bounds level, not just the truth-table level. Verified locally: with the post-fix predicate the test passes clean under ASan; reverting the predicate to the pre-fix memcmp causes ASan to abort with "heap-buffer-overflow ... READ of size 3" on the empty-string case. build-essential on ubuntu-latest already pulls libasan via gcc-13's libgcc-13-dev, so no apt-get changes are needed in the workflow. Found via a focused ASan harness around the task_http.c finish-path ownership logic.
1 parent d3bd7e4 commit ae3292f

4 files changed

Lines changed: 223 additions & 1 deletion

File tree

.github/workflows/Linux-samples-tasks.yml

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,3 +81,22 @@ jobs:
8181
# ever amends the predicate, the copy here must follow.
8282
timeout 60 ./archive_name_safety_test
8383
echo "[pass] archive_name_safety_test"
84+
85+
- name: Build and run http_method_match_test (ASan)
86+
shell: bash
87+
working-directory: samples/tasks/http
88+
run: |
89+
set -eu
90+
# Regression test for the heap-buffer-overflow fix in
91+
# tasks/task_http.c::task_push_http_transfer_generic().
92+
# Pre-fix, an unsafe memcmp(method, "GET", 3) read past
93+
# the strdup'd buffer when method was NULL or shorter
94+
# than 3 bytes. Build under AddressSanitizer so a
95+
# regression to the unsafe expression is caught at the
96+
# bounds level, not just the truth-table level. If
97+
# task_http.c amends the GET-dispatch predicate, the
98+
# copy in http_method_match_test.c must follow.
99+
make clean all SANITIZER=address
100+
test -x http_method_match_test
101+
timeout 60 ./http_method_match_test
102+
echo "[pass] http_method_match_test"

samples/tasks/http/Makefile

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
TARGET := http_method_match_test
2+
3+
SOURCES := http_method_match_test.c
4+
5+
OBJS := $(SOURCES:.c=.o)
6+
7+
CFLAGS += -Wall -pedantic -std=gnu99 -g -O0
8+
9+
ifneq ($(SANITIZER),)
10+
CFLAGS := -fsanitize=$(SANITIZER) -fno-omit-frame-pointer $(CFLAGS)
11+
LDFLAGS := -fsanitize=$(SANITIZER) $(LDFLAGS)
12+
endif
13+
14+
all: $(TARGET)
15+
16+
%.o: %.c
17+
$(CC) -c -o $@ $< $(CFLAGS)
18+
19+
$(TARGET): $(OBJS)
20+
$(CC) -o $@ $^ $(LDFLAGS)
21+
22+
clean:
23+
rm -f $(TARGET) $(OBJS)
24+
25+
.PHONY: clean
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
/* Copyright (C) 2010-2026 The RetroArch team
2+
*
3+
* ---------------------------------------------------------------------------------------
4+
* The following license statement only applies to this file (http_method_match_test.c).
5+
* ---------------------------------------------------------------------------------------
6+
*
7+
* Permission is hereby granted, free of charge,
8+
* to any person obtaining a copy of this software and associated documentation files (the "Software"),
9+
* to deal in the Software without restriction, including without limitation the rights to
10+
* use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software,
11+
* and to permit persons to whom the Software is furnished to do so, subject to the following conditions:
12+
*
13+
* The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software.
14+
*
15+
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED,
16+
* INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
17+
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
18+
* IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
19+
* WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
20+
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
21+
*/
22+
23+
/* Regression test for the heap-buffer-overflow fix in
24+
* tasks/task_http.c::task_push_http_transfer_generic().
25+
*
26+
* Original bug: the dispatch decision was
27+
*
28+
* if (memcmp(method, "GET", 3) == 0 && method[3] == '\0')
29+
*
30+
* `method` is a strdup'd buffer whose allocation is exactly
31+
* strlen(method) + 1 bytes. When `method` is NULL or shorter than
32+
* 3 bytes (e.g. "" or "X") the memcmp reads past the buffer. The
33+
* trailing method[3] guard runs *after* the read, so it cannot
34+
* prevent the overflow.
35+
*
36+
* Fix: dispatch via string_is_equal(method, "GET") with an explicit
37+
* NULL guard upstream.
38+
*
39+
* The match predicate is local to task_push_http_transfer_generic()
40+
* and isn't in a public header, so this test keeps a verbatim copy
41+
* of the relevant fragment as the oracle, exactly as
42+
* archive_name_safety_test.c does for archive_name_is_safe(). If
43+
* task_http.c amends the predicate, the copy here must follow.
44+
*
45+
* The test is most useful when built under AddressSanitizer:
46+
*
47+
* make clean all SANITIZER=address
48+
* ./http_method_match_test
49+
*
50+
* Without ASan the post-fix expression is also bounds-safe and the
51+
* test simply asserts the truth-table. With ASan, *both* the
52+
* truth-table and the bounds-safety are checked at once: building
53+
* the pre-fix expression instead would trip ASan on the short/NULL
54+
* inputs below.
55+
*
56+
* Build standalone:
57+
* cc -Wall -std=gnu99 -g -O0 -o http_method_match_test \
58+
* http_method_match_test.c
59+
* ./http_method_match_test
60+
*/
61+
62+
#include <stdio.h>
63+
#include <stdlib.h>
64+
#include <string.h>
65+
#include <stdbool.h>
66+
67+
/* Drop-in equivalent of libretro-common/string/stdstring.h's
68+
* string_is_equal(). Kept local so the test compiles standalone
69+
* without pulling libretro-common into the build, matching the
70+
* archive_name_safety_test.c convention. */
71+
static inline bool string_is_equal(const char *a, const char *b)
72+
{
73+
if (a == b)
74+
return true;
75+
if (!a || !b)
76+
return false;
77+
return strcmp(a, b) == 0;
78+
}
79+
80+
/* =============== verbatim from tasks/task_http.c =============== *
81+
* The fragment under test is the dispatch decision made on `method`
82+
* inside task_push_http_transfer_generic(), post-fix:
83+
*
84+
* if (!method)
85+
* goto error;
86+
* if (string_is_equal(method, "GET"))
87+
* ... GET fast-path ...
88+
*
89+
* Wrapped here as a single bool predicate that mirrors the post-fix
90+
* control flow: NULL is rejected (returns false, like the goto error
91+
* branch which never reaches the GET fast-path), otherwise the
92+
* GET-equality test runs against the buffer's NUL terminator.
93+
*/
94+
static bool http_is_plain_get(const char *method)
95+
{
96+
if (!method)
97+
return false;
98+
return string_is_equal(method, "GET");
99+
}
100+
/* =============== end verbatim copy =============== */
101+
102+
static int failures = 0;
103+
104+
/* expect() takes a strdup'd input so that the buffer's allocation
105+
* size is exactly strlen+1 bytes, which is what
106+
* net_http_connection_new() does and what makes the pre-fix code
107+
* trip ASan. A literal would sit in .rodata where the OOB read may
108+
* not be caught. */
109+
static void expect(const char *literal_or_null, bool want)
110+
{
111+
char *buf = literal_or_null ? strdup(literal_or_null) : NULL;
112+
bool got = http_is_plain_get(buf);
113+
const char *display = literal_or_null
114+
? (literal_or_null[0] ? literal_or_null : "(empty)")
115+
: "(null)";
116+
117+
if (got != want)
118+
{
119+
printf("[FAILED] %-12s expected %s, got %s\n",
120+
display,
121+
want ? "GET" : "non-GET",
122+
got ? "GET" : "non-GET");
123+
failures++;
124+
}
125+
else
126+
{
127+
printf("[SUCCESS] %-12s %s\n",
128+
display,
129+
want ? "matched GET fast-path"
130+
: "correctly not GET");
131+
}
132+
133+
free(buf);
134+
}
135+
136+
int main(void)
137+
{
138+
/* --- the only true case --- */
139+
expect("GET", true);
140+
141+
/* --- non-GET methods that the function legitimately receives --- */
142+
expect("POST", false);
143+
expect("PUT", false);
144+
expect("DELETE", false);
145+
expect("OPTIONS", false);
146+
expect("MKCOL", false);
147+
expect("MOVE", false);
148+
149+
/* --- the bounds-safety regression cases ---
150+
* Pre-fix, every one of these tripped a heap-buffer-overflow
151+
* read of size 3 in memcmp(). Post-fix, string_is_equal walks
152+
* to the buffer's NUL terminator and is safe on short/empty
153+
* input; the NULL case is rejected by the upstream guard. */
154+
expect("", false); /* 1-byte alloc, pre-fix read 3 */
155+
expect("X", false); /* 2-byte alloc, pre-fix read 3 */
156+
expect("GE", false); /* 3-byte alloc; GET prefix of "GET" but not equal */
157+
expect(NULL, false); /* pre-fix dereferenced NULL */
158+
159+
/* --- near-misses that must NOT collapse to GET --- */
160+
expect("GETx", false); /* GET prefix + extra chars */
161+
expect("get", false); /* case sensitivity */
162+
expect("GE\0T", false); /* embedded NUL: only "GE" is visible to strcmp */
163+
164+
if (failures)
165+
{
166+
printf("\n%d test(s) failed\n", failures);
167+
return 1;
168+
}
169+
printf("\nAll http_is_plain_get regression tests passed.\n");
170+
return 0;
171+
}

tasks/task_http.c

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -266,12 +266,19 @@ static void *task_push_http_transfer_generic(
266266

267267
method = net_http_connection_method(conn);
268268

269+
/* net_http_connection_new() permits a NULL method, so
270+
* net_http_connection_method() may legitimately return NULL here.
271+
* Treat that as a hard error: we cannot dispatch a request without
272+
* a method, and the GET fast-path below would deref NULL. */
273+
if (!method)
274+
goto error;
275+
269276
/* POST requests usually mutate the server, so assume multiple calls are
270277
* intended, even if they're duplicated. Additionally, they may differ
271278
* only by the POST data, and task_http_finder doesn't look at that, so
272279
* unique requests could be misclassified as duplicates.
273280
*/
274-
if (memcmp(method, "GET", 3) == 0 && method[3] == '\0')
281+
if (string_is_equal(method, "GET"))
275282
{
276283
task_finder_data_t find_data;
277284
find_data.func = task_http_finder;

0 commit comments

Comments
 (0)