sapi/cli: guard Content-Length overflow and enforce post_max_size - #22017
sapi/cli: guard Content-Length overflow and enforce post_max_size#22017iliaal wants to merge 1 commit into
Conversation
| # ifdef _WIN64 | ||
| # define SSIZE_MAX _I64_MAX | ||
| # else | ||
| # define SSIZE_MAX INT_MAX |
There was a problem hiding this comment.
more or less of the same (i.e. sizeof 4) but LONG_MAX is more accurate I think. Or even PTRDIFF_MAX.
There was a problem hiding this comment.
Switched to PTRDIFF_MAX, drops the _WIN64 split.
|
|
||
| php_http_parser_init(&client->parser, PHP_HTTP_REQUEST); | ||
| client->request_read = false; | ||
| client->too_large_post = false; |
There was a problem hiding this comment.
too_large_post is only cleared in client_ctor. Works now since each request rebuilds the client, but if keep-alive ever reuses the struct, the flag
lingers and every follow-up request gets a bogus 413. Worth resetting it with the other per-request state, or leaving a note at the declaration
There was a problem hiding this comment.
request_read, last_header_element, and current_header_* are all initialized only in client_ctor for the same reason: the client is torn down at request end. Keep-alive would need a shared per-request reset covering the lot, not just too_large_post.
612aa33 to
3827cd3
Compare
|
The CI failure is unrelated... But the reason seems to be not hard to find as it *might* be a race condition where the address (port) is used by another test. @iliaal You could force push this again to rerun CI :) |
I am 91% 😆 sure it is unrelated. it is in a completely different code path. |
3827cd3 to
89565d8
Compare
Yeah and I am having a headache to fix this. I hate github CI... |
| parser->content_length *= 10; | ||
| parser->content_length += ch - '0'; | ||
| if (parser->content_length > (SSIZE_MAX - (ch - '0')) / 10) { | ||
| goto error; |
There was a problem hiding this comment.
At this point the header name has been copied in php_cli_server_client_read_request_on_header_field() and will leak. We likely need to free current_header_name / current_header_value in php_cli_server_client_dtor().
There was a problem hiding this comment.
Freed both in php_cli_server_client_dtor() and dropped the two asserts, which sat behind content_sender_initialized and so never fired on that teardown. Valgrind on the Content-Length overflow case: 40 bytes definitely lost before, clean after.
| parser->content_length += c; | ||
| if (parser->content_length > (SSIZE_MAX - c) / 16) { | ||
| goto error; | ||
| } |
There was a problem hiding this comment.
I think that it's still possible to exceed post_max_size with Transfer-Encoding: chunked, as php_cli_server_client_read_request_on_body() will accumulate chunks without checking the limit.
There was a problem hiding this comment.
Right. The headers-complete check reads parser->content_length, which a chunked request never sets to the body size, so the limit now applies as the chunks accumulate in on_body(). That needed a parser change as well: all three on_body call sites discarded the callback's return value. Separately, the body buffer was reserved from the declared chunk size, so 7FFFFFFFFFFFFF followed by five bytes exited the server with "Out of memory" before any limit applied; that reservation is clamped now. Covered in gh22003.phpt.
The dev server's HTTP parser accumulates Content-Length digits into an ssize_t without an overflow check; a 30-digit value wraps and the consumer aborts on pemalloc. Guard the decimal and chunked-size accumulators against SSIZE_MAX, then reject in on_headers_complete when the parsed length exceeds post_max_size and reply 413 with the configured limit in the body. A chunked request carries no Content-Length, so enforce the same limit as the chunks accumulate in on_body, and honour a non-zero return from that callback in the parser. Reserving the body buffer from the declared chunk size aborted the server on a chunk header of 7FFFFFFFFFFFFF, so clamp the reservation to post_max_size. A parse error between a header name and its value left the copied name owned by nobody; release both header strings in php_cli_server_client_dtor(). Fixes phpGH-22003
89565d8 to
1921a88
Compare
4 participants
The dev server crashes when Content-Length wraps ssize_t (30+ digit value), or when a legitimately large Content-Length passes pemalloc and aborts the process.
Guard the parser's Content-Length and chunked-size accumulators against SSIZE_MAX, then reject oversize Content-Length in on_headers_complete and reply 413 with the configured post_max_size in the body.
Fixes #22003