Skip to content

Restrict static resources to predefined path [SEC-3801] #15656

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

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

Mark-H
Copy link
Collaborator

@Mark-H Mark-H commented 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.
@Mark-H Mark-H added area-security urgent The issue requires attention and has higher priority over others. pr/review-needed Pull request requires review and testing. labels Apr 8, 2021
@Mark-H Mark-H requested a review from opengeek as a code owner April 8, 2021 14:28
@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Apr 8, 2021
@Ibochkarev Ibochkarev added this to the v2.8.2 milestone Apr 8, 2021
@opengeek opengeek merged commit 9e11d92 into modxcms:2.x Apr 15, 2021
@opengeek opengeek removed the pr/review-needed Pull request requires review and testing. label Apr 15, 2021
@Mark-H Mark-H deleted the security-3801-noms branch April 16, 2021 12:42
@Mark-H
Copy link
Collaborator Author

Mark-H commented May 13, 2021

Description of how this handles paths (from the private forum):

Note up front: the variable naming "sourcePath" is unrelated to media sources.

It tries several things to be as compatible as possible, in order:

  • L61: if absolute is allowed and a path exists at the absolute path, use that
  • L75: if a file is passed that starts with the absolute sourcePath (so for example the path is {assets_path} and /home/user/public_html/assets/foo.pdf is provided), then remove the absolute path temporarily as that's added back on line 91
  • L84-89 (this is the part you're looking at if I understand correctly):
    • First, grab the root-relative relative source path. That is: if the path is set to {assets_path}, see if that is located within the MODX_BASE_PATH, and return only the relative portion of that: assets/.
    • If we have a such a relative source path (i.e. the setting points to a directory with in the base path), and the provided filename starts with that source path (i.e. assets/foo.pdf), then remove the relative portion from the filename as the absolute source path will be added at line 91.

So this supports a couple of variations for maximum BC while restricting to the provided directory:

  • full absolute paths (/home/user/protected/foo/bar.pdf), only if enabled
  • full absolute paths within the allowed directory (/home/user/public_html/assets/foo/bar.pdf)
  • relative paths from the root of the site, as that's what the media browser returns (assets/foo/bar.pdf)
  • relative paths from the provided directory (foo/bar.pdf)

Also worth noting, the path setting has an absolute path, not a relative one, so that can't be matched one-on-one and needs this sort of string comparison for different ways a path may be provided.

Mark-H added a commit to Mark-H/revolution that referenced this pull request Nov 3, 2021
opengeek pushed a commit that referenced this pull request Nov 5, 2021
* Restrict static resources to predefined path  (port of #15656 for 3.x, SEC-3801) 

* Use ::class syntax
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-security cla-signed CLA confirmed for contributors to this PR. urgent The issue requires attention and has higher priority over others.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants