Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Partial content on POST request with 'chunked' transfer encoding #9949

Closed
withings-sas opened this issue Nov 14, 2022 · 10 comments
Closed

Partial content on POST request with 'chunked' transfer encoding #9949

withings-sas opened this issue Nov 14, 2022 · 10 comments

Comments

@withings-sas
Copy link

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+

@cmb69
Copy link
Member

cmb69 commented Nov 25, 2022

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.

Hmm, I tried variations of the following script, but for me, ap_get_brigade() just stalls, instead of returning APR_INCOMPLETE.

<?php

$s = fsockopen("localhost", 80);
fwrite($s, "POST /gh9949.php HTTP/1.1\r\n");
fwrite($s, "Host: localhost\r\n");
fwrite($s, "User-Agent: curl/7.83.1\r\n");
fwrite($s, "Accept: */*\r\n");
fwrite($s, "Content-Type: application/x-www-form-urlencoded\r\n");
fwrite($s, "Transfer-Encoding: chunked\r\n");
fwrite($s, "\r\n");
fwrite($s, "f\r\n");
fwrite($s, "foo=bar&baz=qux\r\n");
// fwrite($s, "0\r\n\r\n");

var_dump(fread($s, 4096));

Could you come up with a reproduce script?

@withings-sas
Copy link
Author

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.c and to run: ./post-issue localhost 80

It must call a php script, as simple as for example:

<?php

$body = file_get_contents('php://input');

echo "size:".strlen($body)."\n";

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
Copy link
Member

cmb69 commented Nov 28, 2022

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.

Ah, indeed. Thank you!

@cmb69
Copy link
Member

cmb69 commented Nov 29, 2022

After having a closer look, I wonder if and how we should handle other apr_get_brigade() failures. Setting the status to HTTP_BAD_REQUEST doesn't look right. Maybe HTTP_INTERNAL_SERVER_ERROR is more suitable for other statuses?

Also php should either not get executed at all, or warned in some way that the content is partial.

That appears to be reasonable, so I've looked what happened if post_max_size=1, and got:

Warning:  Unknown: Actual POST length does not match Content-Length, and exceeds 1 bytes in Unknown on line 0

Hmm, there is no Content-Length for this chunked request, though. ;)

@withings-sas
Copy link
Author

Also php should either not get executed at all, or warned in some way that the content is partial.

That appears to be reasonable, so I've looked what happened if post_max_size=1, and got:

Warning:  Unknown: Actual POST length does not match Content-Length, and exceeds 1 bytes in Unknown on line 0

Hmm, there is no Content-Length for this chunked request, though. ;)

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
Copy link
Author

Hello,

I may have misunderstood your last comment @cmb69, do you confirm you see the current behavior as a bug ?

@cmb69
Copy link
Member

cmb69 commented Dec 6, 2022

do you confirm you see the current behavior as a bug ?

Yes; that's why I added the Status: Verified label. :)

cmb69 added a commit to cmb69/php-src that referenced this issue Dec 6, 2022
`ap_get_brigade()` may fail for different reasons, and we must not
pretend that a partially read POST payload is fine; instead we report
a content length of zero what matches all other `read_post()` callbacks
of bundled SAPIs.
@cmb69 cmb69 linked a pull request Dec 6, 2022 that will close this issue
@withings-sas
Copy link
Author

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
Copy link
Member

cmb69 commented Dec 7, 2022

I've checked the other bundled SAPI's, and they all just return 0 on 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
Copy link
Author

Ok thank you we'll make do with the size :)

cmb69 added a commit that referenced this issue Dec 13, 2022
* PHP-8.1:
  Fix GH-9949: Partial content on incomplete POST request
@cmb69 cmb69 closed this as completed in aef7d81 Dec 13, 2022
cmb69 added a commit that referenced this issue Dec 13, 2022
* PHP-8.2:
  Fix GH-9949: Partial content on incomplete POST request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants
@cmb69 @withings-sas and others