Skip to content

fastcgi_finish_request() doesn't eof requests with no body, allowing setting response headers for othe requests #18275

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

Open
kkmuffme opened this issue Apr 8, 2025 · 9 comments
Assignees

Comments

@kkmuffme
Copy link

kkmuffme commented Apr 8, 2025

Description

<?php

// https://one.example.com
// possibly only with HTTP 2/3, didn't test with HTTP 1.0/1.1
if ( $_SERVER['REQUEST_URI'] === '/result/' || $_SERVER['REQUEST_URI'] === '/world/' ) {
	echo "done";
	exit;
}

// this is required to stop nginx from buffering the response (and otherwise this error doesn't happen)
header( 'X-Accel-Buffering: no' );

header( 'Location: https://one.example.com/result/', true, 302 );
register_shutdown_function( static function () {
	register_shutdown_function( static function () {
		// this is required, otherwise it won't happen
		fastcgi_finish_request();

		// the "sleep" is what creates the issue. A usleep for a shorter amount of time seems to be enough though (I don't think the sleep() is the origin of the issue though, it just allows other requests to be handled in the meantime, which causes this issue)
		sleep( 1 );

		// echoing arbitrary text with a space before any ":" will cause nginx to report:
		// upstream sent invalid header: "a\x20..." while reading response header from upstream
		// echo 'a b';
		// outputting text that looks like a header, will set this as response header for the page we redirect to, but concatenates it randomly with a different response header (see screenshot)
		echo 'X-Risky: yes';
	} );
} );

When opening https://one.example.com, the echoed "X-Risky" suddenly shows up as a response header on the redirected URL, with the value randomly concatenated with another response header (actual one that the redirected request set)

Image

If we echo something with a space before the :, e.g. echo 'a b'; nginx will instead report an error (on the redirected URL too!):

upstream sent invalid header: "a\x20..." while reading response header from upstream

What is potentially a security issue is, that it seems to apply that "X-Risky" to whatever next request is handled by nginx. Even if it is a different subdomain (didn't test it for different domain).
e.g. if I am on one.example.com (which is it's own server {} block in nginx), and set a Location: https://two.example.com (which is it's own server {} block in nginx too, but handled in the same nginx instance), then suddenly I get the X-Risky header in the response of two.example.com (and when using the "a b" I also get the upstream error in the two.example.com nginx error log)
It's not necessarily the "Location:" that this wrong header is applied too:
e.g. replace the "Location:" header in the example above with:
Link: </world/>; as=fetch; crossorigin; rel=preload
when requesting the page in Chrome, the response for /world/ will suddenly have the X-Risky header

Even worse, it seems it's just whatever request is next handled by the nginx instance at the time AFTER the sleep gets that header applied (or the error reported), even if it's a completely different (sub)domain in a different server {} block
(this issue happens independent of request method, e.g. GET, POST,... all same)

The issue seems to be for responses that do not have a request body, but are headers only at the time when fastcgi_finish_request() is called.

I guess PHP fastcgi_finish_request() doesn't send an "EOF" https://github.com/openresty/lua-nginx-module?tab=readme-ov-file#ngxeof

This isn't a security issue in PHP itself (therefore not reported as such), but just an unclear and unexpected error.
It has been reported to nginx as a security advisory independently

PHP Version

PHP 8.3.17 (php-fpm) + nginx 1.27.1.1

Operating System

No response

@kkmuffme
Copy link
Author

Additionally, the HTTP status code is incorrectly set too. If the redirection result would set HTTP status code 404, it will be returned with status 200 though

@kkmuffme
Copy link
Author

The reason for this behavior is #10335 - disabling fastcgi_keep_conn on; in nginx fixes this issue

@bukka
Copy link
Member

bukka commented Apr 24, 2025

This sounds more like an issue in nginx to me. But it's possible that we are messing up the protocol as that echo 'X-Risky: yes'; should not be sent I would think. I need to debug it to see the fcgi records and what nginx is doing.

It has been reported to nginx as a security advisory independently

What was the nginx team response?

@kkmuffme
Copy link
Author

What was the nginx team response?

They're investigating (couldn't reproduce at first, since the important part was missing #18275 (comment))

However, from what it looks like, this isn't an issue in nginx (and nothing nginx can do about that), since for nginx the request is completed, since this is what PHP tells it.

Coincidentally, I found openresty/lua-nginx-module#156 - one could try to use that and see what happens when using ngx.say or ngx.header[...] after the ngx.eof and a sleep - and whether it would cause the same issue we see in PHP or not.

As the OP there writes though, this correct behavior has unwanted side-effects for PHP's fastcgi_finish_request():

Then if we open this url in browser and continuously refresh the page the first one would be served in ~1 ms, but all subsequent request can delay up to 1 second.

@bukka
Copy link
Member

bukka commented May 14, 2025

@kkmuffme Have you got some reliable steps how to recreate.

I just created a test using my tool for this: wstool/wst-php-fpm@90a8106 which sets up nginx and fpm and runs specific requests. The generated nginx config in this case looks like:

pid /home/jakub/prog/wst/wst-php-fpm/workspace/nginx-fastcgi-finish-shutdown-eof/nginx/_env/run/nginx/nginx.pid;
daemon off;

error_log /dev/stderr info;

events {
    worker_connections 4;
}

http {
    log_format wst '"$request" $status';

    error_log /dev/stderr info;
    access_log /dev/stdout wst;

    server {
        listen       127.0.0.1:8525 default_server;
        server_name  _;
        server_name  _;
        root         /home/jakub/prog/wst/wst-php-fpm/workspace/nginx-fastcgi-finish-shutdown-eof/nginx/_env/var/www/nginx;

        location / {
            fastcgi_split_path_info ^(.+?\.php)(/.*)$;
            fastcgi_param PATH_INFO $fastcgi_path_info;
            fastcgi_pass 127.0.0.1:8526;
            include fastcgi_params;
            fastcgi_keep_conn on;
        }
    }
}

So it's just for http/1.1 atm as I wanted to tried just that first (it's easier to debug it). I tried various variants (even with php script) and also did some manual testing to make sure the keepalive is used but so far I was not able to see it. I also used Wireshark to check the protocol exchange and didn't even see that X-Risky header sent.

