| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | file system |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | needs backport to D7, needs security review |
Issue Summary
Problem/Motivation
Drupal may be configured to allow untrusted users to upload files with arbitrary filenames, including filenames with path components. For instance, if Drupal is installed at /var/www/ and the uploaded file repository is at /var/www/sites/example.com/files/, then an uploaded filename of '../../../index.php' would point to the /var/www/index.php file.
To prevent untrusted users from overwriting such critical files, the current logic for the DrupalLocalStreamWrapper::getLocalPath() function works like this:
-
Set
$directoryto therealpath()of the files repository. -
Set
$pathto the target file location. -
Set
$realpathtorealpath($path). -
Return
FALSEif$realpathdoes not begin with$directory.
This causes a problem when the site builder wants to store some uploaded files outside of the Drupal web root. Many site owners have resorted to hard-linked directories, recursive bind mounts, or simply hacking Drupal core to remove the restriction.
Proposed resolution
The logic within the DrupalLocalStreamWrapper::getLocalPath() function should be revised as follows:
-
Set
$directoryto therealpath()of the files repository. -
Set
$pathto the target file location. -
Set
$realpathtorealpath($path). -
Return
FALSEif both of the following are true:$pathcontains'/..'or'\..'.$realpathdoes not begin with$directory
.
Remaining tasks
The patch proposed in #65 needs to be reviewed by the security team.
User interface changes
None.
API changes
The DrupalLocalStreamWrapper::getLocalPath() function would follow symlinks, even if they point outside the files repository.
Original report by tekante
Related issue: #155781: "realpath" check breaks symbolic links in file directory
The method by which directory ascension attacks are prevented in stream_wrappers.inc prevents file system layouts which rely on symlinks within the files directory. Specifically, the use of realpath within the getLocalPath results in the check of whether the destination directory exists within the files directory to fail.
I will attach a patch which allows for the destination directory to exist outside of the configured files directory when it appears this is due to the presence of a symlink and not due to directory ascension characters.
Comments
#1
Patch to allow use of symlinks within files directory attached
#2
Moving to "Needs Review" for the test bot, and bumping to critical b/c this breaks symlink usage from D6 & prior.
#3
I think using symlinks in your files directory is edge-case and qualifies more as 'major'.
Priority levels of issues
#4
I was under the impression that if it was a regression of something that worked in the previous release (D6), then it was considered critical. I do not see that scenario mentioned anywhere in that Priority document. Who would we ask to provide some clarification on that?
[I do not consider using symlinks for the files directory edge-case, we host well over 500 drupal sites and they all use symlinks for the files directory as do many other companies that I know]
#5
To put it in perspective, PostgreSQL issues are not considered critical... we only consider an issue critical if it causes data loss, security holes, or stops multiple pieces of functionality from working for most people. Symlinks aren't even a feature of Drupal per se, so technically it is not really a regression. I still think this is a valuable issue to address, but not critical by the community's standards.
#6
Not sure if it is related, but here is an older, similar issue #155781: "realpath" check breaks symbolic links in file directory which is resolved for d7 core but not for imagecache project.
#7
@moshe thank for the pointer. @carlos8f, thanks for the clarification, makes sense.
#8
Just marked #1055170: Drupal public and private file schemes are not supported junctions (symlinks) of Windows NTFS! as a duplicate. Any change here will need to be cognizant of "junctions" --- what symlinks are called in the land of NTFS.
#9
The Cause of bug on NTFS junction is described in http://drupal.org/node/1055170#comment-4074362 (closed duplicate #1055170: Drupal public and private file schemes are not supported junctions (symlinks) of Windows NTFS! ).
I propose to remove stpos from DrupalLocalStreamWrapper::getLocalPath() but, as bfroehle specify strpos is a security check. In this case, we need to implement own analogue of realpath() for call in getLocalPath(). But this analogue should not transform symlink (junction) to real path. In this case, method getLocalPath() will always work with path that is relative to scheme base folder.
#10
I change issue version to 7.x-dev, because this bug exists in 7.0 release and in current development brunch too.
#11
--- 367,382 ----+ $symlink = ($realpath != $dirpath && !preg_match('/\.\./', $dirpath) && !preg_match('/~/', $dirpath));
The logic here is hard to follow. Since we would have to be very careful here not to introduce a security vulnerability, can you add some descriptive comments about what exactly is going on?
Powered by Dreditor.
#12
Attached patch with comments explaining use of the symlink flag and what the regular expressions are checking for
#13
--- 367,389 ----+ // as a security mechanism $directory values which don't appear in the $realpath
+ // value result in a FALSE return to prevent directory ascension attacks
+ // this breaks systems which want to symlink parts of the filesystem to somewhere
+ // outside of $realpath. This sets the $symlink flag if it appears this has happened
+ // only if no directory ascenension attack strings are observed. Flag is later used to ¶
+ // allow the $directory values which don't appear in $realpath
...
+ // see comment in true case for details about symlink flag test
1) We need a patch created using git against latest code in 8.x.
2) The patch needs to be in unified diff format.
3) Please read http://drupal.org/node/1354 for Drupal's coding standards for comments.
Powered by Dreditor.
#14
Improved logic and comments. Could be better. Is there a function that returns the directory path separator character? (usually '\' for Windows and '/' for Unix).
EDIT: Found it. I wanted the
DIRECTORY_SEPARATORconstant.#15
The last submitted patch, stream_wrappers-allow_symlinks-1008402-14.patch, failed testing.
#16
#17
The last submitted patch, stream_wrappers-allow_symlinks-1008402-16.patch, failed testing.
#18
1) It doesn't look like #16 has been tested locally in any way. The syntax error is obvious compared to #14.
2) The code could explain the reasoning for what is being done a bit better. The comments from the old patch might be a starting point, though also not sufficiently clear.
3) The if condition should not wrap. Since you keep on doing this, I'll make sure to formalize the rules in Drupal's coding standards.
#19
@3) -- Please do. I'll keep breaking unwritten rules until they get written down.
@1-2) -- Better?EDIT: Bah; now I see; the backslashes need to be quadrupled. Rebuilding and locally testing now.#20
The last submitted patch, stream_wrappers-allow_symlinks-1008402-19.patch, failed testing.
#21
Added additional comments which hopefully explains what is going on. If it is still unclear, please let me know and I'll try to be more verbose.
#22
subscribing
#23
Improved comments.
Used the
DIRECTORY_SEPARATORconstant to replace the confusing'(/|\\\\)'pattern.#24
Bob, I'm not sure about using the constant... The issue is that on windows both / and \ are accepted directory separators, even though \ is preferred. (note, however, that mixing of / and \ in the same path is not allowed)
#25
Actually, the Windows command line does allow mixing them in the same path; I've done it. However,
realpath()emits backslashes on windows and forward-slashes on unix, so using the constant is justified for thestrpos()portion of the test.Reverted back to the pattern for the
preg_match()portion. Pity it looks so screwball with the quadrupled backslash characters.#26
Removed leftover cruft from local testing and added an explicit test for symlink support.
#27
Tagging issues not yet using summary template.
#28
#26: stream_wrappers-allow_symlinks-test+fix-1008402-26.patch queued for re-testing.
#29
Subscribe
#30
Adding 'needs security review' tag.
#31
Subscribe. This is an important issue for me. I don't see why the check 'strpos($realpath, $directory) !== 0' is needed at all in DrupalLocalStreamWrapper::getLocalPath. Can someone explain why this check is needed?
Right now, I'm using hardlinks instead of symbolic links.
#32
Updated summary.
#33
#26: stream_wrappers-allow_symlinks-test+fix-1008402-26.patch queued for re-testing.
#34
Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
#35
#34 Re-roll for D8 /core change
#36
#37
Thanks!
#38
subscribe
#39
Stop subscribing; start following
Also, the issue is set to Needs Review, which means it could use testing to verify whether the patch resolves the issue. :)
#40
D7 version of the patch.
#41
@xjm Would you mind taking a look at http://drupal.org/node/1347222? I believe I am having a similar, if not the same, issue. I would like to know if this patch (for D7) would work for my issue. I was told D7 does not support symlinks, however, it did in essence work, and if it could work without showing me an error, it would be a great solution for my site. Thanks.
#42
#41: The best thing to do would be to apply the patch in #40 and actually test whether it resolves the issue. Then, report your findings in this issue. That's what's needed at this time in order for this to be fixed.
#43
Okay, I am trying my first patch!
First error, can't find the filename. Why does the path have "a/" and "b/"?
I took those out and running:
patch -p0 --dry-run <stream_wrappers-allow_symlinks-1008402-40-d7.patch, I got the following result:patching file includes/stream_wrappers.inc
Hunk #1 succeeded at 365 (offset -8 lines).
patching file modules/simpletest/tests/file.test
Thus, seems like it would work without a/ and b/. I dunno what "at 365 (offset -8 lines)" means though.
#44
@KingSalibah read http://drupal.org/node/707484 to learn how to apply a patch.
#45
#43: Yeah, the a/ and b/ are there for git patches. You can either use
patch -p1to apply them, orgit apply. The message about offset is okay, so it looks like it was correctly applied. Glad to see you got that working. :) Now, does it fix the symlinks issue for your site?#46
Patch worked successfully on my Drupal 7.8 site.
[I will spare you my newbie travails, but make no mistake, a designer/developer of a website is not the same as a server administrator or linux user. The broad knowledge required to be a real Drupal admin is quite extensive. There appears to be no light-hearted versions of Drupal admin. You are either in or you're out, unless of course all you want is a cheesy blog.]
I installed git and patched it that way. Results were clean and seems to work fine so far.
#47
Thanks @KingSalibah!
#48
So a symlink pointing to /etc/passwd is good..?
#49
Well, it is tagged "needs security review"...
#50
Good question.
First, one could ask, how did the symlink get there in the first place?
OK, probably because www-data has write access on these directories.
Something hacked the site, and lets it create symlinks to evil locations.
Or, it could even be another site that was hacked, one that has the same www-data user.
What about a whitelist in settings.php, for known symlink destinations?
#51
Tilde(~) is interpreted as is by PHP. It's the shell that interprets it as home dir. It also works only when it is at the beginning like ~/. So the tilde part of the patch is overhead.
#52
@#48-#50
I don't think it is possible to create a symlink using the Drupal API. So if there is a security problem, it isn't with Drupal. Finally, if the
www-datauser has write-access to/etc/password, there is definitely a security problem outside of Drupal. This is especially true of a host with multiple sites running as the same www-data user.@#51
Can we guarantee that the shell will never get involved? What about Hostmonster, where PHP is invoked via CGI?
#53
Why would we have to guarantee that?
If the shell is involved, in the first place, we cannot guarantee anything.
#54
#35 re-rolled with the tilde(~) portions removed and minor comment improvements. Still needs security review.
#55
The last submitted patch, stream_wrappers-allow_symlinks-1008402-54.patch, failed testing.
#56
Corrected and re-rolled. One of the file tests should be skipped if clean urls are disabled.
#57
The patch in #40 has solved a problem on a server where the images were being loaded via a mount point from a SAN (75gb of data that was being references via custom entities), so a huge HighFive from me.
#58
#56: stream_wrappers-allow_symlinks-1008402-56.patch queued for re-testing.
#59
The last submitted patch, stream_wrappers-allow_symlinks-1008402-56.patch, failed testing.
#60
Re-rolled.
#61
Thanks for all your really hard work. You have saved me and others a great deal of time putting all of this together so nicely.
I'm not sure what branch you generated the patch from, but as alluded to in #43, the core/modules/simpletest/tests directory and the core/modules/simpletest/tests/file.test file don't appear in the D8.x install.
I wonder if you wouldn't mind clearing up that point, and I would love to run some tests on it.
#62
Okay, I see what happened. All the testing files were moved out of the SimpleTest module, so we have to reroll the patch to reflect that update.
See #1352000: Forward-port upgrade test clean-ups from 7.x to 8.x.
#63
In reviewing your patch, I also found a conflict with #1183208: Change notification for: Remove variable_get('clean_url') and switch to index.php/path pattern for dirty URL support. Just a minor point where you are inserting
if (variable_get('clean_url', 0)). We need to replace that so as to not undo their work.#64
Here's the patch, updated to accommodate the above issues.
#65
Minor correction to a grammar mistake that I originally introduced in #60.
#66
Great so I have pulled the latest D8, and this patch meets my tests. All the security checks I know are in line. Let's call it 'reviewed and tested.'
#67
can't rtbc your own patch.
#68
Patch looks good but we need more reviews to check that contrib modules a-la cdn still works.
PS: and it still needs security review
#69
Unassigning; needs review by security team.