[HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For #18688

magnusnordlander · 2016-05-02T12:45:44Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	symfony/symfony-docs#6526

Emit a warning when a request has both a trusted Forwarded header and a trusted X-Forwarded-For header, as this is most likely a misconfiguration which causes security issues.

Forwarded header and a trusted X-Forwarded-For header, as this is most likely a misconfiguration which causes security flaws.

fabpot · 2016-05-03T19:02:55Z

src/Symfony/Component/HttpFoundation/Request.php

+        $hasTrustedClientIpHeader = self::$trustedHeaders[self::HEADER_CLIENT_IP] && $this->headers->has(self::$trustedHeaders[self::HEADER_CLIENT_IP]);
+
+        if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader) {
+            trigger_error('The request has both a trusted Forwarded header and a trusted Client IP header. This is likely a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them. When both headers are set and trusted, this method returns only IPs from the Forwarded header.', E_USER_WARNING);


I'm wondering if we could instead throw an exception here. What do you think? Any use case where setting both would be legitimate?

It could be legitimate as a legacy measure from proxies, if both headers contain the same information, I suppose.

That can be detected of course, I mean, it's just a matter of parsing both headers and see if they have the same IPs.

I'll update this PR to do that, because we probably don't want neither to warn nor throw exceptions if a proxy sends both headers for backwards compatibility.

magnusnordlander · 2016-05-05T11:53:00Z

@fabpot: I have now updated the PR to throw exceptions instead, and to not do so when both the Forwarded and X-Forwarded-For header agree with each other.

I am a bit worried about the fact that the very commonly used getClientIp method now throws exceptions when users previously did not expect it to, though. I mean, the request is inconsistent, so clearly we need to do something, but this can happen when the user least expects it to. Should I perhaps "validate" the request early on in the request lifecycle, to make sure that if we're going to throw, we throw early?

stloyd · 2016-05-05T11:57:04Z

src/Symfony/Component/HttpFoundation/Request.php

-            }
+        if ($hasTrustedForwardedHeader && $hasTrustedClientIpHeader && $forwardedClientIps !== $xForwardedForClientIps) {
+            throw new ConflictingHeadersException('The request has both a trusted Forwarded header and a trusted Client IP header, conflicting with each other with regards to the originating IP addresses of the request. This is the result of a misconfiguration. You should either configure your proxy only to send one of these headers, or configure Symfony to distrust one of them.');
+        } elseif (!$hasTrustedForwardedHeader && !$hasTrustedClientIpHeader) {


This can be simple if cause you throw exception in if before it.

…s to the Forwarded header (magnusnordlander) This PR was squashed before being merged into the 2.7 branch (closes #6526). Discussion ---------- Documented how to configure Symfony correctly with regards to the Forwarded header | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | >2.7 | Fixed tickets | Ref: symfony/symfony#18688 Commits ------- 87ab598 Documented how to configure Symfony correctly with regards to the Forwarded header

fabpot · 2016-05-13T15:06:26Z

@magnusnordlander I agree with you that we should bail out as early as possible and consistently.

stof · 2016-05-13T15:13:30Z

src/Symfony/Component/HttpFoundation/Request.php

@@ -1930,4 +1922,35 @@ private function isFromTrustedProxy()
    {
        return self::$trustedProxies && IpUtils::checkIp($this->server->get('REMOTE_ADDR'), self::$trustedProxies);
    }
+
+    private function normalizeAndFilterClientIps($clientIps, $ip)


I would typehint the first argument as array

magnusnordlander · 2016-05-13T19:09:38Z

I'll give some consideration to where to put this validation, and have an updated PR ready tomorrow (also fixing the other two things, of course).

nicolas-grekas · 2016-05-19T08:06:07Z

Status: needs work

fabpot · 2016-06-15T07:14:20Z

@magnusnordlander Do you have time in the next few days to update this PR?

magnusnordlander · 2016-06-22T12:30:57Z

I'll have time this Friday.

…ion handling)

magnusnordlander · 2016-06-25T10:40:18Z

So, the code is updated. The remaining build error is because the composer.json of HttpKernel and FrameworkBundle needs to be updated to require the version of HttpFoundation and HttpKernel respectively, that this is released in. I don't know how to handle that without actually making a release. Any help there?

magnusnordlander · 2016-06-27T09:52:31Z

Ping @fabpot

fabpot · 2016-06-28T05:25:17Z

src/Symfony/Component/HttpKernel/EventListener/ValidateRequestClientIpListener.php

+ *
+ * @author Magnus Nordlander <magnus@fervo.se>
+ */
+class ValidateRequestClientIpListener implements EventSubscriberInterface


What about naming this class ValidateRequestListener to allow adding more checks in the future?

We already have a generic response_listener, so naming it request_listener might be an option as well (not sure about possible conflicts with existing end-user listeners though).

I should think that validate_request_listener has less potential for conflict

fabpot · 2016-06-28T05:29:25Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml

@@ -46,5 +46,9 @@
            <argument type="service" id="request_stack" />
            <tag name="kernel.event_subscriber" />
        </service>
+
+        <service id="validate_request_client_ip_listener" class="Symfony\Component\HttpKernel\EventListener\ValidateRequestClientIpListener">


If we rename the class as I suggest, this should be renamed accordingly (validate_request_listener).

fabpot · 2016-06-28T14:18:18Z

👍

fabpot · 2016-06-29T07:02:04Z

Thank you @magnusnordlander.

This PR was merged into the 2.7 branch. Discussion ---------- fixed HttpKernel dependencies after #18688 | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Commits ------- f809f3e fixed HttpKernel dependencies after #18688

…pKernel (nicolas-grekas) This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] Inline ValidateRequestListener logic into HttpKernel | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18688 #19216 | License | MIT | Doc PR | - I propose to inline the listener introduced in #18688 into HttpKernel. Commits ------- 9d3ae85 [HttpKernel] Inline ValidateRequestListener logic into HttpKernel