I see that you note it that you tried just http/2 so I will try it next but it would be a bit strange that FPM would behave differently for HTTP/2 (it shouldn't really matter from FPM point of view but maybe it handles connections differently). It would be helpful for me if you could share your nginx config (it can be redacted ofc.) so I can try with the version that is closer to it. Also if you can share more info how to easily hit it using few requests that would be also very helpful.

@kkmuffme
Copy link
Author

You're missing 2 things: fastcgi_pass php_fpm; with

upstream php_fpm {
    zone php_zone 8k;
    server unix:/run/php-fpm/php-fpm.sock;

    keepalive 2;
    keepalive_requests 1000;
    keepalive_timeout 60s;
}

and it seems this issue only happens with SNI SSL (I cannot test/reproduce it without it at least)
And nginx must be <1.27.4 bc it seems nginx fixed this X-Risky header issue in 1.27.4:

Security: insufficient check in virtual servers handling with TLSv1.3 SNI allowed to reuse SSL sessions in a different virtual server, to
bypass client SSL certificates verification (CVE-2025-23419).

(assuming that header-like output will now behave like normal output, resulting in an nginx error) I couldn't test it with nginx 1.27.4+ yet, bc I'm waiting for a release for our platform that should hopefully come in the next weeks.

@bukka
Copy link
Member

bukka commented May 14, 2025

Ok I will take a look into that setup and try few more things later. I have done some more debugging and I can see that there is actuall fcgi_write called but it should normally not get flushed for small buffer but can get flushed for large buffer (8k+) so that's another case that could be problematic. It's not probably issue here. What I'm suspecting is that the buffer does not get somehow correctly cleaned up which I would still like to find out.

In general we should prohitbit fcgi_write happening after the request finish as there is no point to do that (at least I can't think of any valid use case for it).

@kkmuffme
Copy link
Author

at least I can't think of any valid use case for it

With a little tweaking this could be made into HTTP Early Hints 103 I guess (like https://frankenphp.dev/docs/early-hints/ already does)

@bukka
Copy link
Member

bukka commented May 14, 2025

That would be done through a different function (e.g. #7025 ) but it will need some extra support on the server and we might need to add some indication for the server. The nginx guys said that they will look into it at some point: nginx/nginx#326 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants