Hi there,

I got an Drupal6 Installation since 6beta3 and updated it to beta4 an noch rc1. Since the first installation, Files I upload via the "File Attachement" Fieldset in my Node Forms have the permissions 600, which returns a 403 Error Message from the server, if I try to access them via a browser. Which is sort of not, what I want them to do.

I have no Idea, why it is like this, I checked all involved files and folders, if they maybe have a 600 permission or something like this, I even changed boldly all permission of all Files and Folder in that installation to 755. But still uploaded attachments get a 600.

Got anyone any Idea?

Thousand Thanks

Files: 
CommentFileSizeAuthor
#130 0002--203204-port-d7-patch-to-d6.patch4.97 KBflickerfly
#109 0001--203204-port-d7-patch-to-d6.patch5.04 KBanarcat
#108 fix_upload_perms-D6_0.patch760 bytesanarcat
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_upload_perms-D6_0_0.patch. View
#90 file_chmod_203204.patch14.44 KBdrewish
Passed: 10800 passes, 0 fails, 0 exceptions View
#89 file_chmod_203204.patch14.44 KBdrewish
Passed: 10800 passes, 0 fails, 0 exceptions View
#86 file_chmod_203204.patch14.37 KBdrewish
Passed: 10800 passes, 0 fails, 0 exceptions View
#85 file_chmod_203204.patch13.96 KBdrewish
Passed: 10800 passes, 0 fails, 0 exceptions View
#79 file_chmod_203204.patch13.85 KBdrewish
Passed: 10800 passes, 0 fails, 0 exceptions View
#75 file_chmod_203204.patch12.09 KBdrewish
Failed: 10564 passes, 1 fail, 0 exceptions View
#74 file_chmod_203204.patch11.52 KBdrewish
Failed: 10564 passes, 1 fail, 0 exceptions View
#67 drupal.file-chmod-67.patch11.58 KBsun
Failed: 9734 passes, 1 fail, 0 exceptions View
#66 file_203204.patch11.69 KBdrewish
Failed: 9753 passes, 1 fail, 0 exceptions View
#64 file_203204.patch11.54 KBdrewish
Failed: 9734 passes, 1 fail, 0 exceptions View
#59 br_file_chmod_variable_4.patch11.01 KBBevan
Passed: 10810 passes, 0 fails, 0 exceptions View
#59 file.inc_.rej_.txt720 bytesBevan
#55 file_chmod_variable_3.patch10.99 KBOwen Barton
Failed: Failed to apply patch. View
#40 file_chmod_variable_2.patch11.03 KBOwen Barton
Failed: Failed to apply patch. View
#37 file_chmod_variable_1.patch4.04 KBOwen Barton
Failed: 7619 passes, 2 fails, 0 exceptions View
#35 file_chmod_variable_0.patch4.05 KBOwen Barton
Failed: Failed to apply patch. View
#30 file_203204.patch2.45 KBdrewish
Failed: Failed to apply patch. View
#26 upload_203204.patch1.77 KBdrewish
Failed: Failed to apply patch. View
#23 file_203204.patch1.68 KBdrewish
Failed: Failed to apply patch. View
#21 file_203204.patch21.9 KBdrewish
Failed: Failed to apply patch. View
#20 file_203204.patch17.83 KBdrewish
Failed: Failed to apply patch. View
#18 file_203204.patch5.52 KBdrewish
Failed: Failed to apply patch. View
#17 file_203204.patch5.52 KBdrewish
Failed: Failed to apply patch. View
#15 file_203204.patch971 bytesdrewish
Failed: Failed to apply patch. View
#6 file_203204.patch982 bytesdrewish
Failed: Failed to apply patch. View
#1 file.inc_.diff180 bytesfossdeveloper
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file.inc_.diff. View

Comments

fossdeveloper’s picture

FileSize
180 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch file.inc_.diff. View

For a temporary fix you can do this ..add the last two lines to the /includes/file.inc

line no : 569
// Move uploaded files from PHP's upload_tmp_dir to Drupal's temporary directory.
// This overcomes open_basedir restrictions for future file operations.
$file->filepath = $file->destination;
if (!move_uploaded_file($_FILES['files']['tmp_name'][$source], $file->filepath)) {
form_set_error($source, t('File upload error. Could not move uploaded file.'));
watchdog('file', 'Upload error. Could not move uploaded file %file to destination %destination.', array('%file' => $file->filename, '%destination', $file->filepath));
return 0;
}

//New to add for fixing the permission - Changing to proper permission
chmod($file->filepath,0644);

I have attached the diff file to see if this is a good way of doing this...Let the developers decide.

Hey my second patch btw :)

Benjamin Birkenhake’s picture

Fine.

I aadded a comparable fix within in the same file by myself ... 3 lines later. ;)

Thank you.

bdragon’s picture

Or run your webserver under a less restrictive umask...

drewish’s picture

Status: Active » Needs review
drewish’s picture

Version: 6.0-rc1 » 7.x-dev
drewish’s picture

FileSize
982 bytes
Failed: Failed to apply patch. View

+1, re-rolled it and copied the comment from the chmod in file_copy().

Murz’s picture

Does this patch applied to Drupal core in future or stay as hack?