* 2.7: [HttpKernel] Inline ValidateRequestListener logic into HttpKernel fixed HttpKernel dependencies after #18688 Conflicts: src/Symfony/Component/HttpKernel/composer.json

* 2.8: [FrameworkBundle] Fix fixtures [HttpKernel] Inline ValidateRequestListener logic into HttpKernel fixed HttpKernel dependencies after #18688 Conflicts: src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/builder_1_services.txt src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/definition_1.txt src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/definition_2.txt src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/event_dispatcher_1_events.txt src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/legacy_synchronized_service_definition_1.txt src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/legacy_synchronized_service_definition_2.txt src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/parameter.txt src/Symfony/Bundle/FrameworkBundle/Tests/Fixtures/Descriptor/route_collection_1.txt src/Symfony/Bundle/FrameworkBundle/composer.json src/Symfony/Component/HttpKernel/composer.json

* 3.0: [FrameworkBundle] Fix fixtures [HttpKernel] Inline ValidateRequestListener logic into HttpKernel fixed HttpKernel dependencies after #18688 Conflicts: src/Symfony/Component/HttpKernel/HttpKernel.php src/Symfony/Component/HttpKernel/Tests/HttpKernelTest.php

* 3.1: [Form] fixed ChoiceTypeTest after #17822 [DoctrineBridge] fixed DoctrineChoiceLoaderTest by removing deprecated factory [ci] Upgrade phpunit wrapper deps [FrameworkBundle] Fix fixtures [HttpKernel] Inline ValidateRequestListener logic into HttpKernel fixed HttpKernel dependencies after #18688

leandigital · 2016-07-21T23:24:00Z

Late to the party here, but I just experienced the exception after upgrading to 2.8.8 and landed on this page. The server is behind Akamai, and looks like it's sending both Forwarded and X-Forwarded-For headers. I checked the new documentation and tried disallowing one of the headers, but it didn't help and the error didn't go away. Any ideas why and how do I only allow one of those headers? Here is the snippet I used: Request::setTrustedHeaderName(Request::HEADER_FORWARDED, null);

