WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 6 months ago

#10205 reopened enhancement

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

Reported by: pgl Owned by: dd32
Milestone: Future Release Priority: normal
Severity: normal Version: 2.8
Component: Filesystem API Keywords: has-patch dev-feedback
Focuses: Cc:

Description

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

Attachments (4)

file.php.15646.diff (1.2 KB) - added by joelhardi 4 years ago.
10205.diff (1.8 KB) - added by dd32 9 months ago.
Add descriptive checks to prevent confusion over intended user_id values
10205.2.diff (5.4 KB) - added by dd32 7 months ago.
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.
10205.3.diff (4.5 KB) - added by dd32 7 months ago.
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.

Download all attachments as: .zip

Change History (39)

comment:1 Denis-de-Bernardy5 years ago

  • Component changed from Administration to Filesystem
  • Milestone changed from Unassigned to 2.9
  • Owner set to dd32

comment:2 dd325 years ago

  • Milestone 2.9 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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)

comment:3 dd325 years ago

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.

comment:4 joelhardi4 years ago

  • Cc joel@… added
  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.

joelhardi4 years ago

comment:5 joelhardi4 years ago

  • Keywords has-patch dev-feedback added; uid filesystem method file.php posix_getuid getmyuid direct install upgrade removed

comment:6 scribu4 years ago

The patch looks good.

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

comment:7 scribu4 years ago

  • Milestone set to 3.1

comment:8 joelhardi4 years ago

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

comment:9 follow-up: nacin4 years ago

  • Milestone changed from 3.1 to Awaiting Review

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.

comment:10 in reply to: ↑ 9 joelhardi4 years ago

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.

comment:11 nacin3 years ago

  • Type changed from defect (bug) to enhancement

comment:12 dd323 years ago

  • Milestone changed from Awaiting Review to Future Release

Will review these in 3.2.

comment:13 Aikar3 years ago

  • Cc Aikar added
  • Type changed from enhancement to defect (bug)
  • Version changed from 2.8 to 3.0

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.

comment:14 nacin3 years ago

  • Type changed from defect (bug) to enhancement
  • Version changed from 3.0 to 2.8

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

comment:15 Aikar3 years ago

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

comment:16 Aikar3 years ago

  • Cc Aikar removed

comment:17 Aikar3 years ago

  • Cc aikar@… added

comment:18 dd323 years ago

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.

Last edited 3 years ago by dd32 (previous) (diff)

comment:19 till3 years ago

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?

comment:20 SergeyBiryukov12 months ago

#24007 was marked as a duplicate.

comment:21 thanatica212 months ago

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.

Last edited 12 months ago by thanatica2 (previous) (diff)

comment:22 rpostgate12 months ago

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.

comment:23 Dennison Williams9 months ago

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

comment:24 ocean909 months ago

#24700 was marked as a duplicate.

comment:25 thanatica29 months ago

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

comment:26 pgl9 months ago

@thanatica2: Make that 4 years.

dd329 months ago

Add descriptive checks to prevent confusion over intended user_id values

comment:27 thanatica29 months ago

I stand corrected.

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

One friggin line. Honestly.

comment:28 nacin8 months ago

  • Milestone changed from Future Release to 3.7

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.

comment:29 mattrude8 months ago

  • Cc matt@… added

comment:30 dd327 months ago

#24107 was marked as a duplicate.

dd327 months ago

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.

dd327 months ago

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.

comment:31 dd327 months ago

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.

Last edited 7 months ago by dd32 (previous) (diff)

comment:32 nacin7 months ago

  • Type changed from enhancement to task (blessed)

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.

comment:33 dd327 months ago

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.

comment:34 thanatica27 months ago

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?

Last edited 7 months ago by thanatica2 (previous) (diff)

comment:35 nacin6 months ago

  • Milestone changed from 3.7 to Future Release
  • Type changed from task (blessed) to enhancement

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

Note: See TracTickets for help on using tickets.