Permalink
Browse files

replace :sanitize_file_name with a call to String#shellescape

  • Loading branch information...
guns authored and net-ssh committed Aug 16, 2010
1 parent 6263198 commit 91e0fa6a332511c50550e277c22f810481f55db0
Showing with 2 additions and 5 deletions.
  1. +2 −5 lib/net/scp.rb
@@ -1,4 +1,5 @@
require 'stringio'
require 'shellwords'

require 'net/ssh'
require 'net/scp/errors'
@@ -337,7 +338,7 @@ def scp_command(mode, options)
# (See Net::SCP::Upload and Net::SCP::Download).
def start_command(mode, local, remote, options={}, &callback)
session.open_channel do |channel|
command = "#{scp_command(mode, options)} #{sanitize_file_name(remote)}"
command = "#{scp_command(mode, options)} #{remote.shellescape}"
channel.exec(command) do |ch, success|
if success
channel[:local ] = local
@@ -398,10 +399,6 @@ def finish_state(channel)
def progress_callback(channel, name, sent, total)
channel[:callback].call(channel, name, sent, total) if channel[:callback]
end

def sanitize_file_name(file_name)
file_name.gsub(/[ ]/) { |m| "\\#{m}" }
end
end
end

16 comments on commit 91e0fa6

@timcharper

This comment has been minimized.

Copy link

timcharper replied Sep 8, 2010

Not that this is a huge issue, but this patch breaks compatibility with Ruby 1.8.6. Specifically, Ruby 1.8.6 doesn't provide String#shellescape.

In light of this, it seems appropriate to output a warning if a user tries to use the new version of the gem with Ruby 1.8.6, and the last compatible version might be listed in the README.

@delano

This comment has been minimized.

Copy link
Collaborator

delano replied Sep 8, 2010

Thanks for letting me know (I've been testing with 1.8.7+).

I'll fix it shortly.

@timcharper

This comment has been minimized.

Copy link

timcharper replied Sep 8, 2010

I don't blame you :-) it's probably time that we all moved on.For heaven's sakes, Ruby 1.9.2 is out! And it's awesome!

@guns

This comment has been minimized.

Copy link

guns replied Sep 8, 2010

Well, the shellwords library is < 150 lines of code. It might be worth extracting Shellwords as a rubygem or just vendoring the thing for < 1.8.6.

I was told by someone in #ruby that Shellwords was an old library, so I felt free to use it. I should have checked it out for myself before I sent in the patch though.

IMHO shellwords is a really underloved library in the Ruby world. I keep seeing people doing system "#{foo} #{bar}" and I get queasy flashbacks to quoting issues in old shell scripts.

@timcharper

This comment has been minimized.

Copy link

timcharper replied Sep 8, 2010

it is an old library :-) however, apparently it's been "upgraded"

I always pass shell arguments implicitly, such as system(foo, bar)

@guns

This comment has been minimized.

Copy link

guns replied Sep 8, 2010

Oh it is an old library? I see, it's time to grep the changelog :)

Multi-arguments to system, exec, and spawn work well (and should be used by more rubyists), but the backtick method has no analogue. So with that I have found %x(#{argv.shelljoin}) useful.

@guns

This comment has been minimized.

Copy link

guns replied Sep 8, 2010

Okay, so from as far back as v1.8.0:

# Examples:
#
#   require 'shellwords'
#   words = Shellwords.shellwords(line)
#
# or
#
#   require 'shellwords'
#   include Shellwords
#   words = shellwords(line)
#

Delano: Would you like me to submit a patch for compat with < 1.8.7?

@delano

This comment has been minimized.

Copy link
Collaborator

delano replied Sep 8, 2010

That would be great!

@guns

This comment has been minimized.

Copy link

guns replied Sep 8, 2010

Okay, no prob, I'll have a patch in by tomorrow. Do you just want compat with 1.8.6, or would like it all the way back to a pre-1.8 series?

@delano

This comment has been minimized.

Copy link
Collaborator

delano replied Sep 8, 2010

Just 1.8.6 and I'll add a note to the README for earlier versions as Tim suggested.

By the way, I'm traveling Thursday to Monday without my laptop (!). I'll create a release in the morning if the patch is ready, otherwise it'll be Monday evening. (It's not high priority so don't rush.)

@delano

This comment has been minimized.

Copy link
Collaborator

delano replied Sep 16, 2010

I just pulled in the compatibility fix from guns and created the 1.0.4 release. It will be available on rubygems.org shortly.

Thanks guys.

@bradland

This comment has been minimized.

Copy link

bradland replied Sep 16, 2011

The use of shellescape here (L341 in this commit) breaks many things that would execute normally when used with scp. For example, if you attempt to use a tilde to represent the current user's home directory, you will receive a rather obscure error. It also breaks Net::SCP#upload! calls that used to work prior to this commit.

Example code:

require 'net/scp'

remote_host = '10.0.38.110'

Net::SCP.start(remote_host, "root") do |scp|
  scp.upload! "./test.txt", "~/"
end

Resulting stack trace:

/Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-scp-1.0.4/lib/net/scp.rb:352:in `block (3 levels) in start_command': SCP did not finish successfully (1) (Net::SCP::Error)
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/channel.rb:590:in `call'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/channel.rb:590:in `do_close'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:576:in `channel_close'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:456:in `dispatch_incoming_packets'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:213:in `preprocess'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:197:in `process'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:161:in `block in loop'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:161:in `loop'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:161:in `loop'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-ssh-2.2.1/lib/net/ssh/connection/session.rb:110:in `close'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-scp-1.0.4/lib/net/scp.rb:205:in `ensure in start'
    from /Users/myuser/.rvm/gems/ruby-1.9.2-p290@net-ssh-test/gems/net-scp-1.0.4/lib/net/scp.rb:205:in `start'

This also prevents us from using an upload call that contains parameters intended to be expanded by the shell on the remote machine:

scp.upload! "./test.txt", "$\{TEST_HOME\}"

This will upload the file 'test.txt' to a file named '${TEST_HOME}' on the server. The actual command sent to the remote is:

scp -t \$\{TEST_HOME\}

In the case of the remote path '~/', the resulting command is:

scp -t \~/

I don't necessarily expect tools like SSH and SCP to escape commands sent to remote systems. Quite the opposite. I expect to be able to leverage the remote tools through these interfaces.

@guns

This comment has been minimized.

Copy link

guns replied Sep 16, 2011

The same restrictions and parameter escaping rules apply to
Kernel#system when supplying arguments as parameters:

>>> system 'ls', '~'
ls: ~: No such file or directory
>>> system 'ls', File.expand_path('~')
(directory listing)

If you supply a single string to Kernel#system, however, the string is
sent as a parameter to /bin/sh -c and works as it does on the command
line.

I think not properly sanitizing filenames in :upload! can very easily
lead to disastrous shell-metacharacter attacks like

scp.upload! 'myfile.txt; sh -c "$(nc -l 1337)"', remotedir [1]

so the onus should not be on the user to sanitize. The original
author clearly agrees, since he did at least attempt to escape some
metacharacters.

What you want can be accomplished readily through the raw Net::SSH
connection object (IIRC) or by shelling out to scp.

[1]: I haven't looked at the library in a while, so I don't know if that
particular filename would work pre-patch, but attacks like this are
decades old.

@bradland

This comment has been minimized.

Copy link

bradland replied Sep 16, 2011

I see a distinction between sanitizing arguments as parameters (common and expected) and sanitizing arguments where no parameterization is available.

Take this example of Kernel#system:

>>> system 'echo ~'
/home/myuser

Sanitization occurs on the parameters, but not the command, thus I'm still able to leverage all the power of the shell. Nothing is taken from me by providing the safer option. The distinction is for the caller to decide. In this way Kernel#system offers protection, but also the flexibility expected.

This is the intended functionality as pointed out explicitly in the documentation:

commandline                 : command line string which is passed to the standard shell
cmdname, arg1, ...          : command name and one or more arguments (no shell)
[cmdname, argv0], arg1, ... : command name, argv[0] and zero or more arguments (no shell)

You can either call an entire commandline (untouched), or call a command, then pass sanitized arguments to it.

In the case of Net::SCP#upload, it is reasonable for the caller to expect remote to be passed un-altered, because this is the normal function of it's namesake. If sanitized parameters are desired, a method more similar to Kernel#system would seem like a better idea.

@guns

This comment has been minimized.

Copy link

guns replied Sep 17, 2011

@bradland

This comment has been minimized.

Copy link

bradland replied Sep 18, 2011

I want to thank you guys for the wonderful explanations and insights. I can definitely see the distinction now between a command line utility and something that is used as a library. I was stuck with the knowledge/mindset that the remote parameter was being sent to a remote server, rather than considering what the 'Net::SCP#upload!` does. Sanitizing does makes sense in the context of a library where two path arguments are passed.

I actually encountered the issue while using the Sprinkle provisioning tool. Many scripts I came across used a Sprinkle helper method called "transfer", which is a wrapper for Capistrano::Configuration::Actions::FileTransfer#transfer which is a wrapper for... at the bottom of this rabbit hole is Net::SCP :-) Many scripts used "/somefile" as a remote path. For example, uploading a file to '/.vimrc' file. The error that comes back just says that the SCP transfer failed, but doesn't give much in the way of insight as to why, so I went digging and ended up here.

Having a :raw => true option sounds like a great compromise. Seeing that no one else has come along to complain, I'm guessing it isn't a problem that many people encounter. Are pull requests accepted for Net::SCP if I were to take some time and try to implement the "raw" option myself?

Please sign in to comment.