Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
leehambley opened this issue Feb 21, 2016 · 7 comments
Open
Labels
Milestone

Comments

@leehambley
Copy link
Member

@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:

  • Identify where we want to escape things, if at all:
    • Arguments to test(), capture(), execute() etc (via the common execute() function)
    • Environment variables, and values when used with with(){}
    • Environment variables when used with default_env
  • Find out if strings are tainted or not by default in the Capistrano context.
  • Find a backwards compatible way to:
    • Issue a release with warnings for "tainted" strings in the places we care about, warning that behavior will change
  • See if it makes sense to compare untrusted (the trust vernacular is deprecated) against their shellescaped counterparts, and exit with an error in that case.

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
Copy link
Member

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
Copy link
Member Author

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 leehambley modified the milestone: 1.10.0 Mar 7, 2016
@leehambley
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

I like where this is going! Couple more ideas:

  • What about shell_safe! and shell_safe? instead of escaped?
  • Should escaped!/shell_safe! also freeze the string to prevent mutations that could break safety?

@leehambley
Copy link
Member Author

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
Copy link
Contributor

I like where this is going!

Agreed.

/cc @websi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants