SA_2006_006 patch creates .htaccess file that without regard for umask and breaks file downloads [#86737]

The problem is that the SA_2006_006 patch in includes/file.inc creates an .htaccess file without regard for the systems umask. On restrictive systems,

(a) the created .htaccess file has perms of 0600,
(b) apache can not read this file and puts an error message in the logfile
(c) apache will not server any files in this directory

The created .htaccess file should have permissions of 0644. I've attached patches for Drupal 4.6 and Drupal 4.7.

This is related to a bigger problem I've had with uploaded files on my server. I've also had to patch image.module for Drupal 4.6 and img_assist.module for Drupal 4.7 to allow the user to set the uploaded file permissions. Is there a better way to handle this, such as a core setting that defines the uploaded permission for all files?

Comment	File	Size	Author
#11	chmod_1.patch	778 bytes	chx
#11
#6	chmod_0.patch	612 bytes	chx
#6
#5	chmod.patch	610 bytes	chx
#5
#1	file.inc.46.diff	833 bytes	douggreen
#1
	file.inc.47.diff	833 bytes	douggreen

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

douggreen CreditAttribution: douggreen commented 29 September 2006 at 23:38

Version:

4.7.3

» 4.6.9

File	Size
file.inc.46.diff	833 bytes

4.6 patch

Comment #2

douggreen CreditAttribution: douggreen commented 29 September 2006 at 23:48

Version:

4.6.9

» 4.7.3

Comment #3

douggreen CreditAttribution: douggreen commented 1 January 2007 at 13:53

Version:	4.7.3	» 5.0-rc1
Priority:	Normal	» Critical
Status:	Needs review	» Active

I'm changing the version to the 5.0 release candidate and status of critical after confirming that the problem still exists.

To reiterate this problem, when the system file path and the temporary directory are the same, .htaccess to that directory (SA_2006_006). This is done without regard for the web server's permissions. And thus on some systems, the newly created file will have permissions 0600 and not be readable by the web server. An unreadable .htaccess file will actually deny access to all files in the directory.

This is a real problem that can be fixed by a single line. If the attached chmod patch isn't sufficient, could we have some discussion, and maybe work on a fix using the new drupal_install system?

Comment #4

drumm

he/him

NY, US

CreditAttribution: drumm commented 2 January 2007 at 05:18

Status:

Active

» Needs work

The 4.7 patch does not apply to HEAD and should be 'quoted with single quotes and '. $variables .' put in like that.'

Comment #5

chx CreditAttribution: chx commented 2 January 2007 at 08:32

File	Size
chmod.patch	610 bytes

And the 4.7 patch is a reversed one.

Comment #6

chx CreditAttribution: chx commented 2 January 2007 at 08:35

Status:

Needs work

» Needs review

File	Size
chmod_0.patch	612 bytes

Oh and it needs review now. I changed to single quotes but I think it was better with double quotes. The notion that "double quotes are slower" are not true for very, very long.

Comment #7

Dries CreditAttribution: Dries commented 2 January 2007 at 10:10

This looks good to me, but I haven't tested it. :)

Comment #8

ChrisKennedy CreditAttribution: ChrisKennedy commented 2 January 2007 at 10:22

Perhaps it doesn't matter, but it seems cleaner to do the chmod() after fclose().

Comment #9

Morbus Iff

they/them

CreditAttribution: Morbus Iff commented 2 January 2007 at 12:51

I agree the chmod should go after the close. However, this should be 664. 644 will prevent users from accessing the files should it get created with the webserver's username (which is a far more common situation than a suEXEC'd webserver, which is what 644 caters too). More info at http://drupal.org/node/10658. 664 is in core for file creation.