./sites/default/files directory permission check is incorrect during install AND status report

I see this as a major issue. It's not a show stopper but thinking of the less tech savvy users of Drupal perhaps we should bump it up. We don't seem to be getting the attention of the developers here.

Testing files access and ownership within PHP will never work correctly. You can't emulate the filesystem logic for all OS, ACL variant and whatever NFS strangeties. It just failed on a very simple case for me 10min ago.

If you need to know if a file can be written to, you fopen($filename, "r+") and check the result. Then use the handle or close it.

Besides it's racy (and thus risky) to check file permission, then manipulate the file. Countless security issues arise from this misconception.

Fixing this is quite complicated since drupal_verify_install_file() check if files, dirs or symlinks can be read, written or executed. Callers should actually *try* to do what they're only checking (read, write, exec), and then handling exceptions as usual.

And this function heavily calls drupal_install_fix_file() which means that drupal_verify_... does modify files. It tries to fix things, and user-friendlines is part of Drupal experience, so let's be it. But it will pass a given test if drupal_install_fix_file() succeeds *without* re-running the test. On a NFS setup it's possible for a chmod() to succeed without having the intended effect on bitmask (because the NFS server logic decides and it sometimes looks funny to a client). This:

if (!is_writable($file) && !drupal_install_fix_file($file, $mask)) { return FALSE; }

Should be (ugly, yes):

if (is_writable($file)) { return TRUE; }
elsif (drupal_install_fix_file($file, $mask) && is_writable($file)) { return TRUE; }
else { return FALSE; }

I see 12 calls of drupal_verify_install_file() in Core. Worth patching ? It's maybe not a security risk, although this design is flaky.

pulled directly from http://us3.php.net/manual/en/function.fileperms.php

This should be "active", not "needs review", since there is no patch yet. It's probably not hard to write one, though :)

Let's keep this focused on the original bug report. Some of the last few comments seem like they're about something different.

The last submitted patch, directory-executable-944582-9.patch, failed testing.

OK, so the tests pass. I guess it also makes sense to create a helper function for the combined writable/executable check, rather than repeating it everywhere. The attached patch does that.

That patch applied well to my last RC. Still not sure if that works through the permissions problem on the site I've been trying to upgrade mind you.

2011.06.07
This solution relates strictly to the Drupal 7 error...

===
Error:
The specified file temporary://file****** 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.
===

The Solution for me was simply..
.
Go to 'Configuration' > 'Media' > 'File System'
[*root*]/admin/config/media/file-system
and change the 'Temporary directory' from...
/tmp
...to...
tmp

(Do Not use a "/" to begin the directory specification)

See also http://drupal.org/node/1106492#comment-4562780
for all of my known details of that error.

#16 My god,

All afternoon trying to figure it out and that's the flippin solution!

[smacks forehead]

Thanks for posting!

NNC

Solved my problem.
So when you specify a file or image field in a content type, the directory isn't necessarily available already. If you try to run devel generate to make a batch of content, the lack of this destination folder throws the same type of error as original post above.

If you manually create a single node and enter the information for these fields, then devel can add additional items without a problem. So there is some disconnect with the field folders. Perhaps they should be created on the content type configuration save. But that still may lead to issues if something tries to access it programmatically before that.

For me Solution #16 worked..Thanks.

I'm using Panels and this error cause all my page layout with Panel doesn't work.. error message from log:

my tmp folder work fine, but the "ctools/css/" just empty...

I had this problem too after moving my local development site to be live on a shared hosting environment - #16 fix worked partially - I was still getting errors on several other pages such as admin/appearance. Looking at the recent log entries, admin/reports/dblog, I found that this was due to Drupal wanting to recreate it's own cache directories. For example - the log entry showed the error:

I deleted the css directory at sites/default/files/css, then Drupal would happily recreate the css directory with the proper owner and permissions. This was also true for the directory sites/default/files/js. Once Drupal recreated those all of the errors were gone and all was right with the world.

This needs to be rerolled for both #22336: Move all core Drupal files under a /core folder to improve usability and upgrades and the git migration. Tagging as novice for the task of rerolling the patch.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

I think this related and should be included #484554-7: Stop relying on Apache for determining the current path

