Adminhtml sales view - Escape remoteIp #5200
Merged
Conversation
This file contains hidden or 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
It is quite trivial to make the remoteIp field map to something other than the REMOTE_ADDR header. Adding the following to `app/etc/di.xml` will allow you to use HTTP_X_FORWARDED_For as the header you're interested in. With this change a customer can spoof their x forwarded header and inject javascript which will run when an admin views the order in the admin panel.
```
<type name="Magento\Framework\HTTP\PhpEnvironment\RemoteAddress">
<arguments>
<argument name="alternativeHeaders" xsi:type="array">
<item name="http_x_forwarded_for" xsi:type="string">HTTP_X_FORWARDED_FOR</item>
</argument>
</arguments>
</type>
```
A developer may decide to make this change at some point when they want to force all calls to RemoteAddress::getRemoteAddress() to use the X_FORWARDED_FOR instead. Maybe they're working on a plugin that requires it, and have many proxies between the customer and the Magento server.
This vulnerability isn't in the wild for M2 as it requires specific developer changes but I know the plugin ecosystem for M1 had many strange modules, this small change should help protect us from future laziness/craziness.
Either way, this is easily preventable. Simply escaping the output with $block->escapeHtml() will do the trick.
|
Hi @convenient , |
NadiyaS
added
the
Issue: Ready for Work
|
Thanks @NadiyaS. Internally for you guys this also references APPSEC-1474, although it was deemed a non-issue as it requires developer tweaking to expose it. |
vkorotun
added
Component: Sales
bug report
bugfix
and removed
CS
Issue: Ready for Work
mmansoor-magento
pushed a commit
that referenced
this pull request
Aug 12, 2016
mmansoor-magento
pushed a commit
that referenced
this pull request
Aug 12, 2016
convenient
deleted the
feature/adminhtml-slight-potential-for-xss-order-remoteip
branch
vkorotun
added
Issue: Ready for Work
magento-engcom-team
pushed a commit
that referenced
this pull request
Jan 15, 2020
MC-23535: [2.4] Fix Skipped MFTF Tests MC-28537
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
It is quite trivial to make the remoteIp field map to something other than the
REMOTE_ADDRheader. Adding the following toapp/etc/di.xmlwill allow you to useHTTP_X_FORWARDED_FORas the header you're interested in. With this change a customer can spoof their x forwarded header and inject javascript which will run when an admin views the order in the admin panel.A developer may decide to make this change at some point when they want to force all calls to
RemoteAddress::getRemoteAddress()to use theX_FORWARDED_FORinstead. Maybe they're working on a plugin that requires it, and have many proxies between the customer and the Magento server. Maybe they just want to hook into some useful related functionality. ¯_(ツ)_/¯This vulnerability isn't in the wild for M2 as it requires specific developer changes but I know the plugin ecosystem for M1 had many strange modules, this small change should help protect us from future laziness/craziness.
Either way, this is easily preventable. Simply escaping the output with $block->escapeHtml() will do the trick.