Make WordPress Core

Opened 15 years ago

Closed 15 years ago

#10423 closed defect (bug) (duplicate)

get_filesystem_method() uses uses wrong owner for validation

Reported by: cyberspice's profile cyberspice Owned by: dd32's profile dd32
Milestone: Priority: normal
Severity: normal Version: 2.8.1
Component: Upgrade/Install Keywords: Upgrade close
Focuses: Cc:

Description

The function get_filesystem_method() creates a temporary file and then checks the ownership comparing it with the result of getmyuid(). getmyuid() does not return the owner of the web server process but of the file calling getmyid(). In this case wp-admin/includes/file.php. This means that in order to support automatic update the Wordpress files have to be own by the same process as the webserver regardless of permissions on the files. This is a potential security risk.

If posix_getiud() is used where available then the owner of the webserver process is compared to the ownership of the temp file. The Wordpress files can be owned by someone else and the update system works as long as the webserver has permissions to write.

I have written more about this (together with a fix) in my bog.

http://www.cyberspice.org.uk/blog/2009/07/15/wordpress-automatic-update-bug/

Change History (6)

#1 @dd32
15 years ago

  • Keywords close added

This is a potential security risk.

Can you explain that a bit further before i just close this as duplicate and refer you to the previous tickets/explanations?

The Wordpress files can be owned by someone else and the update system works as long as the webserver has permissions to write.

If that was the intended direction, I'd have just used an is_writable() call.

The intended aim of that code, Is that if the owner of the Created files, Differs from that of the currently running process then to NOT use the direct setup, due to the created files being owned by someone other than the actual user.

Another way that code could be written which may work on more hosts is:

if ( file_owner($temp_file) !== false && file_owner(__FILE__) === file_owner($temp_file) ) {
//use direct
}

but that would NOT cause the direct setup to be used in your situation either..

the usage of getmyuid() was used to replace fileowner(__FILE__) basically.

Still tempted to close as duplicate, but might be worth changing the code, or at least adding a comment that its the intended code..

#3 @Denis-de-Bernardy
15 years ago

  • Milestone Unassigned deleted
  • Resolution set to duplicate
  • Status changed from new to closed

#4 @Denis-de-Bernardy
15 years ago

maybe we could add a filter, though. you know... so that users who want to check this differently can do so.

#5 @dd32
15 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

maybe we could add a filter, though. you know... so that users who want to check this differently can do so

Then they can use the filter already present, or force it through the constant. Please stop closing things so soon.

Re-opening pending close due to a commit to add some documentation or coding it differently to show that its not a typo.

#6 @dd32
15 years ago

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

ok, i'll make a new ticket for that instead..

Note: See TracTickets for help on using tickets.