Check X-Forwarded-Proto header in Environment::ssl

[bahlo]

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Proto

[aschempp]

How can we trust this header? As far as I understand, it could be easily set from any browser?

[bahlo]

Do we need to trust the header?

A bit of background:
We use Contao behind a proxy that's also doing HTTPS. Contao get's called internally via HTTP and doesn't detect SSL. Because of that we have some http://-urls and get mixed content warnings.
We can set the X-Forwarded-Proto header in the proxy and detect it with this PR in Contao.
I manually made the change in the system and it works without problems.

EDIT: Thank you for the quick reply, really appreciate it.

[Toflar]

The intention is correct but the bugfix is not. You have to make sure that this header is only accepted when the ip is trusted. So you need to check for \Config::get('proxyServerIps') as we do in ip().

[aschempp]

Maybe the correct solution for you would be to set the SSL proxy config?
https://github.com/bahlo/core/blob/38e1da6a236b21b4a77442eedd7fec4d648c8095/system/modules/core/library/Contao/Environment.php#L329

[bahlo]

Added a check for \Config::get('proxyServerIps') using $_SERVER['REMOTE_ADDR'] since it should contain the ip of the proxy.

Edit: I created two if statements because in_array and \Config::get are time consuming and in most cases unnecessary.

[Toflar]

That looks way better. Does that work for you?

[bahlo]

Yes, just tried it and it works 👍

[Toflar]

Well done then. Thx for the PR. BTW, thanks to Symfony this would've worked out of the box in Contao 4. But imho this is a bugfix so approving to merge this into 3.5.

[bahlo]

Great 🎉

[ausi]

If $_SERVER['HTTP_X_FORWARDED_PROTO'] is set to http and the proxy is trusted, ssl() would return true if the proxy communicates via https with the webserver.

How about this one?

if (
    isset($_SERVER['HTTP_X_FORWARDED_PROTO']) 
    && in_array($_SERVER['REMOTE_ADDR'], trimsplit(',', \Config::get('proxyServerIps')))
) {
    return $_SERVER['HTTP_X_FORWARDED_PROTO'] == 'https';
}

return ($_SERVER['SSL_SESSION_ID'] || $_SERVER['HTTPS'] == 'on' || $_SERVER['HTTPS'] == 1);

[discordier]

@ausi You are right, yet however this is a rather exotic usecase to communicate via HTTP with an proxy that talks HTTPS internally.
Nice catch though.

[aschempp]

How does "having a proxy" tell us the header can be trusted? That would only be the case if such a header was removed by a proxy - which I doubt is the case. There are only two suitable solutions to me: 1. we can agree that faking the protocol is no security risk, then the first implementation was enough. 2. only a manual override for the rare case is possible, e.g. by setting $_SERVER variable in initconfig.php

[Toflar]

You are the one configuring the proxy which is why you trust it.

[aschempp]

I trust the proxy, but I don't know if I can trust the header.

[fritzmg]

@aschempp you can trust $_SERVER['REMOTE_ADDR'] just as much as $_SERVER['HTTPS'], can you not?

Btw. this ticket is related to #7542 where @leofeyer also provided a custom configuration for the initconfig.php for such cases (where your server is behind an SSL proxy that only communicates via HTTP).

[aschempp]

@aschempp you can trust $_SERVER['REMOTE_ADDR'] just as much as $_SERVER['HTTPS'], can you not?

No, because these are set by the webserver (Apache/Nginx) and not just forwarded from the browser.

[ausi]

Symfony uses the X-Forwarded-Proto header too and checks if the request is from a trusted proxy, so I think it should be safe if we do the same in Contao 3.5.

[leofeyer]

So do I merge @bahlo's or @ausi's changes now?

[Toflar]

Yes. @ausi's variant.

[leofeyer]

I assume that I need to use System::getContainer()->get('request_stack')->getCurrentRequest()->getClientIp() instead of $_SERVER['REMOTE_ADDR'] in Contao 4, don't I?

[Toflar]

Yes, we should map every method to the current request in contao 4 if possible. @discordier wanted to create a PR for this long ago...

[leofeyer]

@Toflar I was asking because of your comment above:

BTW, thanks to Symfony this would've worked out of the box in Contao 4.

[leofeyer]

Fixed in 40a0541.

[Toflar]

Ah, yeah if we map everything to the request, we can just use $request->isSecure().