Allow the use of symlinks within the files directory.

Moving to "Needs Review" for the test bot, and bumping to critical b/c this breaks symlink usage from D6 & prior.

I think using symlinks in your files directory is edge-case and qualifies more as 'major'.

Priority levels of issues

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]

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.

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.

@moshe thank for the pointer. @carlos8f, thanks for the clarification, makes sense.

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.

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.

I change issue version to 7.x-dev, because this bug exists in 7.0 release and in current development brunch too.

--- 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.

--- 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.

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_SEPARATOR constant.

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.

@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.

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.

subscribing

Improved comments.

Used the DIRECTORY_SEPARATOR constant to replace the confusing '(/|\\\\)' pattern.

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)

note, however, that mixing of / and \ in the same path is not allowed

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 the strpos() portion of the test.

Reverted back to the pattern for the preg_match() portion. Pity it looks so screwball with the quadrupled backslash characters.

Removed leftover cruft from local testing and added an explicit test for symlink support.

Tagging issues not yet using summary template.

#26: stream_wrappers-allow_symlinks-test+fix-1008402-26.patch queued for re-testing.

Subscribe

Adding 'needs security review' tag.

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.

Updated summary.

#26: stream_wrappers-allow_symlinks-test+fix-1008402-26.patch queued for re-testing.

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.

#34 Re-roll for D8 /core change

Thanks!

subscribe

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. :)

D7 version of the patch.

@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.

#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.

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.

@KingSalibah read http://drupal.org/node/707484 to learn how to apply a patch.

#43: Yeah, the a/ and b/ are there for git patches. You can either use patch -p1 to apply them, or git 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?

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.

Thanks @KingSalibah!

Updated to use Issue Summary Template.

So a symlink pointing to /etc/passwd is good..?

So a symlink pointing to /etc/passwd is good..?

Well, it is tagged "needs security review"...

So a symlink pointing to /etc/passwd is good..?

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?

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.

@#48-#50

So a symlink pointing to /etc/passwd is good..?

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-data user 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

Tilde(~) is interpreted ... by ... the shell ... the tilde part of the patch is overhead.

Can we guarantee that the shell will never get involved? What about Hostmonster, where PHP is invoked via CGI?

Can we guarantee that the shell will never get involved? What about Hostmonster, where PHP is invoked via CGI?

Why would we have to guarantee that?
If the shell is involved, in the first place, we cannot guarantee anything.

#35 re-rolled with the tilde(~) portions removed and minor comment improvements. Still needs security review.

Corrected and re-rolled. One of the file tests should be skipped if clean urls are disabled.

Updated issue summary.

Removed mention of tilde (~) as per ufku because the tilde is interpreted by the shell, not by PHP. Drupal is not responsible for protecting against shell-based exploits.

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.

#56: stream_wrappers-allow_symlinks-1008402-56.patch queued for re-testing.

Re-rolled.

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.

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.

In reviewing your patch, I also found a conflict with #1183208: 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.

Here's the patch, updated to accommodate the above issues.

Minor correction to a grammar mistake that I originally introduced in #60.

Updated links to current patch.

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.'

can't rtbc your own patch.

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

Unassigning; needs review by security team.

this is #65 for D7

Patch in #1 no longer applies to D-7.14. The attached patch is a stopgap measure to have symlink functionality available in current Drupal releases while a proper backport is worked out.

Mukhsim.

#65 still needs review by security team.

I see it has been tagged and awaiting security team review for 2+ months.
Would be great if this makes it in in time to be part of 7.15.

The title and OP imply that this is a feature.
If it is truly a bug, can the issue summary be updated to explain it?

Untagging. The issue summary is accurate. This is indeed a feature request, despite the fact that the requested feature existed in previous Drupal versions and was removed for security reasons.

Still needs review by security team before moving forward. So noted in summary, tags, and in several comments. And just for the sight-impaired:

NEEDS REVIEW BY SECURITY TEAM

OK, so this needs review by the security team, but that's been the case for coming up to two years. Is there anything someone who is not on the security team can do to help? Looking at the security team pages they seem more focused on fixing known vulnerabilities rather than reviewing new code.

I have tested #70 in D7.22 and it somewhat works. But for some reason is_link("public://link") returns false when it should not.

If you have a structure as such:
link -> dir
dir/

A streamwrapper should remove the symlink if ->unlink('link') is called, it should not attempt to remove the directory.
And if ->rmdir('link') is called it should also fail, because it is not a directory.

I see that always dereferencing symlinks in the streamwrapper is the fastest way to make drupal support symlinks.
However, I would prefer adhering to the streamwrapper interface and fix the other places in core that would fail because of this.

In #65 you said that you only corrected a minor typo.
But I see that you also deleted a change in file.test.
Was that intentional?

let's see

Assigning reroll to self for Core Mentoring ...

I've been thinking that maybe it isn't so bad that symlinks are made transparent by always dereferencing.

However, this should be fixed:
link -> dir
is_dir("public://link/..") #returns FALSE.

