After upgrade to 4.3 unable to update plugins.

[jobst]

Hi.

After update of wordpress 4.3 I am unable to update plugins.
I receive the following message:

An error occurred while updating TinyMCE Advanced: The update cannot be installed because we will be unable to copy some files. This is usually due to inconsistent file permissions. 

I have not changed anything for months/years:

• no added/deleted plugins,
• no permission/user/daemon changes (for years)
• FS_METHOD ssh2 not changed (for years)

In fact I never had a problem updating my plugins/wordpress (including on all of my other wordpress sites, they are currently on 4.2.4) - I will not update those sites until I find out what the problem is.

The last update to plugins was done a few days back and succeeded (all of my sites).

So since I had no problems updating plugins after the last wordpress core update to 4.2.4 (which happens automatically in my case), so 4.3 IS the culprit.

If you need some debug info, tell me what to do and I will give you that info.

Jobst

[dd32]

This is caused by the is_writable() checks failing via SSH2 for some reason, I'm not sure why yet.

See #30921 for implementation and ​https://wordpress.org/support/topic/after-43-update-im-unable-to-update-plugins-or-themes/ for the many reports of this for the SSH2 extension.

It's likely for 4.3.1 we'll have to exclude SSH2 from checking is_writable() for the seemingly widespread failures (Although I still can't duplicate it on any of my test environments)

[jobst]

Can you give me some more info in which file is_writable() is located, please. Maybe some crude flow of the code (e.g. for a few functions calls BEFORE is_writable() is called) so I do not start completely blind ;-), thanks.

I will do some debugging on my system in a couple of hours (need to do some other stuff first but I will have a few hours spare later on today).

Replying to dd32:

This is caused by the is_writable() checks failing via SSH2 for some reason, I'm not sure why yet.

[dd32]

Replying to jobst:

Can you give me some more info in which file is_writable() is located, please.

Sure, The specific usage is triggered from here: WP_Upgrader::clear_destination() and the implementation is: WP_Filesystem_SSH2::is_writable()

A Simple way to test the is_writable() method will be to use something like the Debug Bar Console plugin, and running this code:

<?php

WP_Filesystem( array( 'hostname' => 'localhost', 'username' => 'my-user', 'password' => 'my-password' ) );

global $wp_filesystem;

var_dump( time(), $wp_filesystem->is_writable( ABSPATH ), $wp_filesystem->is_writable( WP_PLUGIN_DIR . '/akismet/akismet.php' ) );

[jobst]

Found it, it is a permission problem BUT it is an oversight of the person who wrote the changes of the update part of plugins for 4.3.

I first compared the two versions (4.2.4 and 4.3) to find out whether there are any differences at that part of the code - there are lots, the way a deletion is treated is very different:

4.2.4 uses $wp_filesystem->delete

while

4.3 uses $this->clear_destination()

$this->clear_destination() has a vital flaw, it assumes the file owner and group are the same, meaning the group user running apache and the user owning/editing the file are the same, e.g.:

-rw-r--r--  1 nobody nobody 17333 Apr 30 11:30 tinymce-advanced.php
-rw-r--r--  1 nobody nobody   535 Apr 30 11:31 uninstall.php
[root SOME_PATH/wp-content/plugins/tinymce-advanced] #>