I have configuring hosting server for mass drupal installation and thinking about staying on suphp (and patching every Drupal with this patch) or going to mod_php and having problems with file owner of files, created with php.

Without this patch I try to set another umask in suphp (file /etc/suphp.conf, version 0.6.2) for giving correct permissions of uploaded file, but doesn't found correct umask for this.

 My tests:
umask in suphp.conf - permissions of created file
umask=0227 - 400
umask=0447 - 200
umask=0047 - 200
umask=0077 - 600

Does anybody know what umask I must set in suphp.conf to give read permissions to group or to other?

drewish’s picture

Murz, as far as figuring out permissions give this a read: http://en.wikipedia.org/wiki/File_system_permissions#Octal_notation

Murz’s picture

I try many variants from wikipedia examples and other, but it changes only 'owner' permissions, 'group' and 'other' is staying zero.
For example:
default umask from suphp.conf 0077 must do 600 mask for file. It do this.
when I change umask to 0007, file must have 660 mask, but mask is setting to 600.

I think, this is a suphp bug or maybe a suphp feature.

drewish, can you post any working suphp.conf (version 6.2 or 6.x), that create uploaded file permissions like 644 or 640?

rajaubud’s picture

WOW Thank you very much...it works just copy those patch to the files like this for me works

// Move uploaded files from PHP's upload_tmp_dir to Drupal's temporary directory.
// This overcomes open_basedir restrictions for future file operations.
chmod($file->filepath,0644);/
$file->filepath = $file->destination;
if (!move_uploaded_file($_FILES['files']['tmp_name'][$source], $file->filepath)) {
form_set_error($source, t('File upload error. Could not move uploaded file.'));
watchdog('file', 'Upload error. Could not move uploaded file %file to destination %destination.', array('%file' => $file->filename, '%destination' => $file->filepath));
return 0;
}

drewish’s picture

Component: upload.module » file system

really file system rather than upload.module... could i actually get a review of this patch from someone?

Benjamin Birkenhake’s picture

I do understand the arguments for doing this via file-system instead of the module. BUT ... I am a developer. And my Drupal runs on a usual shared Hosting of one of Germanys major Hosters. So it is very likely that there are users with less knowledge than me will face this problem.

Helpermedia’s picture

I'm glad I found this issue, helped to me solve a problem.

I have applied the patch (file_203204.patch) and it worked alright.

drewish’s picture

Status: Needs review » Reviewed & tested by the community

based on #13 i'll mark this RTBC

drewish’s picture

FileSize
971 bytes
Failed: Failed to apply patch. View

here's a clean re-roll.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Since this is the billionth or so time that we're changing chmod permissions around, we really ought to have a test case for this.

Spoke to drewish in #drupal, and he said:

how about i pull out and strip down the file_test.module from hook_file so we have a target to upload files to and look writing a test against that?

Sounds like a good plan to me.

Marking CNW for now.

drewish’s picture

FileSize
5.52 KB
Failed: Failed to apply patch. View

here's the start on this. can't seem to get the permissions checking working and something is screwy on the uploading.

drewish’s picture

FileSize
5.52 KB
Failed: Failed to apply patch. View

here's the start on this. can't seem to get the permissions checking working and something is screwy on the uploading.

BuyBub.com’s picture

Thanks a lot. That one worked for me perfectly.

drewish’s picture

Status: Needs work » Needs review
FileSize
17.83 KB
Failed: Failed to apply patch. View

[5:36pm] drewish: webchick: so i'll contribute some unit tests for http://drupal.org/node/203204 but i'm going to kill some kittens and include some clean up on file.test
[5:38pm] webchick: drewish: LOL :) You kitten killer. ;) Ok

Here's the patch with a test for file_save_upload(). It creates a file_test.module to serve as a target for the upload. It also adds FileTestCase which serves as a base class for the other test cases and adds some helper functions and assertions. I used this to split up the file move/copy/delete function tests into their own classes.

Update forgot to mention that this adds a chmod check to file_copy/move to validate that the expected change sticks.

drewish’s picture

Status: Needs review » Postponed
FileSize
21.9 KB
Failed: Failed to apply patch. View

More cleanups in the tests, fixed some comments removed some unneeded setUp()s. I think I'm actually going to skip the kitten killing and fork the test fixes into their own issue then come back to this. posting the full patch here so i can be sure to get everything back together.

drewish’s picture

just posted #310358: Add a test for file_save_upload and clean up file.test hopefully it'll get committed quickly.

drewish’s picture

Status: Postponed » Needs review
FileSize
1.68 KB
Failed: Failed to apply patch. View
drewish’s picture

Status: Needs review » Reviewed & tested by the community

bumping this back to RTBC since that's where it was before webchick asked for a test in #16.

sun’s picture

Status: Reviewed & tested by the community » Needs work

Unfortunately, wrong function name: it's assertFilePermissions() now.

drewish’s picture

Status: Needs work » Needs review
FileSize
1.77 KB
Failed: Failed to apply patch. View

okay, fixed that and re-ran the file and upload tests with no problems. rtbc?

drewish’s picture

Status: Needs review » Reviewed & tested by the community
drewish’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Shoot. No longer applies. ;(

drewish’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
Failed: Failed to apply patch. View

Okay, here's a re-roll with some test improvements.

drewish’s picture

Status: Needs review » Reviewed & tested by the community

pretty please?

pbryan’s picture

I too suffer from this bug. Here's the scenario: PHP-CGI runs under a different uid:gid than the web server. Because move_uploaded_file sets the permissions to 0600, the file cannot be served by the web server. Drupal is not serving the uploaded file itself; it's depending on the web server to do this.

Here's a good article on why no PHP script should be making any assumptions about the permissions of files uploaded via PHP, and why PHP scripts should explicitly set the permissions (until PHP finds a way to make things consistent): http://blog.tigertech.net/posts/php-upload-permissions/

webchick’s picture

I was going to commit this in my last committing spree so I have no problem doing it now.

Just one quick question though, is 0664 right? The referenced article as well as the initial patches and reviews were 0644, so just making sure that's not a typo. Are there instances on shared hosts where multiple customers could be in the same group and could dangerously write to each others' file directories?

pbryan’s picture

0644 is probably the "safer" permission in my opinion, but only by a small margin. If the PHP-CGI process that creates the file needs to write/update/delete it, 0644 will be fine. This also follows the umask of 0022, which is most common -- if you create your own file in your home directory, it will have 0644 by default.

Someone may argue that the group should have the same read/write permissions as the user (0664). I'd like the understand how that would be used before commenting. The presumption would be you have some process out there running under the PHP-CGI script's gid but not uid, and needs write access to the file. It could happen in theory, but I'm doubtful as to this being a common scenario.

Owen Barton’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
4.05 KB
Failed: Failed to apply patch. View

There is no 'standard' safe file permission. There are many different ways to setup web hosting, system users and permissions, and they have different ideal permissions.

Some examples (which obviously all have their own security pros and cons):
* Apache and mod_php, webmaster accesses files via apache (e.g. webdav or file management script) with same user account as apache - 0600 is right
* Apache and mod_php, webmaster owns files, shares a group with apache - 0660 is right
* Apache and mod_php, webmaster owns files, shares a group with apache, a backup/replication script needs read only access - 0664 is right
* Apache and php cgi, running as separate users, webmaster accesses files as a cgi user, shares a group with apache - 0640 is right

In other words, Drupal's default file permissions have been (while functional and well tested on many different systems) far too open for many hosting setups for a long time. On shared hosts, this potentially gives other (completely unrelated) vhost users read (or even write, if poorly configured) access to another users files (which is a bad thing if they are relying on private file serving to protect private files). It also opens the system up to privilege escalations, for example being able to utilize a vulnerability in Apache in a CGI setup to create/edit files (which wouldn't be passed through Drupal) when otherwise it shouldn't have that permission.

The attached patch updates the previous patch to HEAD, and also adds variable_gets allowing site admins to specify permissions for writable files and directories in settings.php, or install a contrib module to allow them to tweak them as appropriate (or even automatically detect and set the optimal permissions). The default permissions are unchanged.

c960657’s picture

Latest patch:
+ @chmod(realpath($image), variable_get('file_file_writable', 0664));

Earlier patch:
+ $contents = file_get_contents(realpath($image->filename));

I believe the realpath() call can be omitted in these cases.

Owen Barton’s picture

FileSize
4.04 KB
Failed: 7619 passes, 2 fails, 0 exceptions View

This patch omits realpath() in as suggested (although that might be out of scope of this patch). Color module still seems to work fine either way.
Otherwise patch is identical and open for review...

drewish’s picture

Status: Needs review » Needs work

you really ought to update the tests to use the variable you've defined... actually we should probably test this change.

drewish’s picture

Owen Barton’s picture

Status: Needs work » Needs review
FileSize
11.03 KB
Failed: Failed to apply patch. View

This patch updates existing tests to include the variable. It also adds:
- A new test specifically to check that directory permissions are being set correctly to the configured permissions.
- Tests that files are both readable and writable by PHP in various situations (after various moves and also after upload). We were checking directories are writable this way before, and I think that with the added configurability of this it is worth checking both the correctness of the operation (i.e. the file permissions) and the desired functional outcome (i.e. files should still be readable and writable).

Owen Barton’s picture

Priority: Minor » Normal

Considering the security implications I think this should be normal priority (if not critical...)

c960657’s picture