In order to properly test this functionality all drupals tests should be run with the files directory symlinked.
That would probably catch a lot of edge cases.

Rerolled patch from #64, but it still needs a quick typo fix from #65. See also #81.

+++ b/core/modules/system/lib/Drupal/system/Tests/File/DirectoryTest.phpundefined
@@ -162,3 +162,40 @@ function testFileDirectoryTemp() {
     $this->assertEqual($config->get('path.temporary'), $tmp_directory);
   }
 }
+
+  /**
+   * Tests that symlinks are supported within the files directory.
+   */

Note that there are 2 ending curly braces. The last one doesn't have any space indentation so it must mean the end of the test class. And then immediately a function is defined outside of that, testFileDirectorySymLinks(). The error is that this function is a method within the PHP class - the second ending curly brace here should be removed.

+++ b/core/modules/system/lib/Drupal/system/Tests/File/DirectoryTest.phpundefined
@@ -162,3 +162,40 @@ function testFileDirectoryTemp() {
+    // Clean up the mess.
+    file_unmanaged_delete($destination);
+    drupal_unlink($symlink);
+    drupal_rmdir($temp_dir);
+  }
+}

At the end of testFileDirectorySymLinks() there are two ending curly braces.

This is correct despite the syntax error suggesting its this line.

Corrected PHP syntax error as above and corrected the typo that was originally corrected in #65. This should be ready for review.

For what purpose do we allow .. in the paths.
Could it be disallowed entirely?

If .. is being used by the system, then it would fail if you make some path into a symlink.

Rerolled the D7 version from #70.

Updated link to patch under review.

90: stream_wrappers-allow_symlinks-1008402-90.patch queued for re-testing.

Rerolled patch from #92 applies smoothly to 7.29.

ciss, would you mind posting your rerolled patch? :)

@geerlingguy I didn't reroll any patches. I simply used the patch from comment #92 that had been rerolled by RunePhilosof.
If "patch <" shouldn't work for you (I didn't test it), try "git apply" which worked for me on a git checkout of the 7.29 revision.

here's a re-roll of #90, that corrects the test's path:
from: /core/modules/system/lib/Drupal/system/Tests/File/DirectoryTest.php
to: /core/modules/system/src/Tests/File/DirectoryTest.php

here's an updated version of #98, but re-rolled for HEAD, and removing the variable_set/gets all together in the test. this also changes the test file to use a static file, since as it is, existing test didn't have access to WebTestBase::drupalGetTestFiles.

Noone answered #48 yet in the last three years.

@chx #48 is not a question. The reply in #50 and #52, who and how it could be possible...
and actually how it's related to patch that simply allows to store files not only within "files" folder?

#92 worked for me on D7. We keep our files on an external storage so this is very helpful!

#92 works for D7.

How about to commit it or make separated issue for D7 to speedup fixing for D7?

Hi! I'm new to Drupal. I found about this issue during Allen Shaw's (TwoMice) presentation at MidCamp/MadCamp yesterday. I decided to have a look at it during the sprint today. I'm still coming up to speed on the community... I know I need to make the case that this is a bug and not merely a feature request. Anyway, I wanted to put this patch out there before I leave the sprint today.

Quick comments. The proposed patches so far don't fully address the issue. One issue is that the pattern '/..' does not automatically mean there is a parent directory reference. For example, you can have a file named '/..strange.jpeg'...

So I took a different approach. The real issue is not whether the realpath resolves to something outside of the site, but simply whether the $uri has cumulative parent directory references that would direct access outside of it. So I split the $uri on directory separators, then prune out current and parent directory references, stopping if it tries to climb out of the directory. It's a bit complicated, but I don't believe there is a simple solution to this issue.

Anyway, I've got reading to do on procedure around here, but hopefully this is useful.

added a link to instructions to do a beta evaluation in the summary
https://www.drupal.org/contributor-tasks/update-allowed-beta

Any chance getting #92 committed to D7 core or is this always going to expose security issues?

This may be considered a 'feature' but if you have a requirement to implement file uploads outside of document root it quickly becomes a necessary function and a major pain point.

I found myself in that situation. Reticent to make core changes I have created my own streamwrapper which neatly sidesteps the issue.

I have created a sandbox module for the symlink_paths streamwrapper module based on comment #100 above

Please pass comment as I am new to submitting modules and may miss some important steps.

I have added the code to the sandbox project linked above and put in a review request
https://www.drupal.org/node/2644312

If you have interest in this module and are a reviewer (or want to be one) I'd appreciate you taking a look at it

All credit goes to bdone for his patch which is at the core of the module

Someone can make a patch for Drupal8?

Patch of #98 updated for Drupal 8.1 but without the test as it has moved in another file in current HEAD.

#92 still applies cleanly to Drupal 7.52

Looking forward to this so I can finally test and use Drupal 8 easily with my Omega8.cc (Aegir) server.

