Request should not trust any header names by default #20178
Comments
|
@nicolas-grekas quick ping to see if this may be extra relevant/timely, as it's related to #21830 |
|
Nice, then we can close this one ! Here is why: but really, this 2nd arg is a bitfield, so that in AWS ELBs, you just need to do eg: Done! |
|
Okay, but in 3.3, just It might be worth considering changing that default even in 3.3, to "immediate breakage", in the interest of security, as 99.975% of code out there does that call in Or at least change it in 3.4, as that will be the LTS version, and I really really really really really think the defaults are harmful. But I'll let you decide that :) |
|
I agree with @dzuelke here that we should make the second new arg mandatory as of 3.3. But I would not make the default "optimized" for AWS, that's a doc issue instead IMHO. |
|
Very must support this "BC-break" in the immediately next minor version, as well. |
|
Can do @nicolas-grekas, want to give me a deadline? |
|
May 15th if possible, that should match the first betas of 3.3 |
…) takes a new required $trustedHeaderSet argument (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [BC BREAK][HttpFoundation] Request::setTrustedProxies() takes a new required $trustedHeaderSet argument | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | yes | BC breaks? | yes | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20178 | License | MIT | Doc PR | - As discussed in linked issue, and already deprecated by #21830 Commits ------- 72e2895 [BC BREAK][HttpFoundation] Request::setTrustedProxies() takes a new required $trustedHeaderSet argument
(also see symfony/symfony-docs#7045)
http://symfony.com/doc/current/request/load_balancer_reverse_proxy.html and http://symfony.com/doc/current/components/http_foundation/trusting_proxies.html talk about trusting proxies, and http://symfony.com/doc/current/request/load_balancer_reverse_proxy.html#but-what-if-the-ip-of-my-reverse-proxy-changes-constantly in particular mentions AWS as an example.
AWS ELBs do not set a
Forwardedheader, making it necessary to follow the instructions at 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, but they also do not set anX-Forwarded-Host(only…-For,…-Portand…-Proto), which means, that for a very popular use case (running on AWS, or products that build on it, e.g. Heroku), applications would be vulnerable to spoofing of those headers.My hunch is that a lot of developers don't realize that they need to explicitly distrust these headers, especially as the special case documentation and handling logic for
Forwarded, unlike the other headers, has been introduced relatively recently.I propose that out of the box, when a developer calls
Request::setTrustedProxies(), no header names are trusted by default, and they must always be explicitly opted into usingRequest::setTrustedHeaderName(). That's only slightly more work, but means that the default is safe with an empty whitelist, and developers, having to investigate what headers really are being set in their environment, are much more likely to end up with valid settings that do not compromise application security./cc @fabpot, who asked me to open this issue (I may have time for a PR soon-ish)
The text was updated successfully, but these errors were encountered: