Make WordPress Core

Opened 16 years ago

Closed 15 years ago

#8210 closed task (blessed) (fixed)

SSH2 Filesystem transport; Multiple issues

Reported by: dd32's profile 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 16 years ago.
ssh2.dash_and_shell.patch (3.7 KB) - added by t.baboon 15 years ago.
8210-ssh-keys-test.diff (4.3 KB) - added by DD32 15 years ago.
8210.2.diff (21.2 KB) - added by DD32 15 years ago.

Download all attachments as: .zip

Change History (15)

16 years ago

#1 @DD32
16 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
16 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
16 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
16 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
16 years ago

  • Keywords needs-patch added

#6 @t.baboon
15 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

15 years ago

#7 @DD32
15 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
15 years ago

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

#9 @DD32
15 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
15 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
15 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.