SSH2 Filesystem transport; Multiple issues

[DD32]

The SSH2 filesystem transport appears to have a few issues:

• Filenames with multiple dashes cannot be created
  • eg: cforms has a file -----HISTORY.txt which cannot be created
• Filenames are not properly escaped before being escaped
  • eg: run_command($this->link, sprintf('ls -lad %s', $file)); instead of say run_command($this->link, sprintf('ls -lad "%s"', $file)); or better: run_command($this->link, sprintf('ls -lad "%s"', escapeshellarg($file) ) );
  • escapeshellarg() or one unique to the SSH2 transport should be used on such files
• While not specifically a defect, using @fopen('ssh2.sftp://' instead of ssh2_scp_recv() can be much faster according to the PHP docs, It also avoids having to use a temporary file, as you can read it straight into a variable.

I'm going to attach a patch thats a bit of POC, It doesnt "fix" anything mentioned here, just a start towards it, and highlights the areas which need attention.

[DD32]

Filenames with multiple dashes cannot be created

It doesnt appear to be related to escaping either

Filenames are not properly escaped before being escaped

..Should've read: Filenames are not properly escaped before being used

[ShaneF]

Patch looks good enough for 2.7. Werid files do cause error in SSH as "-" and other characters are usually escape things when processed by the shell.

[ryan]

Patch needs to have the if true blocks removed if we want this in 2.7. If it's not actively fixing a bug, however, it should move to 2.8.

[DD32]

The patch was never designed to be commited, Mearly to point out the issues.

I have no intention of finishing off the patch since i cant test it (SSH2 simply too slow for anything), But the possible security issues are there, and need to be delt with along with using the fopen() routine possibly speeding it up.

Bumping to 2.8 unless someone else steps in and cleans it up.

[t.baboon]

My attached patch seems to fix problems #1 and #2 above (dashes in the beginning of filenames and special characters in filenames causing the potential execution of shell commands), while not addressing problem #3 (moving to "ssh2.sftp://"). It uses escapeshellarg() on all paths and adds the string "./" in front of all relative paths or filenames (ones that don't start with a slash), which should fix #1 and #2.

I've not tested it extensively, so some testing assistance would be appreciated.

-- t.baboon

[DD32]

attachment 8210.2.diff added

• Needs Testing
• HUGE performance boost for me,
  • Moved to ssh2.sftp:// wrapper
  • Moved to Streams support for things which couldnt use that wrapper
• Escapes the shell args, Havnt tested with the problematic filenames such as -----HISTORY.txt - Testers please :)
• Fixes the SSH2 Keys problems
  • When using Keys, a Username/Password is not required, However, a Password(Pasphrase) may be provided.
• Set the default for credentials option to have a blank hostname/username, prevents random notice errors..
• All other patches on this ticket may be ignored and considered stale.

My testing has been limited to Plugin Upgrades/Installs, both of which are running pretty nicely.

[DD32]

See also: #8331, specifically ​comment 13 onwards

The patch on this ticket (8210.2.diff) significantly reduces the load on run_command / exec(), so many issues related to not checking the return data from commands are dealt with by PHP's builtin filesystem commands since its running via PHP's Wrappers.

[jdingman]

tested it.

applied the patch, tested the upgrade using wp-config options of FTP_PRIKEY, FTP_PUBKEY, FTP_HOST, and FTP_USER

automattic upgrading works for me

[ryan]

(In [11063]) SS2 FS fixes. Props DD32. fixes #8210