A common misconception is that the .htaccess file created by Drupal in a files directory protects against downloading a file by directly accessing the URL. This means that users have a functional site with private files, but are actually exposing their data.

The attached patch adds in a test to system_requirements() and throws an error if the private directory is can be accessed directly. I'm marking as Major as this could lead to sites exposing data accidentally.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Status: Active » Needs work

Given that there are no tests in the patch (not sure how possible it is to actually test server configuration issues like this via simpletest), I tested this manually. I'm using apache, so the .htaccess file injected by Drupal by default works fine to protect this directory, even if it's inside your webroot. So long as Drupal can write to this directory at all (which it tests for you when you're configuring it at admin/config/media/file-system (you know, since the only thing you ever upload is "media" *sigh*)), it injects the right .htaccess and this check isn't needed. The only way I could actually trigger the situation where Drupal was happy with my private directory config but I could still access the files is if I went in and manually commented out the contents of the .htaccess it gave me.

So, really, the target audience for this check are people not using apache, since Drupal's default handling of the private files dir is pretty solid if you're on apache. Doesn't mean it's not worth adding this (not at all!), but I just wanted to clarify for other people reviewing/testing.

Once I did the hack to trigger the error condition, this status warning successfully appeared. My only concerns with this patch are:

A) It'd probably be helpful if the error message gave you a link to admin/config/media/file-system (like the warning of your files directory not being writable at all gives you under the "File system" warning).

B) It's a little weird that the 'value' of this message is blank when there's an error:

$requirements['file_system_private_secure']['value'] = '';

Seems like it should probably have a pithy summary of the problem, like "Not secure"

C) Totally cosmetic and maybe not worth fixing, but it seems like it'd be nicer for admins reading the status report if all the file system related messages are grouped together. There's no specific mechanism for grouping on this page, so I think the only way to do that would be via the title of the check. Something like "File system private directory" or something.

I'll quickly re-roll for these so this can return to "needs review".

dww’s picture

Status: Needs work » Needs review
FileSize
26.27 KB
2.12 KB
dww’s picture

Issue tags: +Security improvements

Tagging this for "Security improvements" since this is a hardening measure to prevent misconfiguration that leads to information disclosure...

dww’s picture

Since killes asked in IRC, I should clarify. This: "the .htaccess file injected by Drupal by default works fine to protect this directory, even if it's inside your webroot" is care of #517814: File API Stream Wrapper Conversion which is new behavior in D7. If the idea is to backport this check to D6 (not sure that's going to fly with the string freeze), then the need for the test is even greater, since Drupal won't do much to help you in this case.

Bojhan’s picture

We are kind of repeating twice, that private files are publicly accessible, lets assume here if it gives a warning its already bad. And that "access to private files" is enough to trigger that awareness. There are two options to solving it, they should stand out when scanning :

"The private files in %directory are publicly accessible. To ensure that the directory is not directly accessible either move it outside of your website root (and change the path at ) or configure your web server to block acces to the directory."

- Probally can make the two options stand out more.

dww’s picture

Rerolled for the new warning text per Bojhan (thanks for the review!).

mikey_p’s picture

My only concern with this test is what happens if the private files directory isn't writable, or the file isn't saved properly. This would result in a 404 or 403, and without differentiating between the two it's not possible to see if the directory is protected or not. Maybe we could check the specific error code, and count a 404 as unprotected?

dww’s picture

Status: Needs review » Needs work

@mikey_p: Drupal won't let you save the file system settings page with a directory that's not writable. But, it's possible someone (or something) could chmod the directory out from under you. In that case, the status report correctly warns you that your private files dir isn't writable, but this test "passes" (although it generates a warning about not being able to copy the file).

So, we should probably change this test so that it only runs if the private files dir is writable.

dww’s picture

Assigned: Unassigned » dww
Status: Needs work » Needs review
FileSize
3.71 KB

Here, we only do this test at all if the other checks on the private directory (exists, writable) pass. This way, if there's a problem writing to the directory, we don't attempt a bogus check. No change in the resulting error message, so no new screenie. Also fixed a (now bogus) code comment about how the private files dir needs to be outside the webroot to be secure.

Also, since I seem to be driving this home, I might as well assign myself. ;) But please also credit deviantintegral in the commit...

dww’s picture

