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

Bug: Proper shellescaping missing #283

Closed
ncreuschling opened this issue Sep 25, 2015 · 4 comments
Closed

Bug: Proper shellescaping missing #283

ncreuschling opened this issue Sep 25, 2015 · 4 comments

Comments

@ncreuschling
Copy link
Contributor

We recently ran into a problem with unescaped shell commands via execute.

I think, shellescaping with Ruby's built-in method has been removed due to #10 in version 1.5.0. It is my understanding, sshkit does not shellescape any command at the moment (although there will be in #280).

From my perspective, sshkit should properly escape shell commands by default due to high security implications. It would be too much of a burden to have users of sshkit make sure of correct escaping (although it is rather easy with Shellwords::escape).

May I suggest to reintroduce proper shell escaping. Users with the need to run unescaped command (which I do not dispute) should be using a new and different method, e.g. execute_unescaped, instead of execute.

Feedback is very welcome!

@leehambley
Copy link
Member

Unless it's provably perfect (hint: it isn't) Shellwords::escape is more dangerous as an incomplete solution than being clear that we don't do escaping.

What was the concrete issue you had?

@ncreuschling
Copy link
Contributor Author

Unless it's provably perfect (hint: it isn't) Shellwords::escape is more dangerous as an incomplete solution than being clear that we don't do escaping.

I don't understand. Why? You care enough to escape environment variables…

What was the concrete issue you had?

We execute commands with parameters provided by the user or certain configuration. On of the parameter contained a ampersand &.

Pseudo code:

var1 = 'abcd&ef'
execute :command, var1

What gets executed:

$ command abcd&

ef never make it to the argument list for command. Instead they are being executed as next command ef.

@leehambley leehambley changed the title Proper shellescaping missing Bug: Proper shellescaping missing Feb 21, 2016
@leehambley
Copy link
Member

@ncreuschling I'm not sure if you still have a vested interest in this, lots of time has gone by. I'm closing the issue because there's no proper way for us to do shell escaping and guarantee that it behaves the way people expect. Escaping of ENV vars was probably a mistake too.

I'm sure you can acknowledge that any change to this will leave Capistrano in a better state for some, but a worse state for others. If you need your input to be escaped, or are using Capistrano with untrusted inputs, you might better address it in your own code, as I'm sure you've already had to do.

I'm sure we could compromise on a warning if something is not shell escaped, but then making it possible to disable the warning, globally, or on a per-call basis adds more complexity to the DSL API that I don't see a neat solution for just now.

String#tainted springs to mind, but I believe the feature to be too obscure to be widely acceptable as a useful solution. See #333

@leehambley
Copy link
Member

Obsoleted by #333

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

No branches or pull requests

2 participants