That is totally insecure (especially in a world of CMS'es with badly written plugins/extensions), so my setup is:

-rw-r--r--  1 SOMEUSER nobody 17333 Apr 30 11:30 tinymce-advanced.php
-rw-r--r--  1 SOMEUSER nobody   535 Apr 30 11:31 uninstall.php
[root SOME_PATH/wp-content/plugins/tinymce-advanced] #>

In my case (and I am sure for many other secure/safety conscious admins) the user doing the FTP/SSH2 stuff is the same user that owns the file (above called SOMEUSER) NOT nobody (the user running apache).

The real issue is $this->clear_destination uses "is_writable/is_writeable" which is a PHP function running in the context of the user running apache (nobody in the case above), so obviously the files are NOT writeable and never should be!

The ONLY 2 directories open for the user running apache "nobody" should be "SOME_PATH/wp-content/uploads" and "SOME_PATH/wp-content/upgrade", which in my case both have the correct permissions.

Hope this helps.
Jobst

Replying to dd32:

Replying to jobst:

Can you give me some more info in which file is_writable() is located, please.

[dd32]

The real issue is $this->clear_destination uses "is_writable/is_writeable" which is a PHP function running in the context of the user running apache (nobody in the case above), so obviously the files are NOT writeable and never should be!

The SSH2 code uses the ssh2.sftp:// wrapper which directs the checks to happen on the remote filesystem via the SSH connection. PHP is then running the permission checks against SOMEUSER not nobody.

The problem here is that the ssh2.sftp:// wrapper appears to be malfunctioning and always returning false, probably due to stat failing over that link - most likely due to a bug in either libssh or the interpretation of the server permissions.

[jobst]

Sorry, I check with last information.
Jobst

Replying to dd32:

The real issue is $this->clear_destination uses "is_writable/is_writeable" which is a PHP function running in the context of the user running apache (nobody in the case above),

[jani-matti]

I'm getting the following during SSH2 plugin update on 4.3. Seems a bit funky:

PHP Warning: file_put_contents(): Unable to open ssh2.sftp://Resource id #308/usr/share/wordpress/.maintenance on remote host in /usr/share/wordpress/wp-admin/includes/class-wp-filesystem-ssh2.php on line 181

[jobst]

I first edited this and then deleted it in a hurry, I was correct.

It still is a permission problem:

I have a file /SOMEPATH/wp-content/plugins/index.php

• If I chmod that to 0640 I cannot write (below false)
• If I chmod that to 0660 I CAN write (below true)

 WP_Filesystem( array( 'hostname' => '127.0.0.1', 'username' => 'SOMEUSER', 'public_key' => '/PATH_TO_SOME_KEY.pub',  'private_key'=>'/PATH_TO_SOME_KEY.priv') );
  global $wp_filesystem;
  error_log(WP_PLUGIN_DIR . '/index.php');
  $rc = var_export($wp_filesystem);
  error_log($rc);
  $rc = $wp_filesystem->is_writable( WP_PLUGIN_DIR . '/index.php');
  error_log(">".$rc."<");

Gotta play some hockey, back in a couple of hours.

[dd32]

I've duplicated this, and it turns out it some of my assumptions around this are wrong, mostly due to some weird PHP behaviour.

This appears to come down to PHP's implementation of is_writable() not really being supported for stream wrappers - or rather, that the support is limited.

PHP will fetch the owner / permissions of the file over the SSH2 connection, but then base it's calculations of the current system user (nobody) rather than the ssh user in use for that is_writable() call.

It seems to me, that there's two ways to fix this.
a) Remove is_writable() support for SSH2 - It just plain doesn't work as expected.
b) Implement is_writable() support in PHP - this is a option, but isn't really a viable long-term solution.

For B, we'd have to evaluate the owner & permission values something similar to this: (And we don't know our groups, so can't evaluate those)
return $world_writable || ( $ssh_user == $file_owner && $writable_bits )

33480.diff​ implements option A.

[jobst]

That's what I wrote in my comment:5.
Jobst

Replying to dd32:

I've duplicated this, and it turns out it some of my assumptions around this are wrong, mostly due to some weird PHP behaviour.

[navjotjsingh]

Drupal faced the same issue with stream wrappers

​https://www.drupal.org/node/1333390
​https://www.drupal.org/node/944582

And they fixed it by writing their own implementation of is_writable() function.

[dd32]

Unfortunately Drupals use-case is slightly different, and doesn't quite work for us. They simply convert streams to real local paths, and then use the PHP functions on the direct paths (from what I can tell).

In some quick testing, I've found that simply using fopen( $file, 'a' ) doesn't work, as even though it'll return a resource, attempting to write to the file can still fail.

We can implement looking at the permission bits, but ACLs will get in the way, and we'll only ever know a "You might be able to write to this file" or "you probably can't write to this file" - neither are solid enough for our use-cases.

I'm just going to disable it for SSH, just like we've got it disabled for FTP.

[dd32]

