getmyuid() called instead of posix_getuid() in get_filesystem_method() (wp-admin/includes/file.php)

[pgl]

In wp-admin/includes/file.php, the function get_filesystem_method() attempts to figure out whether it is able to write files correctly, and therefore whether it can update or install files directly, or needs to use some other method.

As part of the function, in a particular case it writes a temporary file and compares it to the return value of getmyuid(). I think this is a mistake - the return value of getmyuid() is the owner of the current _file_ that's being run, not the current process - so if the file is owned by a user other than that of the web server's UID, it thinks it can't install directly (even if it actually can, because the directories are group writable).

This can be worked around by simply changing the owner of the file to another user, although this isn't always going to be possible for the person running Wordpress.

To fix this, change the function call to check the return value of posix_getuid() instead of getmyuid(). (NB: this function isn't available on Windows.)

[dd32]

See #8400, #7779

Group-writable is not supported currently.

The owner on new files can never be set correctly, so it'll always be owned by the web server (ie. apache) instead of by the current user

Thats why it checks the owner of the current file, compared to the owner of created files.

I'm closing as wontfix, due to the ownership issues. It could cause huge problem on some shared hosters (most of the major ones at least)

[dd32]

also, as of 2.8, theres a new constant, 'FS_METHOD' which you can define as 'direct' to force it to use direct access, no need for using of the filter.

[joelhardi]

Forgive me for reopening an old bug on an issue that has many duplicate bugs. But, I have a patch.

After banging my head about this bug (constantly getting the FTP "Connection Information" box) no matter what I chown or chmod directories to, I now understand the problem. And, I see that there is the FS_METHOD constant that I can define to work around it. That's good.

But, I still think the default behavior can be improved. As I understand it, the issues are these:

1. WordPress is going to write files as the webserver/cgi process, i.e. www. This is posix_getuid()
2. The PHP files themselves (i.e. wp-admin/update.php for updating a plugin) could be owned by anyone, as long as they're readable by the webserver. Their owner is getmyuid(). Generally, you do not want them to be owned by the webserver/cgi process for security reasons, unless you're auto-upgrading WordPress.
3. WordPress will only let you add or update a plugin if getmyuid() and posix_getuid() are the same (i.e. if everything is owned by www or a suEXEC Apache).

So, for instance in my case, all the files are owned by johndoe, except for a couple of directories like wp-content/uploads that I chown to www so that WordPress can put images etc. there.

If I try to upload a plugin, it fails because getmyuid() returns johndoe, but the webserver creates files as www. The actual fix would be for me to chmod wp-admin/update.php (I think) to www so that getmyuid() will return www. The problems are: I don't want to change my file ownerships this way, it's a security problem; this is very obscure and most people can't be tracing code and chowning files every time WordPress refuses to do something; and WordPress doesn't tell me any of this, it just presents this box asking for FTP credentials that made no sense to me at all, which looks like a mistake. FTP??? I don't even have an FTP daemon running and never told WordPress anything about FTP.

The first time I had saw this FTP box pop up was when I was trying to delete a plugin (testing to see if my uninstall hook worked), which made the FTP box seem even more bizarre.

I understand the file deletion problem on shared hosts -- you don't want WordPress to create files that users then can't delete. Then, these users go "WordPress is broken!" and it creates many support headaches (of course, these users can always use WordPress to delete these files as the www user). Fair enough. Obviously, posix_getuid() is also not available on Windows.

Personally, I think all WordPress should be doing is "Can I write to the filesystem? OK, then I will write to the filesystem," like it does when you upload an image. I should be able to run my daemons and chown my files the way I want! I would point out that WordPress is already creating files owned by the webserver/cgi user (uploaded images and other media), and argue that it's inevitable. The thing to do is not allow it in one case and silently block it in another, but to gently guide the user around these issues. But, I understand that in this case (maybe because people upgrade plugins more often than they delete images) someone had a different design goal.

I don't suggest there's a way to magically solve all these issues, only that there's room for improvement. With that said, I'd suggest one of:

1. My patch. It just checks whether posix_getuid() is available and, if so, allows install to proceed if either getmyuid() or posix_getuid() matches the temp file owner. To me this is normal, the user is attempting a direct file upload, so allow it to proceed if there's no reason why it shouldn't. Unlike the other patches I've seen, it's not swapping out getmyuid for posix_getuid, it's allowing either one to match.
2. An "Are you sure?" confirmation step or informational message after activation. i.e. if getmyuid() and posix_getuid() do not match, say "Uploaded files will be/have been created as the user [uid], so to delete them later you will have to use the WordPress uninstall feature (unless your FTP or shell user account has write access to [uid]'s files)."
3. Minimally, an error message explaining the situation and reason for the FTP box. As I said, just popping up the "FTP Connections" box made no sense to me, and judging by Google I'm not the only one. And I don't have or want FTP, so it's a dead end. Something like:

Sorry, you cannot upload files directly to WordPress because this script 
(update.php) is owned by [uid], but new files created by WordPress are 
owned by [uid]. To fix this problem you can do either of the following:

 * Allow WordPress to create files as [uid] by defining FS_METHOD in 
   wp-config. (with link to wiki page explaining "direct" etc.)

 * Use FTP to upload the files to WordPress (with link)

I'm happy to contribute code toward whatever solution.

[scribu]

The patch looks good.

Don't get your hopes up for 2. or 3. though; that's way too technical for most users.

[joelhardi]

I'll be happy with the patch, I think that solves the problem. 2 & 3 were my backup plan.

[nacin]

Closed #15021.

See also #14753. dd32's reply there is why I am pulling this off 3.1. He can go into more detail here I'm sure.

[joelhardi]

See also #14753. dd32's reply there is why I am pulling this off 3.1. He can go into more detail here I'm sure.

I fully understand the reasoning behind why things currently work the way they do, I just think that it's a band-aid that has side effects that are worse than the problem it's trying to address. I'd also argue that problem is overblown at best and certainly doesn't exist at most large shared hosting companies who have configured UNIX permissions correctly.

Forcing users to chown their WordPress scripts to the webserver user is a sysadmin headache, and a security problem -- you're giving PHP scripts the ability to write to themselves and potentially do code injection. The ​workaround is ugly. Defining FS_METHOD is better, but to me it's still a hack. I'd be willing to consider it an "acceptable" hack only if the UI here weren't so opaque (how are users supposed to know they have to add this constant?).

I'll go over why I think the getmyuid('options.php') check needs to go ...

First, this isn't consistent with how the rest of WordPress works. i.e. when you upload an image or other media, WordPress only checks to see whether it can write to the filesystem. That's it. If this were a real problem, then unsophisticated users not being able to move or delete their webserver user-owned image files would be a much bigger one. It would be one thing if WordPress did this getmyuid() check whenever it creates any file, but in fact this extra requirement (that all WordPress files have to be owned by the same user, who also must be the webserver user) is only being enforced here.

Big shared hosting companies don't run Apache mod_php with webserver user privs any more, exactly because of these sorts of user support file ownership issues. They've learned it's easier to use suEXEC or run PHP via cgi and just have everything in a customer's user dir owned by that user. Each customer has their own user/group. So, the getmyuid() check solves a problem that I would argue doesn't really exist.

Hosts have already addressed this sort of issue where it should be addressed (on the sysadmin side) -- I'd use the analogy of OSI layers here and argue that at WordPress' level (PHP scripting), it shouldn't be forcing people to chown or chgrp their files to any specific user or group. It should be totally agnostic to UNIX users/groups and just care whether it can write to the files or not.

Finally, WordPress can always be used to delete plugins it has installed or images it has uploaded ... the underlying issue (unsophisticated user says "I can't delete my own files!") can also be solved by just having them click "delete" in WordPress.

[dd32]

Will review these in 3.2.

[Aikar]

the line "if ( getmyuid() == @fileowner($temp_file_name) )" really just needs to be deleted.

the line above it opens a WRITE handle to the target directory, and returns true if the user the script is running as has write permissions to that directory.

If the user has write permissions, thats all wordpress should care about.

In my case I have my files setup with Access Lists, and the user has permissions (var_dump(is_writable('wp-conttent')) returns true), but that single line is turning a success condition into a failure.

We need to simply delete that line, and possibly use is_writable available for cleaner code.

Marking this as bug since my WordPress is not functioning as its expected to due to this.

[nacin]

It's not a bug. It's by design. Any improvements on it are by definition an enhancement.

[Aikar]

also, I understand wordpress is trying to display the FTP login method instead of direct under any possible condition for failure, but to disconvenience users who have a more proper server setup is not a good idea.

My server permissions are setup so that the web user will always have access to files, and multiple users can edit files over SSH/SFTP, so the files can be owned by diff people.

If the user has write permissions to a directory, it doesnt matter if they are the owner or not.

checking the owner does not change the fact of do they have permissions or not.

they could have owner on wp-content and not on wp-content/themes and pass the check, but it doesnt change anything.

Simply removing that line and use fs method = direct if the user has write permissions will be more flexible.

This current line says "hey, you have write permissions, but im going to fail you anyways". it doesnt make sense to me. PHP has full write access to a directory yet WP is denying it.

I dont like the idea of having to tell everyone i host websites for they have to add a line to wp-config.php in order to use any auto update feature (I dont have FTP on my server anyways...)

If you really want to be safe, wordpress could simply scan the entire directory tree and ensure write permissions to all directories it needs access to before attempting install, then prompt FTP if it had issues in any directory, and alert user why its asking for FTP instead of just installing.

We dont need any of this posix_getuid or getmyid() stuff

[dd32]

This current line says "hey, you have write permissions, but im going to fail you anyways". it doesnt make sense to me. PHP has full write access to a directory yet WP is denying it.

Thats exactly it. PHP has full write access to the directoryy/files.

Now, Your server might be setup correctly to allow this to work adequately and if that's the case, i encourage you to add define('FS_METHOD', 'direct'); to your wp-config.php file.

However, In a large number of shared hosts, and a large number of servers, the server will not be setup like yours. The files will be writable by apache/php, however files created by apache will not be readable/modifiable by FTP users, or, files created by apache, will be ultimately writable to all users on the server (As every instance of PHP runs as the same user with no security)

It's currently setup to protect users from their shitty hosts. you might not have this problem, a lot of the major us hosts may not have this problem, but a tonne of shared hosts out there DO have this problem. When you've got over 30 million installs around the globe, there's a lot of configurations you have to work around, in this case, selecting the lowest common denominator which is guaranteed to work is current choice.

[till]

I agree with the previous poster while I understand that this is very technical, etc..

I ran into the same issue for the sake of testing, I even set 0777 (yes, I know it's insecure), but the test inside this function still failed. define('FS_METHOD', 'direct'); was my only possible work-around.

Generally, I'm also wondering why a temporary file needs to be created when a is_writable() should suffice?

[SergeyBiryukov]

#24007 was marked as a duplicate.

[thanatica2]

It's not a bug. It's by design. Any improvements on it are by definition an enhancement.

Nice one, you should become a project manager. It is a bug, because it's not working. If it's a bug or a buggy design is not important to neither the users nor the developers. It just needs fixing.

Simply delete this line:

if ( getmyuid() == @fileowner($temp_file_name) )

And it's FIXED. It doesn't take two years to do that.

Like other people have stated, WP only needs write access. Becoming file owner is simply not neccesary. If it has checked for create/write/delete access, than that's all it needs to carry on.

What's more, it fails silently. The user is presented with a form that asks for FTP login info. WHY? At least report WHY auto-updates are failing. Silent fail is ALWAYS bad programming.

And sorry if this reads a little cranky. But come on, two years.

[rpostgate]

I ran in to this problem also. All my fiddling with ownership of the relevant php files didn't seem to affect the outcome, so I decided to patch wp-admin/includes/file.php get_filesystem_method() to add the following check (the test write string isn't strictly necessary, but, what the heck, may as well be paranoid):

                if ( $temp_handle ) {
                        if ( getmyuid() == @fileowner($temp_file_name) )
                                $method = 'direct';
+                       @fputs($temp_handle, "test");
+                       @fclose($temp_handle);
+                       $temp_handle = @fopen($temp_file_name, "r");
+                       if ($temp_handle) {
+                               if (@fgets($temp_handle) == "test") {
+                                       $method = 'direct';
+                               }
+                       }
                        @fclose($temp_handle);
                        @unlink($temp_file_name);
                }

This way the original behavior is unchanged, but if an actual write to the test file is confirmed successful, direct file access is enabled.

[Dennison Williams]

Depending on wp-admin/{plugins,themes,update}.php to be the same uid as the web servers process is security theater, as the user can already get files on the server if they have wordpress admin privs, this just prevents user functionality. If the user can get php code in the server they can backdoor the site via existing themes owned by the webserver, but because of this "feature" they could also backdoor those 3 files (assuming that it is owned by the webserver _and_ writeable by the webserver). Not to mention this "feature" is in conflict with the FS security documentation (​http://codex.wordpress.org/Hardening_WordPress#File_Permissions).

+1 for using the posix_getuid() method

[ocean90]

#24700 was marked as a duplicate.

[thanatica2]

Honestly, how is this still going on?

@rpostgate, I don't mean to be rude to you, but your code adds even more functionality. What needs to happen is to remove the silly check for fileowner. Your code would indeed fix the problem to a user's perspective, but really what's happening is more checks and more things to slow the system down (as if wordpress would need more cpu-cycles to eat up) and are just so pointless.

Earlier I said "2 years", but we're coming close to another 3 months.

It doesn't take more than five minutes to remove one flipping line of code. How is this still going on then??

[pgl]

@thanatica2: Make that 4 years.

[dd32]

Add descriptive checks to prevent confusion over intended user_id values

[thanatica2]

I stand corrected.

But it "kind of" indicates the urgency of this silly little bug, doesn't it?

One friggin line. Honestly.

[nacin]

Paraphrasing a conversation from dd32:

• Updating existing files with new content is easy and would work fine. Creating new files owned as apache:www instead of owner:www is problematic for some hosts, but, this is the same as what our uploads currently are. The original issues may not be as big of a deal on hosts now.

Moving to 3.7 for consideration.

[dd32]

#24107 was marked as a duplicate.

[dd32]

Allow Partial core upgrades to make use of Direct when it wouldn't normally. Core Partial upgrades do not create new files, and thus, do not need as stringent tests.

[dd32]

Patch that checks the file ownership, and writability of files during a FTP Upgrade, allowing for Direct upgrades to be used when the system has previously determined (by FTP) that it's going to be OK. Mostly for cases where fileowner() and getmyuid() are not available.

[dd32]

Attachment attachment:10205.2.diff​​ added

• This attachment enables Partial core updates to use the Direct FS handler when they would otherwise use FTP. This is a safe operation, as partial builds will not include new files, so this is just updating the contents of some files.
• There is the downside that if ABSPATH and the current file IS writable, but some of the other core files which need to be modified are not writable, the upgrade will fail.
• TODO: There should also be a check for && empty( $api->added_files ) in addition to checking it's a partial build in use, so as to avoid cases in the future where a partial build will be forced to include extra files.

Attachment attachment:10205.3.diff​​ added

• This does effectively the same as we currently do in PHP to determine if we can use the Direct FS handler, except, it verifies it over FTP post-upgrade and stores the result in an option.
• Verifies that files can be read/written to by both FTP and via local file operations
• Doesn't handle the case where a database is moved to a new host, or, the server configuration changes making Direct upgrades impossible (This would prevent the user from installing, or upgrading, themes, plugins, and core)
• TODO: Option should be cleared in the event of a upgrade failure, so as to prevent issues configuration changes, We could potentially also include things such as ABSPATH or md5( ABSPATH . DB_NAME . DB_USER . DB_HOST ) or similar to invalidate the option after a hosting move

Both of these patches can work side-by-side, and will reduce friction for upgrades (in particular, auto-upgrades), attachment:10205.2.diff​ is reasonably safe, attachment:10205.3.diff​​ not so much for server configuration changes issues.

[nacin]

It looks like we're going to do 10205.2.diff​ plus the TODO (check the API to make sure the partial doesn't contain new files) for 3.7.

[dd32]

Worth noting here, that attachment:10205.2.diff​​ is only safe for Core updates, and Language Pack updates (but not new LP installs). Themes/Plugins/LP Installs all create new files which isn't the intended behaviour here.

[thanatica2]

I have a sneaking suspicion that diff is not going to solve the problem.

ALL that needs to be done to solve the problem is to remove the check for file ownership. That is litterally all.
As quoted 5 months ago: ​http://core.trac.wordpress.org/ticket/10205#comment:21

Just one more thing: why does it take so freakishly long to get this through?

[nacin]

We're *not* simply removing the check for file ownership. The prospect of having us create a file that the FTP user can't edit is very bad.

Some stats we've collected show that "direct" is used almost everywhere, while removing the ownership check would only mean "direct" support would be chosen in a small minority of cases where "direct" is not already used.

So, we're not doing 10205.2.diff​ for 3.7. It needs some refactoring and a proper soak. We could also consider .3.diff (which would in practice be an effective equivalent to "removing the check for file ownership").

[SergeyBiryukov]

#28034 was marked as a duplicate.

[jason_the_adams]

I definitely second this issue. What I find interesting about this situation is that it seems to assume the user has rights to set the ownership, applying different user/group ownership, and then somehow loses the know-how a few moments later when they ftp in. I'm just trying to imagine an actual scenario in which some consistent person would run into this problem.

I don't mind checking the reason behind why WP has write access, but suggest, at the very least, comparing the gid of the temp file and user.

What's interesting further still is that if the uid of the file and active user are different, then it implies the temp file inherited the uid properly, which would be by the design of the user. I suspect the vast majority of the cases for WP are either something like Wordpress.com, wherein the service manages everything, or shared hosting, wherein the user doesn't have rights to change the ownership. If they are, it seems safe to assume they know what they're doing anyway.

Hahah! Sorry for the passion; I've been struggling with this for months now. I use Capistrano and this issues really causes problems with deployment methods. I'd prefer to use an ACL or something like that with WP. The suggested permissions are a bit more open than I'm comfortable with.

[obenbeimutti]

Yeah, very annoying bug. I only registered to create a ticket for this bug.

Obviously the file ownership test is completly wrong. The only thing that matters is, that you can write on that folder.

"We're *not* simply removing the check for file ownership. The prospect of having us create a file that the FTP user can't edit is very bad."

Yeah that is very bad, but it is not your responsibility. The person who sets up the ftp server should handle that problem, which indeed is not really a problem.

By the way, who uses FTP? Security issues! Heard about SFTP?

When it comes to deployment it seems, wordpress is lightyears behind technology. Absolute urls written into the database, is another huge problem. No professional developer uses such a technique.

[dd32]

For owner-related things, see #10424 and the numberous others mentioned here.

For group-writable when possible, See #30245

[dd32]

In 30384:

Background Updates: Introduce support to take advantage of Group Writable (or World Writable) to Core Background updates.
This is only enabled when new files will not be installed during the update (as indicated by the WordPress.org API), and does not apply to Plugin/Theme/Translation Background Updates.

Additionally, the code to determine if the 'direct' filesystem transport should be used has been tweaked for wider support (where getmyuid() was unavailalbe) which fixes #10424

See #10205, #30245