Add permissions to enforce access to specific resource types [SEC-3811] #15655
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
…missions server-side [Security 3811]
…o enforce access control
…d possibly selected resource type
…ions for improved access control
Mark-H
added
area-security
urgent
Mark-H
added a commit
to Mark-H/revolution
that referenced
this pull request
Apr 8, 2021
…lute paths [SEC-3801] Reported by wfoojjaec, static resources can be used to read any file on the server through the use of absolute paths. This is a tricky balance between feature and vulnerability, and this fix shifts the balance to secure-by-default by requiring static resources to be within a certain path. In the new default for all sites (including upgrades), static resources **must** be in the assets folder. It supports paths that are relative to the configured path (foo.pdf => assets/foo.pdf), relative to the MODX base path if the configured path is within that path (assets/foo.pdf), and absolute paths within the configured path (/home/user/public_html/assets/foo.pdf) for maximum compatibility. By enabling the resource_static_allow_absolute setting, the previous behavior of allowing any valid absolute path to be used is restored. While that allows the user to access any file and should not be enabled lightly, some existing sites may need to do so in order to keep existing static resources working. At least then it's a conscious and informed decision to trust your users (which can be further enhanced by limiting access to static resources per modxcms#15655) I have another patch https://github.com/Mark-H/revolution/commits/security-3801 which also supports media sources, however that's a feature that needs more work/discussion and is not suitable to introduce as a security fix, so I've only included the required paths here. Sorry, Susan. That will hopefully get picked up for 3.x.
|
Can some people please test this to make sure I didn't typo any permission checks and users can still edit things after the upgrade? |
|
I will do Mark, don't worry |
opengeek
pushed a commit
that referenced
this pull request
Apr 15, 2021
Reported by wfoojjaec, static resources can be used to read any file on the server through the use of absolute paths. This is a tricky balance between feature and vulnerability, and this fix shifts the balance to secure-by-default by requiring static resources to be within a certain path. In the new default for all sites (including upgrades), static resources **must** be in the assets folder. It supports paths that are relative to the configured path (foo.pdf => assets/foo.pdf), relative to the MODX base path if the configured path is within that path (assets/foo.pdf), and absolute paths within the configured path (/home/user/public_html/assets/foo.pdf) for maximum compatibility. By enabling the resource_static_allow_absolute setting, the previous behavior of allowing any valid absolute path to be used is restored. While that allows the user to access any file and should not be enabled lightly, some existing sites may need to do so in order to keep existing static resources working. At least then it's a conscious and informed decision to trust your users (which can be further enhanced by limiting access to static resources per #15655) I have another patch https://github.com/Mark-H/revolution/commits/security-3801 which also supports media sources, however that's a feature that needs more work/discussion and is not suitable to introduce as a security fix, so I've only included the required paths here. Sorry, Susan. That will hopefully get picked up for 3.x.
|
Could I convince someone to help port this to 3.x with a high five from this incredibly cute puppy?
|
|
FYI, it is under WIP |
|
Port is added, so I guess the mission completed here :) I removed related label. |
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.
What does it do?
Adds dedicated permissions for weblinks, symlinks, and static resources to control who can:
Also includes a small tweak to the modManagerController to make sure a missing permission uses the "nice" error page, rather than a blank page.
Default policies for content editor, admin and developer are also updated to include these new permissions with the upgrade.
Why is it needed?
SEC3801 reported static resources allowing access to things it shouldn't, which included proof from the more demo site. That demo site is heavily restricted, which led me down a very deep rabbit hole where it turns out the relevant permission (new_static_resource) was not actually enforced. That resulted in this security fix to add missing permissions and making sure they're enforced.
How to test
First rebuild and run setup to add the new permissions.
Related issue(s)/PR(s)
https://community.modx.com/t/new-static-resource-permission-not-checked/3811/6