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

[HttpFoundation] Request::isSecure does not check whether the proxy is trusted #11583

Closed
ged15 opened this Issue Aug 6, 2014 · 14 comments

Comments

Projects
None yet
7 participants
@ged15
Copy link
Contributor

commented Aug 6, 2014

Hello!

Request::isSecure checks whether there is at least 1 trusted proxy set ((bool)self::$trustedProxies), then validates the headers:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Request.php#L1129

    public function isSecure()
    {
        if (self::$trustedProxies && self::$trustedHeaders[self::HEADER_CLIENT_PROTO] && $proto = $this->headers->get(self::$trustedHeaders[self::HEADER_CLIENT_PROTO])) {
            return in_array(strtolower(current(explode(',', $proto))), array('https', 'on', 'ssl', '1'));
        }

        return 'on' == strtolower($this->server->get('HTTPS')) || 1 == $this->server->get('HTTPS');
    }

Shouldn't it actually check whether the proxy is actually trusted by validating its IP address here?

@weaverryan

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

Hi there!

I immediately agree with what you're seeing, but I'm sure we're missing something. BUT, I can reproduce what you're talking about:

<?php

require __DIR__.'/vendor/autoload.php';

use Symfony\Component\HttpFoundation\Request;

$request = Request::create('http://example.com/');
// my remote address is some random IP address, not trusted
$request->server->set('REMOTE_ADDR', '3.3.3.3');
// I "fake" that this is a secure request
$request->headers->set('X_FORWARDED_PROTO', 'https');

// We trust some IP's, but not the IP that this user is coming from
Request::setTrustedProxies(array('1.1.1.1', '2.2.2.2'));

// will dump true
var_dump($request->isSecure());

In this case, I am requesting from NOT a trusted proxy. But because the Request trusts other proxies, I'm able to supply the X_FORWARDED_PROTO header to "fool" it into thinking that I'm connecting securely.

So again, I agree, but I still think there's something we're not seeing :). Is being able to spoof that a request is HTTPS when it is actually HTTP maybe not a security thread? Anyone else?

@ricardclau

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2014

@weaverryan I think your request is not secure because it has the http scheme

@ged15 The convention is to add the trusted proxies either in the front controller via code or using config and then via a compiler pass in the FrameworkBundle. I am not sure what you mean by your comment, can you explain me a bit more?

@weaverryan

This comment has been minimized.

Copy link
Member

commented Aug 6, 2014

@ricardclau Actually, in my code example, it prints out true at the end, meaning that I'm able to create a request that is not secure (it does use http in reality) but ultimately looks secure to the server (because I'm spoofing the header), even though I'm making the request from a non-trusted proxy IP address.

Or maybe I'm not seeing what you're saying :).

@ricardclau

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2014

@weaverryan LOL! I thought you were saying that it was returning false :D

TBH I remember having the issue and testing it back in 2.0 and 2.1 (when the way of setting the https trusting changed in 2.1.6 if I am not wrong) but maybe a regression was introduced afterwards or some edge case

I will test some more situations and provide a further analysis

@ricardclau

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2014

Ok I am more less understanding what it does... and I find it a bit weird

  • If we are not behind a proxy, there is no trusted proxy, we won´t have any of the headers and we rely on $this->server->get('HTTPS') which will only be informed if we allow connections via some port (usually 443) and this is when Apache / Nginx and others add this header for secure connections
  • If we are behind a proxy and add it to the trusted_proxies, we don´t check that the last server is the one we trust so I think there is a bug here. We could check that $this->server->get('REMOTE_ADDR') is in the array of the trusted proxies but in fact we should check all the chain of forwards (analogue to what getClientIp does to retrieve the original client ip). And I think this may be a security issue.

Ping @fabpot because maybe we are missing something but it looks like a quite critical bug

@ged15

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2014

@ricardclau does the referenced PR add more clarity?

@ricardclau

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

@ged15 I think it does, indeed, thanks for taking some time on this as well and let´s wait for @fabpot ´s response.

IMHO, this test should not fail unless we are all missing something about the purpose of either the trusted_proxies or the way Symfony considers a request to be "secure".

And even more than that, I think that being purist we should check all the X-Forwarded-For (and similar headers) servers in a comma separated list

Imagine for instance a chain of ELBs.

user -> elb1 -> elb2 -> http_server

elb1 could be something like checking the url and via some pattern send the request to a cluster of boxes serving some particular web service in a SOA architecture

elb2 could be a second load balancer just to distribute the load amongst servers with the exact same code

And it that scenario, in my opinion, if we add elb1 as a trusted_proxy, the request should be secure as well

Is this clear to you @ged15 @weaverryan @javiereguiluz ?

@stof

This comment has been minimized.

Copy link
Member

commented Aug 13, 2014

Given that @fabpot is on vacation, getting an answer from him will take some time.

@ricardclau

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2014

Well, this has been like this since 2.1.5 when it got added to the framework, so it can surely wait for Fabien to come back from his deserved holidays

And in fact, changing its behaviour will surely affect apps that rely on the current way

@stof stof added the HttpFoundation label Aug 18, 2014

@ged15

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2014

ping @fabpot

@fabpot fabpot added the Hackday label Sep 5, 2014

@kipit

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2014

It's seem that the usage of trustedProxies collection make it behave mostly like a toggle, except when used in client IP's filtering, which is the most important point: we cannot spoof the client IP. But, still we can spoof all other forwarded header :

  • X-Forwarded-Host
  • X-Forwarded-Port
  • X-Forwarded-Proto

IMO, from this list only the "X-Forwarded-Proto" is sensitive, the IP should be checked as @ged15 said…

@kipit

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2014

Well, in fact, i’ve read getClientIps() code to quickly, the client IP can be also spoofed when we the trustedProxies is filled, no matter from where the request really comes from. This defeat somehow the usage of trustedProxies.

Hell no! Sorry! That's perfectly fine, no matter where the request comes from, only the REMOTE_ADDR is trusted, or the first non trusted proxy address, so sysadmin should have been done is job.

@xabbuh

This comment has been minimized.

Copy link
Member

commented Jul 25, 2015

Is this still an issue given the changes that were made in #14166?

@ricardclau

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2015

Hi @xabbuh

The new code looks much better so I am pretty sure the original issue is fixed with it.
In fact, some users issues seem to confirm it (#15360)

@xabbuh xabbuh closed this Jul 28, 2015

fabpot added a commit that referenced this issue Mar 2, 2016

minor #11655 Failing test for non-trusted proxies (ged15)
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #11655).

Discussion
----------

Failing test for non-trusted proxies

Discussion in #11583
@fabpot, should this test fail?

Commits
-------

e0e82bb added tests for non-trusted proxies

@fabpot fabpot reopened this Mar 2, 2016

@fabpot fabpot closed this Mar 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.