If you add the following to setUp(), all file tests will pass on Windows too (they currently don't, with or without the patch):

    // Only a few permission values are supported in PHP on Windows. In
    // particular, the "group" and "other" bits always reflect the "user" bits.
    if (substr(PHP_OS, 0, 3) == 'WIN') {
      variable_set('file_file_writable', 0666);
      variable_set('file_directory_writable', 0777);
    }
drewish’s picture

c960657, that makes me wonder if we need to set the default settings in a system update depending on the platform type.

c960657’s picture

I don't think there is any need for that. Windows simply ignores the "group" and "other" parts of the permission mask, so it doesn't matter what they are. Most users wont notice that the actual permissions differ from 0664 and 0775, respectively, unless they are using chmod() and fileperms() directly, and in that case they need to be aware of the Windows limitations anyway.

The default settings work fine on Windows, except that files are writable by all users on the machine. I don't think that problem can be solved using chmod(). If a Windows user wants more fine-grained access control, he can probably adjust the permissions on the directory containing the files.

Mentioning the Windows limitations in the documentation of these new variables would probably be a good idea, though I am not sure exactly what these limitations are. The code comment in #42 was just based on a few experiments with PHP 5.2.6 on Windows XP.

Damien Tournoud’s picture

Why not reporting that bug to PHP? We should not have to deal with this.

ñull’s picture

See this bug report http://bugs.php.net/bug.php?id=42291&thanks=6 . Everyone who finds this issue important enough, please make use of the possibility to vote there. In the meanwhile I'll just have to wait until my distribution finally brings out the version that fixes this. In the bug report itself you can read suggestions to work around it, almost treating it as a feature rather then a bug.

I am really sick and tired of this attitude that we pass the responsibility on the other. You just have to consider if this is urgent enough and if we have the time to wait until it is fixed in PHP and it becomes finally available in a proper repository. We cannot expect normal users to find exotic hosting providers that compile the latest development snapshots. Multi platform also means that we take into account some awkward peculiarities that might take a little bit longer to be noticed by other (PHP) development communities.

drewish’s picture

com2, I think Damien Tournoud was referring to the Windows specific differences.

David Strauss’s picture

Group write is probably a good idea for uploaded files. Under mod_php, users often only have access to files Apache has decided to allow group writes to.

Owen Barton’s picture

The current patch in #40 has group write permissions (same as existing core code).

Damien Tournoud’s picture

Status: Needs review » Needs work

The patch looks good, but the two new variables (file_file_writable and file_directory_writable) are misnamed. Those should be named file_mask and file_directory_mask or something like this.

Owen Barton’s picture

@Damien I agree there could be more descriptive, however a "mask", at least the octal form of it, is the inverse of permissions (i.e. it defines what bits should NOT be set), so I think would be confusing in this case. See http://en.wikipedia.org/wiki/Mask_(computing) and http://en.wikipedia.org/wiki/Umask

I guess we could go with file_file_writable_perms and file_directory_writable_perms perhaps, although that is beginning to get long. What do you (and others) think?

sun’s picture

Even if it's platform-specific, how about file_chmod_directory and file_chmod_file? IMHO, those would be clear to everyone at first sight.

Owen Barton’s picture

@sun - I don't have a problem with chmod specifically, but I think that we somehow need to also indicate that the specific value of this variable should make the files writable - otherwise the usage/intention is not clear enough.

file_chmod_directory_writable and file_chmod_file_writable work for me (although they are also a bit long).

Damien Tournoud’s picture

@Grugnog2: writable should go. It has little sense IMHO.

Owen Barton’s picture

Status: Needs work » Needs review
FileSize
10.99 KB
Failed: Failed to apply patch. View

@Damien - ahh yes, I see your point - if anything the chmod is done to make files webserver readable (or conversely "other accounts" non-readable, depending on setup) - nothing really to do with making them writable.

Here is a new patch that goes with sun's suggestion, which I think is clearest.

drewish’s picture

Status: Needs review » Needs work

Generally this looks pretty good. I like the new choice of variable name.

I think we should either remove the comment from the end of:

+      @chmod($directory, variable_get('file_chmod_directory', 0775)); // Necessary for non-webserver users.

or expand it to be something more intelligible. My preference would be to just drop it.

I still want to see the tests actually changing the file_chmod_* variables and then checking that the files end up with the appropriate permissions.

It's not clear to me why after checking the permissions we're calling:

+    // Verify file actually is readable and writeable by PHP.
+    $this->assertTrue(is_readable($new_filepath), t('File is readable.'), 'File');
+    $this->assertTrue(is_writeable($new_filepath), t('File is writeable.'), 'File');

Does this really catch any cases that checking the octal permissions would not? It seems like it just adding lines and noise to the tests.

sun’s picture

Without knowing context and having looked at the code: Windows only supports is_writeable() and is_readable() and only certain octal permissions.

drewish’s picture

sun, i don't see anything on the docs for fileperms() that indicate it wouldn't work on windows. see c960657's comments on #44.

Bevan’s picture

FileSize
720 bytes
11.01 KB
Passed: 10810 passes, 0 fails, 0 exceptions View

The attatched hunk failed, due to some refactoring of file_save_upload() to reuse the same row in the database for uploaded files. I put the additional chmod in this patch before the additional code that was already committed, to group filesystem-manipulating code together, and database-manipulating code together.

I rerolled Grugnog2's patch from #55. All file tests pass.

drewish’s picture

I'm thinking it might make sense to add a drupal_chmod() function to wrap this. It'll also tie into some of the stream wrapper work that's going on else where.

the issues i raised in #56 still apply.

flickerfly’s picture

Just a refuge from duplicate #339539: File upload permissions do not allow FTP or other users to read files: Patch for D6 plugging into the discussion. Looks like progress is being made. Thanks!

kiamlaluno’s picture

Title: Uploaded Files have 600 » Uploaded files have the permissions set to 600
KNerd Terry’s picture

Hi all
Is there a more recent patch or is the one from #59 good?

drewish’s picture

Status: Needs work » Needs review
FileSize
11.54 KB
Failed: 9734 passes, 1 fail, 0 exceptions View

this adds a drupal_chmod() function.

Status: Needs review » Needs work

The last submitted patch failed testing.

drewish’s picture

FileSize
11.69 KB
Failed: 9753 passes, 1 fail, 0 exceptions View

hummm... didn't notice the failure... i'll have to look into that. in the mean time here's a version that removes the @ before file_put_contents() and adds a watchdog message when chmod fails.

sun’s picture

Status: Needs work » Needs review
FileSize
11.58 KB
Failed: 9734 passes, 1 fail, 0 exceptions View

Actually, I cannot see a reason why FileUnmanagedCopyTest() should fail here.

Only minor clean-ups here. RTBC for me (when tests pass).

Status: Needs review » Needs work

The last submitted patch failed testing.

Andrew Schulman’s picture

subscribing

fodber’s picture

It is a security hole to let attachments be downloaded by Apache.
Attachments should be served only by php scripts where access control can be carried out.

So IMHO this is not a bug, or actually the bug is that attachments are just downloaded from the filesystem.

drewish’s picture

fodber, reading your message i don't feel like you really understand the issue at hand. drupal has two file transfer modes, public and private. you seem to only be taking the later into account. but regardless of that we need to standardize on permissions for files that drupal creates.

sun’s picture

Agreed. When using private file transfer mode, the files directory should be located outside of the document root anyway. What matters is that you can access the files and not only the system user the web server is invoked with.

Ingumsky’s picture

There's still no progress with file uploading as I can see -(( I've just updated my production site to 6.10 and was forced to patch file.inc with #6 by drewish as I did it last three or four times -(

But anyway thank you drewish! Your patch works fine as ever.

drewish’s picture

Issue tags: +D7FileAPIWishlist
FileSize
11.52 KB
Failed: 10564 passes, 1 fail, 0 exceptions View

here's a clean re-roll... i'm going to see if i can't get this straightened out on the plane home from drupalcon.

drewish’s picture

FileSize
12.09 KB
Failed: 10564 passes, 1 fail, 0 exceptions View

found a chmod() that was just added to the image code.

anarcat’s picture

Status: Needs work » Needs review

I guess this should be back in the testing queue now?

Status: Needs review » Needs work

The last submitted patch failed testing.

Martin.Schwenke’s picture

The current patch looks really good (apart from failing testing). I think the 0644 is the correct mode for files since that is consistent with all the previous file chmod()s in includes/file.inc, so I won't want to change it.

However, for those who might want to change the file_chmod_directory and file_chmod_file options, it took me a bit of searching to figure out how to set them. Will there be a GUI option to set these options? Or should I shut up about that until the current bug is fixed? :-)

drewish’s picture

Status: Needs work » Needs review
FileSize
13.85 KB
Passed: 10800 passes, 0 fails, 0 exceptions View

Martin.Schwenke, I was thinking that leaving it as a magic setting would work for now and we could do a follow up to discuss adding a setting for it.

I added a few more calls to assertFilePermissions() in the unmanged file copy tests to get all the cases and in the process found a couple of oddities in FileUnmanagedCopyTest::testNormal() that I corrected. I also modified drupal_chmod() to check the $mode parameter using !isset() rather than empty() because a mode of 0 is valid.

Eventually I tracked down the issue to assertFilePermissions() reading cached file stat info. Adding a call to clearstatcache() cleared the test failures right up (pardon the pun).

nmashruwala’s picture

Version: 7.x-dev » 6.x-dev

subscribing

anarcat’s picture

Version: 6.x-dev » 7.x-dev

I don't think you meant to change the version here.

nmashruwala’s picture

eep! You're right, I didn't; thanks for changing it back :)

drewish’s picture

any takers for a review? i think this should be good to go.

Owen Barton’s picture

Status: Needs review » Needs work

- * @} End of "defgroup file".
I see this removed - is this intentional? I don't see the start of the group removed.

+ * This function will use the 'file_chmod_directory' and 'file_chmod_file'
+ * variables for the default modes for directories and files. By default these
+ * will give everyone read access so that FTP'd users or non-webserver users
+ * can see/read these files, and give group write permissions so group members
+ * can alter files uploaded by the webserver.

I think it would be good to note that the defaults are intended for uploaded/generated files (i.e. and may not be correct for edge cases, like settings.php etc) and also give some specific examples ("FTP's" looks a little odd). What about:

+ * This function will use the 'file_chmod_directory' and 'file_chmod_file'
+ * variables for the default modes for directories and uploaded/generated files.
+ * By default these will give everyone read access so that users accessing the
+ * files with a user account without the webserver group (e.g. via FTP) can read
+ * these files, and give group write permissions so webserver group members
+ * (e.g. a vhost account) can alter files uploaded and owned by the webserver.

The tests and code style look good to me.

drewish’s picture

Status: Needs work » Needs review
FileSize
13.96 KB
Passed: 10800 passes, 0 fails, 0 exceptions View

I don't think we really need to keep the end group bit, sort of like you don't need the ?> tag at the end of a php file.

Your suggested comment seemed like a clear improvement so I just adopted it.

drewish’s picture

FileSize
14.37 KB
Passed: 10800 passes, 0 fails, 0 exceptions View

