Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#8210 closed task (blessed) (fixed)

SSH2 Filesystem transport; Multiple issues

Reported by: DD32 Owned by:
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.7
Component: Upgrade/Install Keywords: has-patch needs-testing tested
Focuses: Cc:


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.

Attachments (4)

8210.diff (4.6 KB) - added by DD32 7 years ago.
ssh2.dash_and_shell.patch (3.7 KB) - added by t.baboon 7 years ago.
8210-ssh-keys-test.diff (4.3 KB) - added by DD32 7 years ago.
8210.2.diff (21.2 KB) - added by DD32 7 years ago.

Download all attachments as: .zip

Change History (15)

7 years ago

#1 @DD32
7 years ago

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

#2 @ShaneF
7 years ago

  • Component changed from General to Upgrade
  • Keywords has-patch commit added
  • Milestone changed from 2.8 to 2.7
  • Owner anonymous deleted

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.

#3 @ryan
7 years ago

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.

#4 @DD32
7 years ago

  • Keywords has-patch commit removed
  • Milestone changed from 2.7 to 2.8

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.

#5 @DD32
7 years ago

  • Keywords needs-patch added

#6 @t.baboon
7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

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

7 years ago

#7 @DD32
7 years ago

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.

#8 @ryan
7 years ago

  • Type changed from defect (bug) to task (blessed)

#9 @DD32
7 years ago

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.

#10 @jdingman
7 years ago

  • Keywords tested added

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

#11 @ryan
7 years ago

  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.