In 33688:

WP_Filesystem: SSH2 handler: Remove support for is_writable() via SSH, it turns out PHP doesn't verify the writability via SFTP and instead uses a comparison based on the current php system process user instead of the ssh user.
This fixes the 'The update cannot be installed because we will be unable to copy some files.' error encountered during updates by skipping the write test completely.

Props jobst.
Fixes #33480 for trunk

[dd32]

re-opening for 4.3.1

[jobst]

I do not think it will be that tricky to "fix" this.
Note that I have "FS_METHOD" and everything else set in wp-config.php, so below functions all use "class-wp-filesystem-ssh2.php"

I can get the owner:

$rc = $wp_filesystem->owner( WP_PLUGIN_DIR . '/index.php');
error_log("OWN >".$rc."<");
// 2015-08-21 21:46:24 OWN >1234<

The user/owner of the connection is in the array:

error_log($wp_filesystem->options['username']);
// SOMEUSER

I can get the mode of the file as well:

$rc = $wp_filesystem->getchmod( WP_PLUGIN_DIR . '/index.php');
errorlog("CHMOD >".$rc."<");
// 2015-08-21 21:46:24 CHMOD >644<

The only thing I cannot find YET is how to connect the username "SOMEUSER" to the "UID 1234", I know you could use "php_posix" but not everyone would have that installed.

If saving the integer UID of the user opening the ssh2 connection into $wp_filesystem->options, you have a easy way to know that the file is WRITEABLE.

Jobst

[jobst]

I am trying to complete/fix the problem but I have discovered a small issue that may (or may not) needs fixing before continuing.
@dd32, can you check my question (below) please.

Assumption: The function "owner" in class "class-wp-filesystem-ssh2.php" on line 272 is partially wrong.

It gets the owneruid using

  @fileowner('ssh2.sftp://'.$this->sftp_link.'/'.ltrim($file, '/'));

which is correct but then it gets the username using

  $ownerarray = posix_getpwuid($owneruid);

which is trying to get a LOCAL username (if the UID exists!!) from a REMOTE UID!
Which is incorrect (misleading -> false info).

It should really do below to get the required (username) info:

 $username = $this->run_command(sprintf('stat -c \'%U\' %s', escapeshellarg($file)), true);
 return $username;

which will return the username immediately, bypassing the fileowner stuff but rather executing "stat" on the remote machine getting the correct info immediately.

So am I correct that the function "owner" has a bug?

Jobst

[dd32]

@jobst you're correct in that the fileowner may be different, it's assuming that the SSH server is the local server (which it should be in the majority of cases).

I'd like to avoid relying upon run_command() as it's not available for accounts which disallow shell access (For example, chrooted accounts).

WordPress also doesn't use the owner field, it's effectively wasting IO operations retrieving it for compatibility with the FTP return values, the same goes for all of these calls:

$struc['perms'] 	= $this->gethchmod($path.'/'.$entry);
$struc['permsn']	= $this->getnumchmodfromh($struc['perms']);
$struc['group']    	= $this->group($path.'/'.$entry);
$struc['size']    	= $this->size($path.'/'.$entry);
$struc['lastmodunix']= $this->mtime($path.'/'.$entry);

As far as I'm concerned, removing is_writable() support from ssh is acceptable here, we already don't implement it on FTP for the reasons of not being able to effectively determine it.

[jobst]

Ok, seems reasonable - why did you reopen it?

Replying to dd32:

@jobst you're correct in that the fileowner may be different, it's assuming that the SSH server is the local server (which it should be in the majority of cases).

[dd32]

Ok, seems reasonable - why did you reopen it?

The fix needs to go into 4.3.1, it's currently only in trunk.

[dd32]

In 33883:

WP_Filesystem: SSH2 handler: Remove support for is_writable() via SSH, it turns out PHP doesn't verify the writability via SFTP and instead uses a comparison based on the current php system process user instead of the ssh user.
This fixes the 'The update cannot be installed because we will be unable to copy some files.' error encountered during updates by skipping the write test completely.

Merges [33688] to the 4.3 branch.
Props jobst.
Fixes #33480 for 4.3