p.s. I also tested if I put a non-writable DRUPAL_PRIVATE_FILES_TEST.txt file in the directory, and file_unmanaged_save_data() helpfully created a test file called DRUPAL_PRIVATE_FILES_TEST_0.txt instead. So, now that we only try this check if we already verified the private files directory is writable, I can't see how it's possible to fail to write a test file...

mikey_p’s picture

Status: Needs review » Reviewed & tested by the community

This makes much more sense that trying to write the file and check the result, which is what I would have tried. This way we avoid an additional error message if the directory is not writable.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 929166-9.private-files-check.patch, failed testing.

dww’s picture

Status: Needs work » Needs review
FileSize
3.78 KB

To clarify, includes/file.inc is full of drupal_set_message() on error. So, if the private files dir is unwritable and we tried calling file_unmanaged_save_data() to check the return value, there's no exception to catch, we just automatically get a nice ugly:

The specified file temporary://filetbUhZs could not be copied, because the destination directory is not properly configured. This may be caused by a problem with file or directory permissions. More information is available in the system log.

which we have no way to hide. That's lame (something to clean up in D8). To avoid this, #9 is really the best approach. We need to remember if the private dir is writable (which we're already testing), and only try this new check if so.

That said, it doesn't hurt to check the return value from file_unmanaged_save_data() and only include this check if it worked. See attached patch.

So, what's up test bot? I can't reproduce that test failure locally at all... :/

deviantintegral’s picture

Thanks for the reviews.

At least with MAMP on OS X, the default Apache configuration allows files to be accessed. Either MAMP is blocking the .htaccess rule, or the .htaccess isn't being generated properly.

IMO this should be backported to D6 and even D5 if possible. Though it's not really a security vulnerability, I wonder if it's reasonable to send out a security PSA about it once this hits D6.

Another thought; this patch doesn't really know for sure that the private files aren't accessible, just that they likely aren't. Any thoughts on changing the wording so that it's indicated that "as far as core can tell" the directory is secure?

Here's a mildly updated patch that doesn't add a 'private' key to $directories if it's not set. It seems like a mild DXWTF to have keys that don't point to valid paths.

dww’s picture

@deviantintegral:

A) I agree we should backport. However, I doubt that's going to fly given that these new strings would have to be translated. I guess we'll have to see what Gabor thinks once this is in D7.

B) I don't think #14 is necessary. 'private' => FALSE is a perfectly fine thing to have in an array of $directories, and we immediately skip such entries thanks to this:

     if (!$directory) {
       continue;
     }

Therefore, I don't think the additional conditional in #14 is worth the trouble. But, I guess we can leave it up to the core committers to decide which approach they prefer. However, #14 includes the new check twice, thanks to git helpfully merging both of them for you. ;) So, #14 needs work. But, I think we should just let #13 stand as-is.

deviantintegral’s picture

Status: Needs review » Reviewed & tested by the community

As what's mentioned in B is code that's all ready in core, I'm fine with #13. If a committer thinks it's an issue we can do a followup patch.

dww’s picture

Assigned: dww » Unassigned
Status: Reviewed & tested by the community » Needs review

Hate to do this, but chx reminded me in IRC of the fun we had over at #245990: HTTP request testing unreliable and may be undesirable :( There are a lot of possible configurations where a test like this could fail, resulting in bogus status messages, etc. I don't know if we're going to be able to catch them all and reliably warn people that need to know and leave people alone who are actually fine.

deviantintegral’s picture

I think given that web servers can be configured in all kinds of broken ways, the best solution is to be less definitive in the status message. For example, it's entirely possible that some other vhost has been configured to allow access to the directory, and there's no way we could ever test that.

The attached patch changes the error status to REQUIREMENT_WARNING and indicates in both the success and error conditions that Drupal can't know for sure if files are secure or not.

dww’s picture

Hrm, I'm not sure Bojhan is going to be happy about those warnings. ;)

- If it's green and "happy", I doubt anyone is actually going to read carefully enough to check to see if the files really are protected.
- If it's a false warning due to server configuration that breaks the test, but the files really are secure, there's no way for the admin to hide the bogus warning.
- In http://drupal.org/files/issues/Status%20report%20(protected)%20|%20HEAD.png seems like the "It is important to verify that the directory is protected manually due to the differences in web server configuration." part should go in the description, not the value. And, it should probably be "It is important to manually verify..." since "manually" modifies "verify", not "protected". It might also be useful to explain how to manually verify this (or link to something that explains it).

