Make WordPress Core

Opened 15 years ago

Closed 15 years ago

Last modified 14 years ago

#2190 closed defect (bug) (fixed)

Imge upload and suexec problem

Reported by: SteveAgl Owned by:
Milestone: Priority: high
Severity: critical Version: 2.0
Component: General Keywords:
Focuses: Cc:

Description (last modified by davidhouse)

On my provider that use suexec and standard dir perms are drwxr-s--- when I upload an image, and as today, WP creates new directories 2006/01 this dirs are created with perms drwxr-x-- and this make the image unaccessible and it show up only the alt text.

Cause the chmod using FTP sw like FIlezilla can't set s neither a shell access is available this make necessary use a tool the provider give through Cpanel to reset all directory perms to default, this solve the problem but it's annoyance.

Cause this it's a larger hosting provider that has tons of WP users the problemi will rise ia lot in few days as i see on my forum

Attachments (2)

suid.diff (513 bytes) - added by ryan 15 years ago.
Preserve suid and guid bits on upload dirs
dir.diff (729 bytes) - added by ryan 15 years ago.
umask test

Download all attachments as: .zip

Change History (28)

#1 @SteveAgl
15 years ago

I noticed that directory created by the cache system got the right permission, then problem looks limited to dirs created through the upload interface

15 years ago

Preserve suid and guid bits on upload dirs

#2 @ryan
15 years ago

See the attached patch. Look in wp_upload_dir() in the wp-includes/functions-post.php. Change 0000777 to 0007777. This should cause all directories created under wp-content/ to have the same permissions as the wp-content/ directory. wp-content/ needs to have to proper permissions already set.

Does that help? Note that the new permissions will apply only to newly created directories. You'll have to delete the directories with the bad permissions so that they are recreated with the new permissions.

#3 @ryan
15 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [3388]) Preserve suid and sgid bits when creating new directories. fixes #2190

#4 @SteveAgl
15 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Nope it still create the directory without the right chmod, i didn't tested but maybe cache will be affected too now ?

#5 @SteveAgl
15 years ago

  • Priority changed from normal to high
  • Severity changed from major to critical

#6 @ryan
15 years ago

  • Milestone set to 2.0.1

#7 @SteveAgl
15 years ago

As told you on meetup of yesterday, I tried with a fresh install. The wp-content is drwxr-s---, after I run WP, the cache dir got drwxr-x---the same for uploads when i uploded a picture. What is tsrange is tht dirs inside cache dir got the same permission of wp-contents (drwxr-s---). Directories inside uploads dir didn't get the same but inherit the permission of uploads dir. This make that in the write panel the picture doesn't show up, neither it dows in public post. I hope that differences between cache dir and it's subdir will help you undrstand what the problem is. Later today yo can find me on AIM (ID steveagl) or gtalk (stefano.aglietti) if youneed more info or other test. If necessary i can opne an informative ticket with my provider.

#8 @ryan
15 years ago

