Use of `eval` introduces possibility of shell injection unnecessarily.

[charles-dyfis-net]

Unfortunately, fixing this will require changing the configuration format -- it may need to wait for a new major release.

Current configuration is of the form:

options:
- "--exclude '*'"
- "--include 'results/*'"

...that is to say, correct shell escaping is required to be part of the data and the boundary between individual options is not meaningful (for example, --exclude is one argument and * is another, but they're entered as part of the same list element; one could add - "--exclude '*' --include 'results/*'" as a single element with the exact same semantic meaning).

The use of eval makes this possible, but that also introduces potential for human error (any command substitution, redirection, globbing, or other syntax present in the options array will be evaluated by the shell executing the scripts).

A safe alternative would be to instead use configuration of the form:

options:
- "--exclude"
- "*"
- "--include"
- "results/*"

...and have jq be responsible for transforming it into a shell array, as follows:

eval "set -- $(printf '%s\n' "$payload" | jq -r '.source.options // [] | @sh')"

...which runs the command:

set -- '--exclude' '*' '--include' 'results/*'

...allowing "$@" to be expanded to the desired arguments later in the script.

Notes

• For the reasons for using printf over echo, see https://unix.stackexchange.com/a/65819/3113, or the APPLICATION USAGE and RATIONALE sections of the POSIX standard for echo)
• Passing eval a single string is strongly preferred over passing it a list of strings. Notably, passing eval multiple strings does not meaningfully pass it distinct arguments: the strings it is passed are concatenated together (with spaces between them) into a single larger string before that single, larger string is passed to the parser. I'm glad to provide some examples of cases where this results in unintended behavior should those be desired.

[charles-dyfis-net]

Thinking about it a bit more: There is a reasonable opportunity to avoid the shell injection vulnerability without breaking backwards compatibility if we're willing to change the shell from sh to bash: xargs can be used to perform shell-like word-splitting and quote removal, emitting a NUL-delimited stream which bash provides builtins to read into an array. It's not quite 100% compatible with a real shell's behavior, but it's good enough to make all the examples in this resource's documentation work.

In bash 4.0 4.4 or later, readarray -d '' args < <(xargs printf '%s\0' <<<"$options") will split the string options into the array args, which can later be expanded with "${args[@]}".

I've described this in more detail at https://stackoverflow.com/questions/26067249/reading-quoted-escaped-arguments-correctly-from-a-string

[pburkholder]

Hi Charles,

Thanks for these PRs and suggestions. Things are kind of quiet there this week, so we'll dig into them more next week.

Are you using s3-resource-simple or just wanting to provide a public service to wrap out the year (I'm just curious, we're happy to have your contributions regardless)?

Happy New Year!

--Peter

[charles-dyfis-net]

Thank you! Next week is actually sooner than I'd expected; my spouse used to work for FHWA and was advising me not to expect anyone back in the office for a good while.

Yes, I am actually using this (particularly, a local build with #51 applied) in practice. (Might end up moving away from it at some point, or at least supplementing it with an additional resource, as aws s3 sync isn't quite capable enough for my purposes wrt managing object lifecycles/expiration, but it was good enough for a prototype).

[charles-dyfis-net]

BTW, if y'all are comfortable in practice with adding bash as a dependency, I'll be glad to put together a PR after #51 is dealt with (to avoid any need to deal with merge conflicts from having two changesets touching the same pieces in flight).

I might be tempted to start using xargs to get shell-like parsing following the preexisting/documented behavior for options:, and then add a new options-literal: or similar with the behavior described in this ticket.