chx did a review on irc and pointed out:
- that having separate message for passing and failing tests doesn't match up with the rest of core so i fixed that to just show expected and actual permissions in both cases.
- i should be using !isset() rather than is_null() in the assert functions.

fixed both of those.

chx’s picture

Status: Needs review » Reviewed & tested by the community

Very nice: new, simple API with tests.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work

As far as I know, we always close the doxygen groups. Why would we change that?

drewish’s picture

Status: Needs work » Needs review
FileSize
14.44 KB
Passed: 10800 passes, 0 fails, 0 exceptions View

okay okay, here's the closing defgroup

drewish’s picture

FileSize
14.44 KB
Passed: 10800 passes, 0 fails, 0 exceptions View

damz pointed out that i'd dropped a new line.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Thanks you very much Andrew.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Looks great! Committed to CVS HEAD. :-)

ñull’s picture

Status: Fixed » Patch (to be ported)

Can this be backported to D6? Nice that it is fixed in future version of Drupal, but what about us users of the present?

c960657’s picture

Was this actually committed to HEAD? I cannot see the commit on http://cvs.drupal.org/viewvc.py/drupal/drupal/includes/file.inc

drewish’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

yeah it doesn't look like it went in.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Oops, the commit failed and it was accidentally committed with my last commit. As a result, no credit was given, for which I apologize. I can do a dummy commit if you want to get the credit. Just re-open the issue.

Thanks for all the hard work, andrew.

Status: Fixed » Closed (fixed)

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

Ingumsky’s picture

Status: Closed (fixed) » Active

It's still an issue in 6.11 as I can see. I thought it was backported to 6.x, wasn't it?

Bevan’s picture

Status: Active » Closed (fixed)

It was not backported to 6. Please create a new issue if you think that is necessary. Since 6 is stable this is probalby too-large a change though.

wretched sinner - saved by grace’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)

@Bevan I think it is worth having a discussion on a D6 backport, due to the premise in #339539: File upload permissions do not allow FTP or other users to read files: Patch for D6 that the default permissions changed from D5. Here is the best place for that discussion, as we have the history.

If Gabor could let us know if he would be willing to consider this fro D6, then we know whether to push ahead on it or not.

corcken’s picture

Version: 6.x-dev » 6.12

I don´t get it. Is this going to be fixed in a Update of 6.xx or will this bug just stay? Do i have to patch if i need this in 6 or is it worth waiting for a fix? Sorry...

kiamlaluno’s picture

Version: 6.12 » 6.x-dev

The referring version is 6.x-dev because it's the version where the bug will be fixed. This means that a future release of Drupal 6 will contain the fix; the future release could be 6.13, or 6.14, or 6.15.

The only way to fix a bug in an official release is to create another official release.

flickerfly’s picture

Looks good, glad it was fixed in D7, but could really use it in the next version of D6 since many are still applying security upgrades and those with this patch installed will have extra steps (and hopefully will remember to do it, a classic problem of mine).

marcoBauli’s picture

just adding myself to the list, i'm experiencing this in D6 too..

prodosh’s picture

A backport is absolutely needed. Many users will be living with Drupal 6 for a quite a while to come and on a daily basis this is one of the biggest nuisances they face in Drupal 6.

In the meantime there is a Drupal 6 patch here: http://drupal.org/node/339539

jordan8037310’s picture

Adding myself to the list as well, still would like to see this fixed in 6.x official release.

mimhakkuh’s picture

subscribing. i'd love to see this fixed in an d6 upgrade.

anarcat’s picture

Status: Patch (to be ported) » Needs review
FileSize
760 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix_upload_perms-D6_0_0.patch. View

So this is not exactly a port of the D7 patch (which includes unit testing), but I think it's still worth a review here and see if it would pass testing... I can work on porting the actual patch (minus unit testing): http://drupal.org/files/issues/file_chmod_203204_5.patch

anarcat’s picture

here's a more thorough port of the D7 patch. The only place I could find that wouldn't port properly is in the image.inc file, since D6 doesn't have image_save()... I have also found other chmod() occurences, but they're in the install system (modules/system/system.install and includes/install.inc), so I'm deliberately not touching that for now, as this hasn't been patched in D7 either.

kiamlaluno’s picture

The patch looks good. As Drupal 6 API changes are not allowed, I guess the patch will not be ported to Drupal 6.

anarcat’s picture

Well, that patch is explicitely targeted at Drupal 6... It's not a really major API change: just adding a function. We're not changing order of arguments or an existing API in any way, plus it's forward compatible with D7 (although that could change).

If someone else is also of this opinion, I'll reroll the patch without that function (if so, please mark as needs work).

kiamlaluno’s picture

It's not a really major API change: just adding a function.

Still, it's a change in the API, and those kind of changes (even if it just a function) are not done, in Drupal 6.
You can submit a patch to fix a bug, and that will be committed; different patches will not applied to Drupal 6 code.

Martin.Schwenke’s picture

This is strange. The usual reason for disallowing API changes in a stable release cycle is to avoid incompatibility. Adding a function to an API does not introduce any incompatibility, it just adds functionality.

drewish’s picture

I think that Martin.Schwenke is correct in this case. We're not breaking any existing calls since it's an addition to the API. Gabor or Dries will have to be the final answer though.

Owen Barton’s picture

