Skip to content

Weird behavior when updating to 3.3.18 #28233

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

Closed
jmartinsnexity opened this issue Aug 20, 2018 · 16 comments
Closed

Weird behavior when updating to 3.3.18 #28233

jmartinsnexity opened this issue Aug 20, 2018 · 16 comments

Comments

@jmartinsnexity
Copy link

jmartinsnexity commented Aug 20, 2018

Symfony version(s) affected: 3.3.18

Description
I have a weird behavior when updating from symfony 3.3.17 to 3.3.18 : render_esi blocks in Twig won't work (all sub-requets I guess) if the master request is POST, behind a proxy. Tricky context...

The encountered error is as Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException thrown by Symfony/Component/HttpKernel/EventListener/FragmentListener.php line 90.
It only appears in production environnement, when we hit servers through the Load Balancer.

I think this is linked to the security patch #cve-2018-14774 (https://symfony.com/blog/cve-2018-14774-possible-host-header-injection-when-using-httpcache).
As we are behind a Load Balancer, we set trusted proxies as mentioned in the doc using IP ranges, but still, subrequests result in an AccessDeniedHttpException.

How to reproduce
To reproduce the error, you need to use render_esi blocks in your template ( {{ render_esi(controller('MyAppBundle:Controller:actionName')) }} ). The page has to be the result of a POST request and the server has to be accessed via a proxy.

@stloyd
Copy link
Contributor

stloyd commented Aug 20, 2018

@jmartinsnexity Try locally fix from: #28144

@jmartinsnexity
Copy link
Author

Thank you @stloyd but the #28144 fix doesn't change anything for my problem.

After reviewing some of last issues, I think my problem might be related to the #28225 or #28226

@nicolas-grekas
Copy link
Member

Please confirm #28241 fixes your issue.

@jmartinsnexity
Copy link
Author

jmartinsnexity commented Aug 23, 2018

Hello @nicolas-grekas, i'm sorry to tell that the #28241 does not fix the issue.
I may have misslead you by saying that my issue could be related with #cve-2018-14774.

As I couldn't reproduce it locally, I set up a new environnement on production server in order to do some debug.
Here is what I found :
The Exception is raised by Component/HttpKernel/EventListener/FragmentListener line 90.
This occurs because on line 86 signer->check() return false, which is caused by a different URI hash result in Component/HttpKernel/UriSigner.

If the MasterRequest is GET, the subRequests are signed by HTTPS url and checked by HTTPS url : it works.
If the MasterRequest is POST, the subRequests are signed by HTTPS url but checked by HTTP url : it fails.

When it fails, it means that in Component/HttpFoundation/Request, the function isSecure returns false.
After reading the function, I think the problem might be linked with the value returned by "$this->getTrustedValues(self::HEADER_CLIENT_PROTO)"

In v3.3.17, $this->getTrustedValues(self::HEADER_CLIENT_PROTO) returns ["https"].
In v3.3.18, it returns [].