It works okay for me. Look in the save() function in wp-includes/cache.php. See the following code:

		// Make the base cache dir.
		if (!file_exists($this->cache_dir)) {
			if (! @ mkdir($this->cache_dir))
				return false;
			@ chmod($this->cache_dir, $dir_perms);

Remove the "@" characters to enable verbose output. Maybe that will tell us what's going on.

#9 @SteveAgl
15 years ago

I tested, no errors, the cache dir was recreated (i deleted it first) with no errors but with drwxr-x--- same for the uploads dirs. In cache dir then the subdirs were created with drwxr-s--- the subdir in the uploads where created with drwxr-x--- making them unreadable.

Maybe neither the cache is corrected read i can't check it. But the different way subdirs get permission it's really strange. I was wondering if could depend by the umask value that get applied when dirs got created.

that's a big problem cause this provider have tons of people with WP blog installed, i dunno, cause i don't have any experience about that, if could be a misconfiguration of suexec. I'm opening a ticket with the holster to get some info. If you want i can give you access to a domain I'm not using and that i use now for testing this, so u can investigate better by yourself

#10 @ryan
15 years ago

umask would affect the initial creation of the directory, but we immediately chmod the permissions of the directory after creating. Maybe that chmod isn't working for some reason.

Try the attached patch. Does that help?

15 years ago

umask test

#11 @SteveAgl
15 years ago

No luck, applied the patch, uploaded modified cache.php, deleted cache dir and subdirs, openend the blog. The cahce dir was recreated wth same permission (drwxr-x---) subdirs have the right permission same as wp-content (drwxr-s---).

I have a ticket openend with hosting provider too, but till now no answer :(

#12 @SteveAgl
15 years ago

My provider asked me if we need to know how apache with suexec was compiled... iI don't think it does matter doesn't it ?

#13 @ryan
15 years ago

I don't think it does, but any extra information wouldn't hurt.

#14 @SteveAgl
15 years ago

I got some info and maybe a solution from my hosting, here there is the translation of the answer (not literal translation)

Our suexec is fully recoded by our developper but we do not think the problem is in the suexec.

We have a security system that make unreadable all the folders setted to 777

Just yesterday on e of our customers complained about the fact that when uploading some images with WP they didn't show up and every time he had to use our cpanel option to restore the permission on the whole site directory. From this wee realized that there was something wrong with the CHMOD and the perssion settings, stuff unecessary on our web spaces.

We edited the "admin-functions.php" at line 1760 (they refer to 2.0 distibution) where we had commented the line cthat does the CHMOD.

Set correct file permissions
$stat = stat(dirname($new_file));
$perms = $statmode? & 0000777;
@ chmod($new_file, $perms);

Set correct file permissions
$stat = stat(dirname($new_file));
$perms = $statmode? & 0000777;
@ chmod($new_file, $perms);

Now the image upload works flawlessy.

A short suggest to WP developpers. Instead that use always CHMOD they could check, using "is_readable()" and "is_writable()" if the file need to be chmoded or not

I hope this will help to solve the problem, i wait for a feedback :)


#15 @ryan
15 years ago

The idea is to have the user set their preferred permissions on wp-content and then propagate those permissions to all directories and files created by WP. A new directory may be writable/readable without the chmod(), but it may not have the preferred permissions. This permission propagation is done so that the directories can be made writable by both the user and the web server or so that various security schemes can be accommodated. If wp-content has correct permissions for a given host, then I'd think those permissions could be applied evenly to all subdirs and files.

I'm wary of making this change since it could break other hosts. An alternative we could try is to strip the execute bit off the permissions of uploaded files. Changeset [3501] does just that. Does that help?

#16 @davidhouse
15 years ago

Did [3501] fix this or just help? Can I close it?

#17 @davidhouse
15 years ago

Never mind, I should have refreshed before posting that.

#18 @SteveAgl
15 years ago

Any luck again, i uploaded the last SVN with the changeset 3501 in a full clena fresh installation, after logging to blog and tried to upload n images the cache dir and wp-content still get sthe same perssione as I described above.

I always think it's strange that cache subdirs get a different permission than wp-contents subdirs, bue they have the same permission when created.

No idea how to get around this problem

#19 @SteveAgl
15 years ago

Got another suggestion from my hosting provider:

Before to create a directory, the script could compare the UID (using
getmyuid()) of itself and the UID of the parent directory (wp-content in this case using fileowner ("file or directory name")) directory, if this compare fails (when PHP run as an apache module) the new directory will be created with MKDIR ("dirname", 0777), if it pass (when PHP run as a CGI module and then it doesn't needs chmod) the directory will be created with mkdir ("dirname").

What do you think about that ?

#20 @SteveAgl
15 years ago

They also said that the problme looks more related to the 777 permssion, on thoyr systems setting a dir to 777 make that dir unavailable for security reason.

#21 @SteveAgl
15 years ago

Another update, the hosting provider team worked hard all the dy to sort out the problem, here there is another message i got from them:

We have analyzed and solved the problem.

The first problem was in our system. The clena of the WP code made us to find a little problem in our suexec code that we have now corrected and now dir permission is now correct.

The second half of the problem is about the file permission for the files, and here we have found a little imperfection with WP code we have identified and corrected.

In file "admin-functions.php" at line 1775 we have correct as follow:

Set correct file permissions
$stat = stat(dirname($new_file));
$perms = $statmode? & 0000777;
@ chmod($new_file, $perms);

#### AFTER
Set correct file permissions
$stat = @ stat(dirname($new_file));
$perms = $statmode? & 0007777;
$perms = $perms & 0000666;
@ chmod($new_file, $perms);

PS Applying these 2 changes, the first one is already implemented at our systems, wordpress and the images upload works perfetctly.

I'm trying now the suggeste patch and testing it, if it's ok i'll submit a patch later

#22 @SteveAgl
15 years ago

Ok now it's all ok, the Changeset [3501] does what was expected, after they updated their suexec to have directory getting the right permission. I didn't have to apply the suggest modifications cause i supposed it was equivalents to yours as it was. They say thanks to WP to have show up this little problems that with others applicatons they had never encountered.

We can close this ticktes and consider the problem solved.

Thanks folks for the hard work :)

#23 @ryan
15 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#24 @davidhouse
15 years ago

  • Description modified (diff)

Never mind, I should have refreshed before posting that.

#25 @davidhouse
15 years ago

Err... I didn't just write that.

#26 @(none)
14 years ago

  • Milestone 2.0.1 deleted

Milestone 2.0.1 deleted

Note: See TracTickets for help on using tickets.