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
[HttpKernel] InlineFragmentRenderer should add the X-Forwarded-For of real client IP #7557
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point the condition inside
if
always evaluates totrue
, so the code following this block will never be reached. Anyway this block just duplicates the original logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, it brings back the vulnerability fixed by introducing the list of trusted proxies, since it bypasses the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just work if the X-Forwarded-For exists...
This dont duplicates the original logic, it just get the client ip address from the X-Forwarded-For header (client, proxy1, proxy2, ...) instead of the hardcoded 127.0.0.1... this was made to avoid this hardcoded IP address
It does not bypass the check... there is no check for this, once a time that the real client IP address is not at the trustedProxies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kinncj Getting it from the
X-Forwarded-For
when trusting proxies is already handled by the existing code. The returned ip should always be the closest untrusted ip, not the original IP (as headers can be forged by the client)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X-Forwarded-For
should return always the original IP.If this is the validation it must be wrong...
This validation is breaking the original X-Forwarded-For semantic...
The validation must be a new issue and not related to this PR... you cannot avoid the validation removing the original IP address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kinncj As mentioned by @stof, the
X-Forwarded-For
header may be forged by client, so you should not trust its contents. But if you are sure you need this, you can always get the header contents by yourself. Or you could suggest adding something likeRequest::getUntrustedClientIp()
, but you should not introduce security holes in the name of your convenience :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talking with blanco, we found an another sollution.
check: #7559
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And FYI, anything can be forged by the client... even a X-Parmegiana-For... you cannot act as a firewall/proxy for this...
Anyway,
rails/rails#2490
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And again, the InlineFragmentRenderer is not a real request, so it cannot overwrite the original REMOTE_ADDR to 127.0.0.1
RFC 2616, Sec 5.