ninsuo · 2016-07-27T15:26:09Z

Well some of our members use proxies (Forwarded) and we use reverse proxies (X-Forwarded-For), as a result, boom. Do I miss something?

magnusnordlander · 2016-07-28T22:34:49Z

@iSyndicate So, first of all, it seems odd that Akamai is sending you different information about the provenance of the request in the different headers. That's not been my experience with them, and I suspect might be a configuration issue. However, you should, as you have attempted, be able to fix that by distrusting one of the headers, and that snippet is indeed correct. In which file, and where, did you put that snippet?

@ninsuo: Well, yes. You can't trust the information coming from random strangers (or in this case rather, random clients) on the Internet. Ideally your reverse proxy would be filtering out the untrusted Forwarded header (because, since you have marked it as a trusted proxy, you are relying on it for accurate information on the provenance of the request, and the Forwarded header coming from your clients is not trustworthy), but if that is not possible, you should make sure Symfony distrusts the Forwarded header, as detailed here: http://symfony.com/doc/current/request/load_balancer_reverse_proxy.html#my-reverse-proxy-sends-x-forwarded-for-but-does-not-filter-the-forwarded-header

ninsuo · 2016-07-29T09:47:21Z

Thank you for clarification.

But as everybody can set any header I don't see any point of using them as a reliable information. We do trust and use only the headers our nginx is setting, no matter what our members are setting.

We can use Referer header (as well as lots of others) to flag client as "using a proxy" and giving him a lesser trust score (more chances to get 3DSecure, MFA, ...) and removing it is quite inconvenient.

hightoxicity · 2016-07-29T09:51:47Z

@magnusnordlander

You can not trust X-Forwarded, Forwarded, both of them, your system engineer is in charge of reading remoteip_addr on the first front node of your infrastructure and provide you (to your framework) the value read in a secure way that you can trust (according together), I think about a dedicated header, your system engineer sets/overrides in all cases. You can keep Forwarded and X-Forwarded headers for information and to decorate... I do not see very well where the Symfony framework has to be intelligent on such security problem.

magnusnordlander · 2016-07-29T10:38:19Z

This particular piece of code only runs when you have configured Symfony to trust a proxy server. By default, Symfony doesn't trust any proxy server, and does not consider X-Forwarded-For nor Forwarded to contain reliable information on the Client IP.

…:setTrustedProxies() - deprecate not setting it (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | no tests yet | Fixed tickets | - | License | MIT | Doc PR | - Follow up of #18688 PR adds a second `$trustedHeaderSet` argument to `Request::setTrustedProxies()`, can be either `Request::HEADER_FORWARDED` or `Request::HEADER_X_FORWARDED_ALL` to set which header to trust from your proxies - the idea being that without this info, one will get some `ConflictingHeadersException`, but those may be lost in the logs. Commits ------- d3c9604 [HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it

Emit a warning when a request has both a trusted

4c262d4

Forwarded header and a trusted X-Forwarded-For header, as this is most likely a misconfiguration which causes security flaws.

carsonbot added the Status: Needs Review label May 2, 2016

magnusnordlander mentioned this pull request May 2, 2016

Documented how to configure Symfony correctly with regards to the Forwarded header symfony/symfony-docs#6526

Closed

javiereguiluz added Bug HttpFoundation labels May 2, 2016

Updated according to fabbot review

da4b6c5

fabpot reviewed May 3, 2016
View reviewed changes

magnusnordlander added 2 commits May 5, 2016 13:30

Changed warning to exception, and only throws when headers disagree.

f0c3d15

CS fix

2eea0f1

stloyd reviewed May 5, 2016
View reviewed changes

stof reviewed May 13, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 19, 2016

magnusnordlander added 5 commits June 25, 2016 11:29

Added a listener to validate requests

c017290

Changed listener to only throw on master requests (to preserve except…

99b072e

…ion handling)

Added fabbot CS fixes

41d2603

Made the profiler handle unknown client IPs.

ea691e7

Updated tests

a9f864d

fabpot reviewed Jun 28, 2016
View reviewed changes

magnusnordlander added 2 commits June 28, 2016 15:53

Renamed listener and CS fixes.

66f19f1

Fixed test failures

d52d098

fabpot closed this in 2d37230 Jun 29, 2016

HeahDude added a commit to HeahDude/symfony that referenced this pull request Jun 29, 2016

fixed HttpKernel dependencies after symfony#18688

f809f3e

nicolas-grekas mentioned this pull request Jun 29, 2016

[HttpKernel] Inline ValidateRequestListener logic into HttpKernel #19217

Merged

nicolas-grekas added a commit that referenced this pull request Jun 29, 2016

Merge branch '2.7' into 2.8

77053e7

* 2.7: [HttpKernel] Inline ValidateRequestListener logic into HttpKernel fixed HttpKernel dependencies after #18688 Conflicts: src/Symfony/Component/HttpKernel/composer.json

This was referenced Jun 30, 2016

Release v2.7.15 #19248

Merged

Release v2.8.8 #19250

Merged

Release v3.0.8 #19251

Merged

Release v3.1.2 #19252

Merged

arjenm mentioned this pull request Jun 30, 2016

Default TrustedHeaderNames are not standard... are they? #17641

Closed

AntoineLemaire mentioned this pull request Jul 7, 2016

ConflictingHeadersException with routing?callback=fos.Router.setData FriendsOfSymfony/FOSJsRoutingBundle#255

Closed

nicolas-grekas mentioned this pull request Mar 1, 2017

[HttpFoundation] Add $trustedHeaderSet arg to Request::setTrustedProxies() - deprecate not setting it #21830

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For #18688

[HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For #18688

magnusnordlander commented May 2, 2016 •

edited

Loading

fabpot May 3, 2016

magnusnordlander May 3, 2016 •

edited

Loading

magnusnordlander May 3, 2016

magnusnordlander commented May 5, 2016

stloyd May 5, 2016

fabpot commented May 13, 2016

stof May 13, 2016

magnusnordlander commented May 13, 2016

nicolas-grekas commented May 19, 2016

fabpot commented Jun 15, 2016

magnusnordlander commented Jun 22, 2016

magnusnordlander commented Jun 25, 2016

magnusnordlander commented Jun 27, 2016

fabpot Jun 28, 2016

fabpot Jun 28, 2016

magnusnordlander Jun 28, 2016

fabpot Jun 28, 2016

fabpot commented Jun 28, 2016

fabpot commented Jun 29, 2016

leandigital commented Jul 21, 2016

ninsuo commented Jul 27, 2016 •

edited

Loading

magnusnordlander commented Jul 28, 2016

ninsuo commented Jul 29, 2016

hightoxicity commented Jul 29, 2016 •

edited

Loading

magnusnordlander commented Jul 29, 2016

[HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For #18688

[HttpFoundation] Warning when request has both Forwarded and X-Forwarded-For #18688

Conversation

magnusnordlander commented May 2, 2016 • edited Loading

Choose a reason for hiding this comment

magnusnordlander May 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magnusnordlander commented May 5, 2016

Choose a reason for hiding this comment

fabpot commented May 13, 2016

Choose a reason for hiding this comment

magnusnordlander commented May 13, 2016

nicolas-grekas commented May 19, 2016

fabpot commented Jun 15, 2016

magnusnordlander commented Jun 22, 2016

magnusnordlander commented Jun 25, 2016

magnusnordlander commented Jun 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 28, 2016

fabpot commented Jun 29, 2016

leandigital commented Jul 21, 2016

ninsuo commented Jul 27, 2016 • edited Loading

magnusnordlander commented Jul 28, 2016

ninsuo commented Jul 29, 2016

hightoxicity commented Jul 29, 2016 • edited Loading

magnusnordlander commented Jul 29, 2016

magnusnordlander commented May 2, 2016 •

edited

Loading

magnusnordlander May 3, 2016 •

edited

Loading

ninsuo commented Jul 27, 2016 •

edited

Loading

hightoxicity commented Jul 29, 2016 •

edited

Loading