@nikkubhai (#29), you may try to manually create a subfolder inside /tmp, let's say /tmp/d7/subdomain.example.com (replacing last part with actual site folder name for recognition purpose, although any subfolder name would be equivalent). Set permissions to 777 for all three folders and change the setting in admin/config/media/file-system accordingly.

I noticed that writing directly to /tmp, even with permissions set to 777, won't work with some D7 installs. I don't know at this point if this is a problem with D7 or a hosting account feature.

Guys this is an easy fix. It is only a Unix files permission problem. Here is the fix:

1. Change directory to the root of your Drupal site.

cd /var/www

2. Set your permissions to 777 and try it out. You can back off permissions to lock it down later, but get it working for now. Ensure to do it RECURSIVE. the -R is what is killing people in this post because there are already files in the directory that need to be writable for Drupal to work properly.

chmod 777 -R sites/default/files

3. Now create a directory called tmp at the root of your site.
mkdir tmp

4. Set the permissions there too.
chmod 777 -R tmp

Enjoy.

John Galbraith
www.binarybb.info

Tagging for manual testing.

I went through the steps and was able to reproduce the issue.

I then tested the patch and upon clearing the cache received a warning message that there was something wrong with my Drupal installation.

After navigating the to startus report page, I had the appropriate error. (See screenshot)

So, the patch looks good :D

Let me know if I missed something or need to do something else.

Thanks all! And thanks xjm for your continued help and support.

Okay I was totally confused by #43 until I realized it actually refers to this function (not the one quoted above):

I read the doc for drupal_realpath(); unfortunately, it doesn't seem to indicate what one is supposed to use instead:

This function was originally written to ease the conversion of 6.x code to use 7.x stream wrappers. However, it assumes that every URI may be resolved to an absolute local filesystem path, and this assumption fails when stream wrappers are used to support remote file storage. Remote stream wrappers may implement the realpath method by always returning FALSE. The use of drupal_realpath() is discouraged, and is slowly being removed from core functions where possible.

Only use this function if you know that the stream wrapper in the URI uses the local file system, and you need to pass an absolute path to a function that is incompatible with stream URIs.

Marked #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs as duplicate.

The patch here doesn't fix #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs. PHP's is_writable() returns false when the directory is writable, so the file_directory_is_writable() function introduced here still fails since it calls is_writable().

The patch that OnkelTem posted in #1333390 does fix it; and as a side effect, fixes this, because it actually writes the file. You can't do that if the directory isn't executable. (See the comments in #4)

This is rather important to me, as I'm attempting to migrate a large number of Drupal sites to a server that uses ACLs. I'll have to manually patch every install after every update until this is fixed.

I re-closed #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs as a duplicate. Let's broaden the scope of this issue to include that bug, and update the patch here to use the technique from that issue:

The patch at #944582 fixes the executability test by introducing a wrapper around is_executable() called drupal_is_executable(). However, it also adds a wrapper, file_directory_is_writable(), around is_writable(), which calls both is_writable() and the new drupal_is_executable().

The latter is incorrect; the only writability test that has any chance of working is the one in this issue: you write a file. If we replace the file_directory_is_writable() in that issue with OnkelTem's implementation of drupal_is_writable() from this issue, it should fix both.

This is the patch we should incorporate into #31:
http://drupal.org/files/linux-acl-drupal-is-writable-d7.9.patch

We also still need to address catch's feedback from #43.

agreed

@David_Rothstein

Would you clarify your proposition via providing alternative patch which we can test on ACL-enabled setups?

Sure, here's a patch along the lines of what I had in mind.

It would need more work to be a real patch, but I think this is probably good enough to test. The essential change is from this:

to this:

***

@mjorlitzky: So is the issue summary at #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs incorrect, then? In any case, seems like this is worth testing.

@mjorlitzky, if the patch is accepted, it will also be backported to Drupal 7. We can probably create a "draft" D7 version for testing as well.

Thank you, I'll try to move a few sites to our new server later tonight and they'll need to be patched. I'll let you know how it goes.

I confirm that patch works on clean Drupal 7.12 on ACL-enabled setup.
I haven't tested on remote file-systems though, which is actually not very important in most setups.

Both approaches - test-writing and with using drupal_realpath() have some drawbacks, but I personally like this solution more, despite this warning in docs:

The use of drupal_realpath() is discouraged, and is slowly being removed from core functions where possible.

I hope we will find another solution until drupal_realpath() is removed from core.

xjm, David_Rothstein - thank you!

I was able to apply http://drupal.org/files/d7-944582-59-do-not-test.patch to Drupal 7.15 and it works.

I think this is a major WTF for users on shared hosting that use ACLs. Quite hard to diagnose too!

I wonder if D6 is affected... probably not, because it doesn't use stream wrappers.

@anarcat

Yes, D6 is affected. At least I remember for sure that I have been creating ugly 'write-test' patch for it.

I tried installing D8 but I get a symfony error, so... unknown.

The patch in #59 still works for D7, can we have that reviewed/tested? The issue that we ran into was that if an ACL allowed read/write access to the (otherwise unreadable) sites/default/files directory, IMCE would claim that it couldn't write there and fail.

And again. Here's a patch rebased for 7.18.

I want to see what the bot says about 7.x (we can change this back to 8.x when the bot comes back).

Also, slightly related: #1885510: Can't install on some systems (like windows) due to folder permissions (executable check not needed) and #1886164: Install in different language requirement warns translations dir not executable, when the dir is executable on some systems

Yes. In those issues, we used the rational that the files dir was not tested for being executable for a reason to take out the executable checking. Another thought there was that trouble with a non-executable directory was so rare, checking was not needed.

So, I'm wondering if people who are familiar with this issue, can provide some guidance and an update:

1) How common is the problem with D7 (or D6) that someone installing or running a site will have incorrect permissions that need to be fixed? Here and in those other issues, to test, the execute permission was deliberately removed. Aside from that, does the situation happen?

2) checkout the approach in #1886164: Install in different language requirement warns translations dir not executable, when the dir is executable on some systems. That tries to keep the checks for executable in, but make it work for windows by checking if whatever/. is readable since the is_executable() function does not work for all systems. How is that different from the approach here?

#1885510: Can't install on some systems (like windows) due to folder permissions (executable check not needed) got committed, so that would need to be taken into account for any further patch updates here

Well, there's no reason to make the installer inconsistent with everything else, so that doesn't seem like a meaningful way to split things to me. But if you think #1333390: file_prepare_directory() / is_writable() on Linux don't support ACLs can be solved without running into the issues here you should consider reopening that one.

As far as I can tell, though, this is all pretty crazy, and #1885510: Can't install on some systems (like windows) due to folder permissions (executable check not needed) at most just worked around the problem? If it's really true that Windows will fail if the directories are (forced to be) executable and Linux will fail if they aren't, that is a tough one to solve :) I certainly didn't realize that before.

This should go back to 8.x at least…

This worked for me. Thanks!