Repository navigation
Partial content on POST request with 'chunked' transfer encoding #9949
Description
Description
Using sapi_apache2 and performing a POST query with the "transfer-encoding: chunk" header, if the query is interrupted before all chunked/data were sent, PHP still accept the partial content received and execute normally, returning a 200 status.
Per RFC, it should return 400, which apache2 does if the php module is not activated. Also php should either not get executed at all, or warned in some way that the content is partial.
Looking at this closely, it seems the issue comes from the following code:
In function php_apache_sapi_read_post of sapi_apache2.c lines 196-205 (on PHP8.2 branch):
while (ap_get_brigade(r->input_filters, brigade, AP_MODE_READBYTES, APR_BLOCK_READ, len) == APR_SUCCESS) {
apr_brigade_flatten(brigade, buf, &len);
apr_brigade_cleanup(brigade);
tlen += len;
if (tlen == count_bytes || !len) {
break;
}
buf += len;
len = count_bytes - tlen;
}
return tlen;
The return of ap_get_brigade exit normally if it receives anything other than APR_SUCCESS and in particular does not check if it returns APR_INCOMPLETE in which case it should return a 400 response code.
I checked a quick and dirty fix:
apr_status_t ast;
ast = ap_get_brigade(r->input_filters, brigade, AP_MODE_READBYTES, APR_BLOCK_READ, len);
while (ast == APR_SUCCESS) {
apr_brigade_flatten(brigade, buf, &len);
apr_brigade_cleanup(brigade);
tlen += len;
if (tlen == count_bytes || !len) {
break;
}
buf += len;
len = count_bytes - tlen;
ast = ap_get_brigade(r->input_filters, brigade, AP_MODE_READBYTES, APR_BLOCK_READ, len);
}
if (ast == APR_INCOMPLETE) {
ctx->r->status = 400;
}
return tlen;
And it returns 400 correctly (but clearly I am not suggesting this as the correct fix, just demonstrating that the ap_get_bricade() return code should be taken care of).
PHP Version
8.0+
Operating System
Ubuntu 18.04+
Activity
cmb69 commentedon Nov 25, 2022
Hmm, I tried variations of the following script, but for me,
ap_get_brigade()just stalls, instead of returningAPR_INCOMPLETE.Could you come up with a reproduce script?
withings-sas commentedon Nov 25, 2022
Yes sure I should have done that before, please find attached the (ugly) c code that we used to reproduce it.
post-issue.zip (the exit(0/0) is to simulate a violent crash but I don't think it's necessary)
To compile:
gcc -Wno-div-by-zero -o post-issue post-issue.cand to run:./post-issue localhost 80It must call a php script, as simple as for example:
What makes it not hung I guess is the fact that we (abruptly) close the TCP connection, which triggers the end of the while loop.
Do not hesitate to ask for more information.
cmb69 commentedon Nov 28, 2022
Ah, indeed. Thank you!
cmb69 commentedon Nov 29, 2022
After having a closer look, I wonder if and how we should handle other
apr_get_brigade()failures. Setting the status toHTTP_BAD_REQUESTdoesn't look right. MaybeHTTP_INTERNAL_SERVER_ERRORis more suitable for other statuses?That appears to be reasonable, so I've looked what happened if
post_max_size=1, and got:Hmm, there is no Content-Length for this chunked request, though. ;)
withings-sas commentedon Nov 30, 2022
Yes it's indeed normal, according to the RFC.
Yes HTTP_INTERNAL_SERVER_ERROR would be good I guess, the php script should interrupt and/or be signaled to stop in some way, how could you trigger this internal error ?
withings-sas commentedon Dec 6, 2022
Hello,
I may have misunderstood your last comment @cmb69, do you confirm you see the current behavior as a bug ?
cmb69 commentedon Dec 6, 2022
Yes; that's why I added the
Status: Verifiedlabel. :)Fix phpGH-9949: Partial content on incomplete POST request
withings-sas commentedon Dec 7, 2022
Thank you for the fix, though when testing it (on php8.2), I still can reproduce the issue: apache returns HTTP 200 and the page gets executed. Are you sure returning 0 is enough ? Do we need to add a check for this length in the php code (and how) ?
cmb69 commentedon Dec 7, 2022
I've checked the other bundled SAPI's, and they all just return
0on failure to read (except, lsapi, which also raises a warning). I don't want to introduce any stricter behavior (at least not for stable PHP versions), and I think the most important point is that no partial content can be read.withings-sas commentedon Dec 7, 2022
Ok thank you we'll make do with the size :)
Fix GH-9949: Partial content on incomplete POST request
Merge branch 'PHP-8.1' into PHP-8.2
Merge branch 'PHP-8.2'