Commit 66155ff
committed
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.1 parent fd69102 commit 66155ff
1 file changed
Lines changed: 120 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
46 | 54 | | |
47 | 55 | | |
48 | 56 | | |
| |||
809 | 817 | | |
810 | 818 | | |
811 | 819 | | |
812 | | - | |
| 820 | + | |
| 821 | + | |
| 822 | + | |
813 | 823 | | |
814 | 824 | | |
815 | 825 | | |
| |||
820 | 830 | | |
821 | 831 | | |
822 | 832 | | |
| 833 | + | |
| 834 | + | |
| 835 | + | |
| 836 | + | |
| 837 | + | |
| 838 | + | |
| 839 | + | |
| 840 | + | |
| 841 | + | |
| 842 | + | |
| 843 | + | |
| 844 | + | |
| 845 | + | |
| 846 | + | |
| 847 | + | |
| 848 | + | |
| 849 | + | |
| 850 | + | |
| 851 | + | |
| 852 | + | |
| 853 | + | |
| 854 | + | |
| 855 | + | |
| 856 | + | |
| 857 | + | |
| 858 | + | |
| 859 | + | |
| 860 | + | |
| 861 | + | |
| 862 | + | |
| 863 | + | |
| 864 | + | |
| 865 | + | |
823 | 866 | | |
824 | 867 | | |
825 | 868 | | |
| |||
1153 | 1196 | | |
1154 | 1197 | | |
1155 | 1198 | | |
1156 | | - | |
| 1199 | + | |
| 1200 | + | |
| 1201 | + | |
| 1202 | + | |
| 1203 | + | |
| 1204 | + | |
| 1205 | + | |
| 1206 | + | |
| 1207 | + | |
| 1208 | + | |
1157 | 1209 | | |
1158 | | - | |
| 1210 | + | |
1159 | 1211 | | |
1160 | 1212 | | |
1161 | 1213 | | |
1162 | 1214 | | |
1163 | 1215 | | |
1164 | 1216 | | |
1165 | | - | |
| 1217 | + | |
| 1218 | + | |
| 1219 | + | |
| 1220 | + | |
| 1221 | + | |
| 1222 | + | |
| 1223 | + | |
| 1224 | + | |
| 1225 | + | |
| 1226 | + | |
| 1227 | + | |
1166 | 1228 | | |
1167 | 1229 | | |
1168 | 1230 | | |
| |||
1191 | 1253 | | |
1192 | 1254 | | |
1193 | 1255 | | |
| 1256 | + | |
| 1257 | + | |
| 1258 | + | |
| 1259 | + | |
| 1260 | + | |
| 1261 | + | |
| 1262 | + | |
| 1263 | + | |
| 1264 | + | |
| 1265 | + | |
1194 | 1266 | | |
1195 | | - | |
| 1267 | + | |
| 1268 | + | |
| 1269 | + | |
1196 | 1270 | | |
1197 | 1271 | | |
1198 | 1272 | | |
1199 | | - | |
| 1273 | + | |
| 1274 | + | |
| 1275 | + | |
| 1276 | + | |
| 1277 | + | |
| 1278 | + | |
| 1279 | + | |
| 1280 | + | |
| 1281 | + | |
| 1282 | + | |
| 1283 | + | |
| 1284 | + | |
| 1285 | + | |
| 1286 | + | |
| 1287 | + | |
| 1288 | + | |
| 1289 | + | |
| 1290 | + | |
| 1291 | + | |
| 1292 | + | |
1200 | 1293 | | |
1201 | 1294 | | |
1202 | 1295 | | |
| |||
1234 | 1327 | | |
1235 | 1328 | | |
1236 | 1329 | | |
| 1330 | + | |
| 1331 | + | |
| 1332 | + | |
| 1333 | + | |
1237 | 1334 | | |
1238 | | - | |
| 1335 | + | |
| 1336 | + | |
| 1337 | + | |
| 1338 | + | |
| 1339 | + | |
| 1340 | + | |
| 1341 | + | |
| 1342 | + | |
1239 | 1343 | | |
1240 | 1344 | | |
1241 | 1345 | | |
1242 | 1346 | | |
1243 | 1347 | | |
1244 | 1348 | | |
| 1349 | + | |
1245 | 1350 | | |
1246 | | - | |
| 1351 | + | |
| 1352 | + | |
| 1353 | + | |
| 1354 | + | |
| 1355 | + | |
| 1356 | + | |
| 1357 | + | |
| 1358 | + | |
1247 | 1359 | | |
1248 | 1360 | | |
1249 | 1361 | | |
| |||
0 commit comments