CLIUtils.generateArgline does not quote IFS characters other than space

[charles-dyfis-net]

Newlines and tabs can be injected into generated commands unquoted. This results in direct security issues, particularly if an untrusted data source is used for options.

Consider a tool wherein a user-specified option can be one of a limited number of values, but wherein additional arguments are used to form an argv array (not an uncommon design pattern):

(CLIUtils/generateArgline "run-tool"
  (into-array String ["-o" "safety-checked-arg\tunchecked-arg\tanother-unchecked-arg"]))

This generates the following command:

run-tool -o safety-checked-arg unchecked-arg another-unchecked-arg

...or:

(CLIUtils/generateArgline "run-tool"
  (into-array String ["safety-checked-arg'\r\nrm -rf /\r\n'"]))

...which generates:

run-tool 'safety-checked-arg'
rm -rf /
''

[charles-dyfis-net]

This issue goes well beyond failing to quote non-space characters. We also don't quote quotes (so the user can, SQL-injection-style, remove themselves from same):

(CLIUtils/generateArgline "run-tool" (into-array String ["''$(rm -rf /)''"]))

run-tool ''$(rm -rf /)''

(Notably, paired single quotes cancel each other out, so ''foo'' is exactly equivalent to foo).

[charles-dyfis-net]

(Not sure how the issue was closed, btw -- that looks like it was a PEBKAC).

[charles-dyfis-net]

Another permutation of this issue, this one verified remotely exploitable: Glob characters are not expanded.

If * is specified as a parameter, rather than quoted and passed as a literal to the command being run, this is expanded to a list of matching files by the remote shell.

[charles-dyfis-net]

One item which substantially complicates developing a satisfactory patch for this issue:

Replacing the current behavior of CLIUtils.testGenerateArgline() with completely faithful conversion from String[] to quoted argument list would break behavior which is currently validated by the unit test suite.

Consider the following example:

/* current test-suite code, from TestExecTool.testGenerateArgline() */
assertEquals("invalid",
   "test 1 2 \"34\"",
   CLIUtils.generateArgline("test", new String[]{"1", "2", "\"34\""}));

Since the input array contains literal quotes, an algorithm which faithfully represented its input array within its output would have different behavior:

/* proposed test validating more defensible behavior */
assertEquals("invalid",
   "test 1 2 '\"34\"'",
   CLIUtils.generateArgline("test", new String[]{"1", "2", "\"34\""}));

...thereby representing the exact user-provided input, quotes intact.

(Handling single quotes or mixed quotes gets a little more interesting -- but I have a working, tested implementation; it isn't submitted as a pull request only due to the conflict with preexisting semantics).

[gschueler]

Hi Charles,

In examining the usage of the CLIUtils, the code actually has a few different use-cases for it:

1. allow conversion between list of strings to a single string and back
   • used to e.g. take CLI input from "dispatch" and pass it as a single string to the server
   • used to take a set of option->value pairs at job execution time, and store it as a single string in the DB, and later convert back to key/values
2. generate the argument list to pass to shell commands, from input which is a single string with embedded property references

In addition, I'm not even sure what kind of quoting is necessary to solve the same potential problem in a Windows environment.

We need to refactor the usage of CLIUtils a bit more to solve shell quoting issues and remove conflation between its use-cases.

So I propose we do something like this:

• move the "safe" quoting to a Unix-shell specific class for generating the exec arguments
  • use this class for quoting arguments to execute on unix nodes
• move/revamp the CLIUtils for the conflated "argLine" split and generate which is used internally by rundeck
• either keep the status-quo, or enhance the argLine split/generate to be better.

thoughts?

also, thanks for bringing this to light and kicking off the beginning of a fix :)

[charles-dyfis-net]

In an ideal world, we'd enhance the argLine split/generate to be able to unambiguously represent any valid C string (ie. anything not containing a NUL) as an argument. Not implementing this restricts the user without any obvious benefit.

However -- that's not a trivial amount of work. I'm not familiar with any existing parser for either POSIX sh or zsh command lines implemented for Java, certainly not enough to vouch for completeness or safety. (Python has its excellent shlex module, but it's not trivial to port). [Aside: zsh was mentioned because its semantics are closer to those of rundeck's argLine format as documented and used than those of POSIX sh; in zsh, parameter expansion results are not string-split or glob-expanded, so ${foo} will always become exactly one parameter, rather than an unknown number depending on its contents].

[gschueler]

need to add documentation about quoting arguments in exec strings