So, this should probably get another UX look before this is RTBC.

Meanwhile, updated the patch a bit to avoid trying the test if the tmp dir isn't writable. See #930122: Regression: temp directory handling broken by confusion between file_directory_temp and file_temporary_path for more. :/ Also, since we already initialized $directories['private'], there's no need to do the variable_get() again and incur the cost of a function call (even if the value is cached).

dww’s picture

Here's some of the fix to the message in the status-ok case. Still not sure this is reasonable UX, so tagging appropriately.

Bojhan’s picture

Although dangerous - I would not necessarily recommend a message like this (if we don't know, we should probably not have error messages for it). I would seriously reconsider adding this, even if its a security improvement we can not give any clear answers to our users.

Onto the actual copy writing : it reads as if, there is something wrong. But its likely there isn't anything wrong.

I have tried rewording, but in case of "maybe" its really hard to not make a confusing line:

"%directory is private and not accessible from the web."
"However manually verify that the directory is protected, since certain web configurations ...."

The word "However" should trigger a scan through, more so then "it is important to".

deviantintegral’s picture

At this point, I don't see a reasonable way to word a status message so that it addresses all of the above concerns. What if instead of a status message (that may or may not be accurate) we improve the form text for configuring private files? Here is the text in the attached patch:

t('A local file system path where private files will be stored. This directory must exist and be writable by Drupal. This directory should not be accessible over the web. For example, place the directory outside of your Drupal installation directory, or block access to it through your web server configuration. For more information about securing private files, see the <a href="@handbook">online handbook</a>.', array('@handbook' => 'http://drupal.org/handbook/modules/file'))

deviantintegral’s picture

Issue tags: +String freeze

Tagging for RC.

deviantintegral’s picture

This patch removes the "For example" sentence so it's not in code and can be managed via the handbook.

Status: Needs review » Needs work

The last submitted patch, 929166-24.private-files-description.patch, failed testing.

deviantintegral’s picture

Status: Needs work » Needs review
FileSize
1009 bytes

Let's try this again.

sun’s picture

Title: No warning if private files are accessible over the web » Add warning if private files are accessible over the web
Version: 7.x-dev » 8.x-dev
Category: bug » task
Issue tags: +Needs backport to D7
yoroy’s picture

yoroy’s picture

So, original text:

A local file system path where private files will be stored. This directory must exist and be writable by Drupal. This directory should not be accessible over the web.

In the patch:

A local file system path where private files will be stored. This directory must exist and be writable by Drupal. This directory should not be accessible over the web. For more information about securing private files, see the online handbook.

That takes this from 167 to 243 characters, which is quite long. Trying a condensed version:

An existing local file system path that is writable by Drupal and not accessible over the web. See the online handbook for more information about securing private files.
marcingy’s picture

Status: Needs review » Reviewed & tested by the community

I think this sounds good but I'm not 100% certain I should be rtbcing as it likely needs a second review from someone with a better way with words, but anyway!

catch’s picture

Looks good here as well.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This breaks strings, but I think it's worth it given the severity if it doesn't work.

Committed and pushed to 8.x and 7.x. Thanks!

Tor Arne Thune’s picture

Status: Fixed » Needs work

But it's not called handbook anymore, but rather Drupal.org online documentation. And the link redirects to http://drupal.org/documentation/modules/file, so we should probably use that. Otherwise it looks good :)

webchick’s picture

Priority: Major » Minor

Oops! Good catch! But this is no longer major. :)

Tor Arne Thune’s picture

Status: Needs work » Needs review
FileSize
1.09 KB

Attached a patch with the suggested changes in #33. :)

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs usability review

Looks good to me.

Tor Arne Thune’s picture

webchick’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

Marking as a duplicate of #1483662: URLs for drupal.org module help pages need updates which seems to have a more holistic fix.

Tor Arne Thune’s picture

Priority: Minor » Major

Resetting original priority, just for the records :)

Tor Arne Thune’s picture

Status: Closed (duplicate) » Fixed

Actually, it should probably be marked as fixed, for the original issue... of this... issue.

Status: Fixed » Closed (fixed)
Issue tags: -Security improvements, -String freeze, -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.