Uploaded files have the permissions set to 600

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

Fine.

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

Thank you.

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

marked #238167: File Attachment Upload not setting proper permissions on uploaded files as a duplicate

marked #236504: file_save_upload does not change the default file permissions to 664 as a duplicate

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

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.

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

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

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?

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;
}

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

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.

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.

based on #13 i'll mark this RTBC

here's a clean re-roll.

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:

Sounds like a good plan to me.

Marking CNW for now.

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

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

Thanks a lot. That one worked for me perfectly.

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

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.

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

here's a re-roll now that #310358: Add a test for file_save_upload and clean up file.test got committed.

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

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

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

marked #290199: 403 Error while downloading uploaded file as a duplicate

Shoot. No longer applies. ;(

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

pretty please?

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/

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?

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.

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.

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.

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

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

marked #196272: uploaded files may lack the read access for everyone as a duplicate

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

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

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

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

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.

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

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.

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

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.

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

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.

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

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.

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

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

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

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:

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:

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

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.

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.

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.

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!

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

this adds a drupal_chmod() function.

The last submitted patch failed testing.

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.

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

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

The last submitted patch failed testing.

subscribing

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.

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.

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.

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.

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.

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

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

The last submitted patch failed testing.

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

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

subscribing

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

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

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

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

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:

The tests and code style look good to me.

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.

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.

Very nice: new, simple API with tests.

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

okay okay, here's the closing defgroup

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

Thanks you very much Andrew.

Looks great! Committed to CVS HEAD. :-)

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

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

yeah it doesn't look like it went in.

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.

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

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

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.

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

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

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.

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

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

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

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

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

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

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.

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

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

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.

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.

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.

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.

subscribing

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 :

marking critical

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

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.

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.

Cleaning up what's have already been done.

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

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

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?

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

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

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.