capistrano / sshkit

A toolkit for deploying code and assets to servers in a repeatable, testable, reliable way.
MIT License
1.14k stars 253 forks source link

Issue: Trusted strings, shell escaping and backwards compatibility. #333

Open leehambley opened 8 years ago

leehambley commented 8 years ago

@ncreuschling @mattbrictson /cc

Continuing from the discussion in #283 I'd like to raise the general issue of shell escaping (shellwords) and what we mean/expect about "escaping strings" and so forth, whilst closing #283 it came to my mind that Rails using Object#taint, Object#tainted? and family works mostly pretty well.

I want to unify behavior.

I would propose the following:

I'm afraid that strings coming from the same file that is running will be trusted by default, so the whole plan falls apart, I'd have to look more closely at the API to see if the default taint value for a string is tainted, untainted, or (ideally) none.

Ideally, this would mean, I could do something like this:

SSHKit.config.default_env = {
   "$MY_LITTLE_PONY".untaint => "i should however be escaped" 
}

This would lead to us calling a literal export $MY_LITTLE_PONY=i should however be escaped, for better or worse.

mattbrictson commented 8 years ago

Hmm, I am not familiar with taint/untaint in Ruby, so I don't know what is involved. Could this be done like HTML escaping in Rails views, where strings are assumed to be unsafe unless raw or html_safe is explicitly called?

The purist in me says that SSHKit should do no escaping at all, and then it is up to users to explicitly escape what needs to be, but I realize that this is an unrealistic (and unsafe) expectation.

Like you say, the best compromise is probably to do the best automatic escaping we can, and give users the ability to opt-out on a per-string basis if they know what they're doing.

leehambley commented 8 years ago

Could this be done like HTML escaping in Rails views, where strings are assumed to be unsafe unless raw or html_safe is explicitly called?

I believe this was once implemented (or, may still be) in terms of tainted/untained. There are some simple rules, for example if you load something from a file (YAML.load) the result is, iirc considered to be "tainted"

Unfortunately in our case it would be difference, since it's all in required files, then it's all trusted by default (I think) hence my questioning whether the default is "none", "trusted" or "untrusted".

If the default were "no value" we could compare it against the shellwords escaped and trust/untrust it if they differ. If the user has specified a value ahead of time then we

Another option would be to use refinements, and add something like tainted/untained to Strings within SSHKit ourselves.

leehambley commented 8 years ago

I looked into this, taint unfortunately doesn't work. But I'm going to write a refinement for the String and Symbol classes that escape them and set a flag when they are initialized. SSHKit will rely on this flag internally, and warn, or fatally err out if the string and it's shellwords escaped doppelgänger are different.

leehambley commented 8 years ago

Simple test-cases:

require 'test_helper'

class Shellwords::Escape::RefinementTest < Minitest::Test

  using Shellwords::Escape::Refinement

  def test_trusting_a_string_that_is_equal_to_it_s_escaped_value
    assert "helloworld".escaped?
  end

  def test_not_trusting_a_string_that_is_equal_to_it_s_escaped_value
    refute "hello & world".escaped?
  end

  def test_trusting_a_string_that_is_not_equal_to_it_s_escaped_value_but_force_trusted
    assert "hello & world".escaped!.escaped?
  end

end

And the implementation...

module Shellwords
  module Escape
    module Refinement
      refine String do
        def escaped!
          @_sw_escaped = true
          self
        end
        def escaped?
          case @_sw_escaped
          when nil then self == Shellwords.shellescape(self)
          else @_sw_escaped
          end
        end
      end
    end
  end
end

Refinements might be the wrong hammer here, I'm still trying to work out how the API would look.

We would have to call using .... in the DSL class to get the fallback behaviour (string is untrusted if it doesn't match itself escaped,Shellwords.escape() is not idempotent which makes that always true for untrusted strings.)

I think someone who wanted to override these would have to pull in the refinement via using ... and then call escaped! on the string, hopefully the refinement (or at least the value of @_sw_escaped) persists into the DSL scope for us to make a decision, print a warning message, or similar.

mattbrictson commented 8 years ago

I like where this is going! Couple more ideas:

leehambley commented 8 years ago

Probably both wise decisions, in the end we can choose (once)

Freezing the string does seem like a decent idea incase of naming it safe.

ncreuschling commented 8 years ago

I like where this is going!

Agreed.

/cc @websi