Status: Needs review » Reviewed & tested by the community

I don't see any issues with committing this to Drupal 6 - this will not break any contrib modules, and they are free to use their own methods of chmod-ing if they choose (although hopefully they will migrate to this method over time). What's more this will fix an ongoing issue that sets the bar for people using Drupal higher than it needs to be.

Read through the patch and it all looks clear and sane.

ac’s picture

subscribing

adrian’s picture

This needs to be committed to the next point release.

this basically breaks the ability for scripts which do backups and restores of sites running as something other than the web user.

setting 'set uid' on the files directory doesn't work either, from man chmod :

           4000    (the set-user-ID-on-execution bit) Executable files with this bit set will run with effective uid
                   set to the uid of the file owner.  Directories with the set-user-id bit set will force all files
                   and sub-directories created in them to be owned by the directory owner and not by the uid of the
                   creating process, if the underlying file system supports this feature: see chmod(2) and the
                   suiddir option to mount(8).
adrian’s picture

Priority: Normal » Critical

marking critical

kiamlaluno’s picture

Priority: Critical » Normal

Critical
When a bug renders a module, a core system, or a popular function unusable. Possible examples from core are the inability to create a new node or blocks won't display. These are to be fixed immediately because the software is not usable at all. Basically if the project (core, module, theme, etc.) can't be used as intended, then it is a critical problem.

This is not a critical bug; you are still able to create nodes, create comments, etc...

anarcat’s picture

Priority: Normal » Critical