+++ b/core/lib/Drupal/Core/StreamWrapper/LocalStream.php
@@ -137,10 +137,30 @@ protected function getLocalPath($uri = NULL) {
+        // If the target path contains directory-traversal parts such as '/..' and

Line exceeding 80 characters
There is one line which contains more than 80 characters. Applying the patch please review.

Thanks for the patch, It works for me

There is a fundamental problem with the patch: What if realpath returns FALSE? In the original function, that value is returned by the function.
Here, however, a value is always returned because $target_dir = realpath(dirname($repository . $target)) . DIRECTORY_SEPARATOR; always has a value.

There should be a check to see if realpath returns FALSE before proceeding, and a way for the function to return a FALSE value.

Hello, is there a change for this to be reviewed by the security team so it can be included soon (maybe in the next 8.3.x update)?
My use case is to get it work easily using BOA.

Thanks so much for the community effort.

#92 still applies cleanly to Drupal 7.56

#92 still applies cleanly against D7. I just successfully patched Drupal-7.58.

Applied #117 to Drupal 8.5.6 and it seems to be fixed for me.

This is not exactly the same but I would like to piggyback on this issue. I symlinked my theme folder that is in another repo/folder...My vagrant sees the folder, but Drupal doesn't see my theme.

Is there a way to get this working?

Thank you

Here's a reroll of #92 to be compatible with Drupal 7.62.

Patch from #117 works for me without issues too, and help solve issue #3059139: Move all cache files to separate cache directory in Drupal files storage

Hi All,
how can i help make #117 available for D8.7.x ?

There are many pages on Drupal.org describing how to contribute your own code (you can search for "drupal how to make a patch"). Here's one that looks comprehensive to me: Novice code contribution guide. Since you just want to update an existing patch for a new version of Drupal, you'll probably want to download the code for the latest Drupal version, and then manually add the code in the patch, then generate a new patch.

Here my patch for the 8.7.x

Re-displaying the patch from #129 for 7.x.

Patch from #134 solves this for me!

Patch #134 looks good to me and works fine, so moving it to RTBC.

This cannot be RTBC as it is still tagged as needing tests and an issue summary update.

Applied #134 to Drupal 8.8.1 works like a charm.Thanks for this great job!

@apermuy Not sure why you hid the patch in #129. It still applies perfectly against 7.69. And this issue will need a backport to D7.

Version bump.

Would love for someone with 9.x experience to step in and add the tests.

This works in Drupal 9.1

The patch still passes for 9.2.2.

Thank you.

Fixed CS issues of patch #134.

Still need works for Test.

Opened an MR that rerolls #148, which no longer applies to the latest 10.1 dev. Still needs tests.

Removing the "needs issue summary update tag" because it appears that the IS was updated but the tag was not removed.

Could someone possibly roll a patch for current dev that would apply to 10.1 ?

@jimconte Did you try downloading and using the patch from the MR?

Thank you @solideogloria ! This applied cleanly to 10.1.6 with cweagans/composer-patches

Make sure to download it, rather than using the URL. The URL stays the same for the MR, so if someone commits to the MR, your patch will change if you're using the URL.

You can put it in a project folder and use ./patches/some-file.patch

I close the old MR and opened a new one for 11.x (11.3 no longer works with the old MR).

Also, in reviewing the code, I'm pretty sure there is a mistake with the handling of $realpath variable, which is set but never used later: $realpath = realpath(dirname($path)) . '/' . basename($path);

Coding standards unused variables doesn't set this because of the null check.

I fixed some regressions and added tests.

Putting notes from a few weeks ago in Slack:

Symlinks shouldn't scare anyone.

The following is based solely on a visual review of the code:

Blocking paths based on containing relative access:

public://symlink/foo.txt = allowed
public://symlink/bar/../foo.txt = prohibited

Filesystems do not work like this, normal code uses relative paths all the time. Just because a path has ../ isn't grounds to block it, especially when its accessing the same path that would otherwise be allowed.

Fundamentally the root fault is in all this code is in regards to thinking in realpath's, realpath's never exist (IMO they shouldn't even be on the streamWrapper API, however I digress). public://symlink/../foo.txt isn't /usr/local/symlink/../foo.txt it is public://foo.txt. Before a path ever makes it to the actual filesystem where a symlink can be evaluated ./ and ../ should be long long gone. This isn't new, its been done in other streamWrappers in the past (S3FS is where I've personally implemented it, however there were other wrappers before me).

We shouldn't have a fear of escaping public:// because it is essentially /, and you can't go any lower than /, however that is what the current protection is, it is an indication in a lack of trust in the streamWrapper's to resolve paths (largely due to such code not existing).

Opinion:
This should be separated into two issues:
1) Normalize all paths prior to passing it to the real filesystem (especially in realpath())
2) Repurpose this issue to remove the streamWrapper root path protection completely.

The first essentially gives you a stable platform for path resolution with or without symlinks. After that supporting symlinks outside the path root is just a matter of no longer restricting to the streamWrapper base.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.