@jmartinsnexity jmartinsnexity changed the title Weird behavior when updating to 3.3.18 (security patch #cve-2018-14774) Weird behavior when updating to 3.3.18 Aug 23, 2018
@nicolas-grekas
Copy link
Member

@jmartinsnexity would you be able to create a small app that reproduces the issue with e.g. Symfony 2.8 or 3.4 (3.3 is not maintained anymore) when behind your varnish setup? It'd like to clone it and run it locally. Thanks.

@nicolas-grekas
Copy link
Member

Alternatively, could you try reverting this patch partially and figure out if why change breaks your case? Trying to debug the internal state changes around it would be awesome if you can.

@jmartinsnexity
Copy link
Author

jmartinsnexity commented Aug 23, 2018

@nicolas-grekas
I found the main difference in SubRequest headers between 3.3.17 and 3.3.18 when MasterRequest is POST.

Let say my browser IP is 1.1.1.1.
v3.3.17 headers in SubRequest :
X_FORWARDED_FOR : "1.1.1.1, 127.0.0.1"
X_FORWARDED_PROTO : "https"

v3.3.18 headers in SubRequest :
X_FORWARDED_FOR : "127.0.0.1"
X_FORWARDED_PROTO : NULL

I guess the new SubRequestHandler removes the 'X_FORWARDED_PROTO' header for Cached SubRequests even if the connection is secure.

I'll try to go into deeper debug to find out what lines break my case.

@jmartinsnexity
Copy link
Author

jmartinsnexity commented Aug 23, 2018

When the method of the masterRequest is POST, the REMOTE_ADDR in the SubRequest is 127.0.0.1 and not the IP of the LoadBalancer.

Is it possible that in SubRequestHandler line 44 "IpUtils::checkIp($remoteAddr, $trustedProxies)" considers 127.0.0.1 as unsecure, and removes X_FORWARDED_* headers on line 46 ?

@nicolas-grekas
Copy link
Member

Are you sure you defined your trusted proxies correctly, ie the list contains the ip of varnish as seen from PHP?

@nicolas-grekas
Copy link
Member

Can you spot any different in the incoming master request except POST?

@nicolas-grekas
Copy link
Member

I'm running out of ideas so a reproducer would be very welcome.

@jmartinsnexity
Copy link
Author

@nicolas-grekas
Please consider this my last chance before developing a whole new app to reproduce my case.
Thank you for your time and attention.

I came out with a solution, let me explain, you'll know much better than me if it's good. Everything is about Symfony/Component/HttpKernel/HttpCache/SubRequestHandler.php.

As mentioned earlier, i didn't understand why 127.0.0.1 is not set as trusted proxy before the handle function removes untrusted values, so I tried to pull up 5 lines (from line 79-83 to 42-46) :

        // ensure 127.0.0.1 is set as trusted proxy
        if (!IpUtils::checkIp('127.0.0.1', $trustedProxies)) {
            Request::setTrustedProxies(array_merge($trustedProxies, array('127.0.0.1')), Request::getTrustedHeaderSet());
        }

Then, in the "// remove untrusted values" block, I replaced "$trustedProxies" by "Request::getTrustedProxies()", as it's been modified just before.

This modification preserves the trusted "X_FORWARDED_PROTO" request header, so the signed subrequest hash is correct when checking it.
All my render_esi twig blocks are now rendered, even behind my Load Balancer, on POST request.

Full modified handle function :

    public static function handle(HttpKernelInterface $kernel, Request $request, $type, $catch)
    {
        // save global state related to trusted headers and proxies
        $trustedProxies = Request::getTrustedProxies();
        $trustedHeaderSet = Request::getTrustedHeaderSet();
        $trustedHeaders = array(
            Request::HEADER_FORWARDED => Request::getTrustedHeaderName(Request::HEADER_FORWARDED, false),
            Request::HEADER_X_FORWARDED_FOR => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_FOR, false),
            Request::HEADER_X_FORWARDED_HOST => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_HOST, false),
            Request::HEADER_X_FORWARDED_PROTO => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_PROTO, false),
            Request::HEADER_X_FORWARDED_PORT => Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_PORT, false),
        );

        // ensure 127.0.0.1 is set as trusted proxy
        if (!IpUtils::checkIp('127.0.0.1', $trustedProxies)) {
            Request::setTrustedProxies(array_merge($trustedProxies, array('127.0.0.1')), Request::getTrustedHeaderSet());
        }
        
        // remove untrusted values
        $remoteAddr = $request->server->get('REMOTE_ADDR');
        if (!IpUtils::checkIp($remoteAddr, Request::getTrustedProxies())) {
            foreach (array_filter($trustedHeaders) as $name) {
                $request->headers->remove($name);
            }
        }

        // compute trusted values, taking any trusted proxies into account
        $trustedIps = array();
        $trustedValues = array();
        foreach (array_reverse($request->getClientIps()) as $ip) {
            $trustedIps[] = $ip;
            $trustedValues[] = sprintf('for="%s"', $ip);
        }
        if ($ip !== $remoteAddr) {
            $trustedIps[] = $remoteAddr;
            $trustedValues[] = sprintf('for="%s"', $remoteAddr);
        }

        // set trusted values, reusing as much as possible the global trusted settings
        if ($name = $trustedHeaders[Request::HEADER_FORWARDED]) {
            $trustedValues[0] .= sprintf(';host="%s";proto=%s', $request->getHttpHost(), $request->getScheme());
            $request->headers->set($name, implode(', ', $trustedValues));
        }
        if ($name = $trustedHeaders[Request::HEADER_X_FORWARDED_FOR]) {
            $request->headers->set($name, implode(', ', $trustedIps));
        }
        if (!$name && !$trustedHeaders[Request::HEADER_FORWARDED]) {
            Request::setTrustedProxies($trustedProxies, $trustedHeaderSet | Request::HEADER_X_FORWARDED_FOR);
            $request->headers->set(Request::getTrustedHeaderName(Request::HEADER_X_FORWARDED_FOR, false), implode(', ', $trustedIps));
        }

        // fix the client IP address by setting it to 127.0.0.1,
        // which is the core responsibility of this method
        $request->server->set('REMOTE_ADDR', '127.0.0.1');

        try {
            return $kernel->handle($request, $type, $catch);
        } finally {
            // restore global state
            Request::setTrustedProxies($trustedProxies, $trustedHeaderSet);
        }
    }

@nicolas-grekas
Copy link
Member

This patch turns 127.0.0.1 as an always trusted proxy. I'm not sure why we should do this...

nicolas-grekas added a commit that referenced this issue Aug 24, 2018
…meters (nicolas-grekas)

This PR was merged into the 2.8 branch.

Discussion
----------

[HttpKernel] fix forwarding trusted headers as server parameters

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28233, #28226, #28225, #28240
| License       | MIT
| Doc PR        | -

Commits
-------

9295348 [HttpKernel] fix forwarding trusted headers as server parameters
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 24, 2018

I just merged #28241 into 3.4. Could you update to 3.4@dev before trying to reproduce (and check again that 3.4@dev still has the issue?) Please also double check your trusted proxies list.

@jmartinsnexity
Copy link
Author

Thank you @nicolas-grekas, the problem is solved in latest 3.4 version !

@nicolas-grekas
Copy link
Member

Awesome, thanks for checking.

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

5 participants