This bug was marked critical after discussions with the D6 maintainer. Furthermore, without this patch, backing up a Drupal site or through FTP with an external tool is impossible. While backups are not part of core (and I don't argue for core inclusion here), I sure think it's a critical issue that needs to be resolved. It's also impeding the work of other projects like Drush and Aegir and has been accepted in Drupal 7.

Marking critical again.

kiamlaluno’s picture

I wonder how the bug could be critical in Drupal 6, and not in Drupal 7; the number of sites running Drupal 6 is not a measure of the criticality of the bug, IMO.

Damien Tournoud’s picture

Cleaning up what's have already been done.

prodosh’s picture

@KiamLaLuno:
The primary purpose of the Upload module is to allow a user to upload a file as an attachment to a node and allow the node owner and other users with permissions to download attachment.

This bug prevents the node owner and other users from downloading the attachment. Hence using your quote "When a bug renders a module, a core system, or a popular function unusable." is in fact true here.

Lots of people including myself will be grateful to have the fix implemented soon.

adrian’s picture

I managed to work around this for aegir by adding 'umask(0002)' to the settings.php file.

prodosh’s picture

I can confirm that Adrian's "umask(0002);" (#124 above) in settings.php works for standard Drupal 6.13 installation as well. Thanks for this really simple fix - sure beats patching.

Until the patch finds its way into core, would it be an idea to add this into the settings.php file of the next Drupal release with some comments about what it's for?

varenius’s picture

I have another solution: use the filefield_paths module. My problem was that the uploaded files got permission 600 and therefore I could not copy the files over FTP with my ftp user. The files were accessible on the site just fine, it was just that backups were tricky (impossible with many files).

#124 didn't work for me, the files still got 600.

However, I have another site on the same server which didn't show this problem. I figured out that the difference was the filefield_paths module (http://drupal.org/project/filefield_paths). This module makes it possible for me to have the filesystem in sites/default/files but get my uploded files in a subfolder, for example sites/default/files/uploaded_files. When filefield_paths is enabled I can configure a content type to put any attached files in my subdir uploaded_files. These files does get 664 and not 600. so using filefield_paths to move the files to a subdir solves my problems.

Note: When you upload a file to a node, filefield_paths doesn't move it and set right permissions right when you click "Attach". You have to also click "Save" to save the node. After that, everything works for me. I have no explanation for exactly why and how this works, but I'm happy for the moment.

Drupal 6.13, core Upload module, filefield_paths 6.x-1.3 (most recent).

M_Z’s picture

@MagicalBengt (#126): Great suggestion! I tried it out and it worked very well! This should solve the problem in most cases of others as well.

I figured out what's behind this solution: filefield_paths module uses file_move function of file.inc (which uses file_copy function with chmod(..., 0664) - also located in Drupal core file.inc) to move uploaded files (which are "temporarily" located at "sites/default/files" and "temporarily" have the "wrong" permissions 600) to files path which you have to specify in your content type settings (e.g. "sites/default/files/uploaded_files"). The file movement will be started after clicking "Save" (and not after clicking "Attach") which is the reason for the behaviour described at #126.

The only drawback is - as far as I can see - that you have to set such a files path for each content type where you want to use file upload. Or is there a better way to do this settings?

But it is a clean solution without need to patch Drupal core.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

From reading the comments, it looks like the discussion was around whether this is an API change or not, and once you decided it is not, you RTBC-ed it. I'd say it is an API change (yeah, an API addition is an API change, since modules written for the Drupal version shipping with drupal_chmod() would not be compatible with earlier Drupal versions). So it is an API change.

Anyway, a similar issue with set_time_limit() arose not long ago, and the fine folks solved it in D6 by not introducing a drupal_set_time_limit() but copy the relevant code to all parts where it should be used. That one was a smaller code snippet, so it was fine to copy around. In this case, it looks like unmanageable that way (we have a watchdog message and at least a variable_get()). So since this issue is important, this API addition seems reasonable.

However, looking at the patch, there are a few things:

- the patch uses file_put_contents() which is PHP 5 only; Drupal 6 is supposed to work on PHP 4
- at one point chmod($dest, ... becomes drupal_chmod($destination, ... without that variable NOT renamed elsewhere
- one chmod(realpath($image), ... becomes druapl_chmod($image, ... (note lack of realpath).

To me this makes it look like this patch was not tested. Please do not mark a patch RTBC based on your gut feel of being API change or not, but instead concentrate on testing the actual patch. Also, unfortunately the feedback after it being marked RTBC is on workarounds, not actual patch testing.

huntny’s picture

I find the way to figure it out without touching drupal core.
By putting in a few lines of code to chmod a permission at settings.php

if(isset($_FILES['files'])){
   foreach($_FILES['files']['tmp_name'] as $key => $val) chmod($val,0644);
}

Hope this help us while waiting for drupal core to patch things up.

flickerfly’s picture

Status: Needs work » Needs review
FileSize
4.97 KB

I made the corrections that Gabor mentioned above.

flickerfly’s picture

I tested the patch on a basic, fresh install of drupal with the following vital stats:

PHP Version => 5.2.4-2ubuntu5.10
System => Linux core 2.6.18.8-linode22 #1 SMP Tue Nov 10 16:12:12 UTC 2009 i686

I stand behind it as working well in that environment, but am not setup to test it on PHP 4. If some others could test it out and move the status to RTBC (reviewed & tested by the community), I'll see if I can encourage Gabor to check this out again.

Patching is as simple if you know the correct commands. This one is a bit different from the typical because of anarcat's dev environment. (just add the option -p1 instead of -p0 as is normal.) Here are the directions to get you testing:

  1. Place the patch in the root of your drupal install
  2. run this command: patch -p1 < 0001--203204-port-d7-patch-to-d6.patch
  3. Test the uploading and ability to download those files you uploaded. (Won't work on previously uploaded files that failed.)
  4. Come back and tell us it works great (or how it failed if you must)
Budi’s picture

Status: Needs review » Reviewed & tested by the community

Tested on PHP 4.4.6 and it seems it works well.

jordannoel’s picture

thanks #126
MagicalBengt

filefield_paths module (http://drupal.org/project/filefield_paths) really helps....

greenmachine’s picture

The umask trick at 124 was not working on my server (running aegir), and thus some files were ending up at 600 and hence not able to be backed up by aegir.

Applying the patch at #130 resolves the issue for me. However I had to do the patch somewhat manually as I got the following message:

patch: **** malformed patch at line 43: $variables = array('%directory' => $directory, '!htaccess' => '
'. nl2br(check_plain($htaccess_lines)));

flickerfly’s picture

greenmachine, did you use the -p0 or -p1 argument in the patch command. You may have missed the note in #131 about that. Thanks for testing it.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

@flickerfly: I'm seeing the same error, wondering how @Budi tested this at all?

$ patch -p1 < 0002--203204-port-d7-patch-to-d6.patch.txt 
patching file includes/file.inc
patch: **** malformed patch at line 43:        $variables = array('%directory' => $directory, '!htaccess' => '<br />'. nl2br(check_plain($htaccess_lines)));
flickerfly’s picture

@Gabor: Thanks, sorry for wasting your time.

I'm going to leave PHP4 support to someone else as it isn't in my present or future.

I'd like to not have to apply it with each upgrade so if someone else has the steam to resolve that matter, please do!

alanburke’s picture

@adrian
In #124, what exactly did you add to settings.php to sort this temporarily.

darrice’s picture

@alanburke

You just add the following to a line in your settings.php file (probably along with some comments ;):

umask(022);

Some info on umask:

standard:
rwxrwxrwx = 777
with umask 002:
rwxrwxr-x = 775
with umask 022:
rw-r--r-- = 644

So unless you have a need to execute the uploaded files (like using aegir), you should probably stick to using umask(022) resulting in the normal 644 permission

s.Daniel’s picture

Confirming previously posted results. Patch 0001 works as expected (php5). 0002 throws the error Gabor posted.

N.Berg’s picture

Patch 0001 worked well and fixed the problems for us on drupal 6.20, php5. (Patch 0002 failed to be applied)

dpearcefl’s picture

Status: Needs work » Postponed (maintainer needs more info)

Is this issue fixed in the latest D6? is there any interest in pursuing this issue?

sun’s picture

Status: Postponed (maintainer needs more info) » Needs work

@dpearceMN: Please stop doing this.

dpearcefl’s picture

I respectfully asked the questions. Perhaps if some action was shown on this issue, I wouldn't have to ask.

flickerfly’s picture

There is interest, no it has not been fixed in D6, but I just run a patch after each time that I update and it seems to work out fine.

dpearcefl’s picture

flickerfly, I see you submitted a patch in #130. Is this the patch you are using to patch your 6.X site? Would you be interested in resubmitting it so it gets into the 6.x-dev codebase? You submission in #130 was ignored I believe because the filename didn't match was the testbot was expecting.

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.