Remove all uses of eval

[zaneduffield]

Beyond being risky, many of these uses of eval were actually vulnerable to shell injection.

Switching all cases to nameref variables improves both security and readability, while only raising the minimum bash version from 4 to 4.3.

A simple example of an exploit against the eval-based version follows

#!/bin/bash
. cmdarg.sh

declare -a array
declare -A hash
cmdarg 'a:[]' 'array'

cmdarg_parse "$@" || exit 2

run like

./pwn.sh -a '"; whoami; #'

[akesterson]

Test look good.

$ PREFIX=/home/andrew/local make test
AK_PREFIX=. /home/andrew/local/bin/shunit.sh -f tunit -t tests | tee tunit.txt
[tests/test_clean_state.sh] shunittest_clean_state .... [OK]
[tests/test_clean_state.sh] shunittest_clean_state_subshells .... [OK]
[tests/test_clean_state.sh] shunittest_clean_state_usable .... [OK]
[tests/test_dashdash.sh] shunittest_dashdash .... [OK]
[tests/test_dashdash.sh] shunittest_missing_dashdash .... [OK]
[tests/test_dashdash.sh] shunittest_withbool_missing_dashdash .... [OK]
[tests/test_dashdash.sh] shunittest_withopt_with_dashdash .... [OK]
[tests/test_equals.sh] shunittest_test_equals_parsing_longopt .... [OK]
[tests/test_equals.sh] shunittest_test_equals_parsing_shortopt .... [OK]
[tests/test_helpers.sh] shunittest_test_describe_and_usage_helper .... [OK]
[tests/test_helpers.sh] shunittest_test_describe_helper .... [OK]
[tests/test_helpers.sh] shunittest_test_usage_helper .... [OK]
[tests/test_info.sh] shunittest_info_accept_valid .... [OK]
[tests/test_info.sh] shunittest_info_reject_invalid .... [OK]
[tests/test_longopt.sh] shunittest_longopt .... [OK]
[tests/test_longopt.sh] shunittest_longopt_shortopts_still_work .... [OK]
[tests/test_longopt.sh] shunittest_longopt_usage_messages_array .... [OK]
[tests/test_longopt.sh] shunittest_longopt_usage_messages_boolean .... [OK]
[tests/test_longopt.sh] shunittest_longopt_usage_messages_hash .... [OK]
[tests/test_longopt.sh] shunittest_longopt_usage_messages_string .... [OK]
[tests/test_types.sh] shunittest_array_undefined .... [OK]
[tests/test_types.sh] shunittest_array_values .... [OK]
[tests/test_types.sh] shunittest_boolean_no_optarg .... [OK]
[tests/test_types.sh] shunittest_flags_required .... [OK]
[tests/test_types.sh] shunittest_hash_malformed .... [OK]
[tests/test_types.sh] shunittest_hash_undefined .... [OK]
[tests/test_types.sh] shunittest_hash_values .... [OK]
[tests/test_validators.sh] shunittest_validator_failure_recognized .... [OK]
[tests/test_validators.sh] shunittest_validator_for_array .... [OK]
[tests/test_validators.sh] shunittest_validator_for_hash .... [OK]

==== 30 TESTS in 0 SECONDS : 0 ERRORS, 0 FAILURES ====

[akesterson]

For what it's worth I've known about this since the creation of the library, and I'll be honest, I'm not sure why we should worry about the scenario being described here. I see the example, but that's not an example of an exploit, it's just execution of commands. The effect is no different than someone doing

./pwn.sh -a "$(whoami)"

So why is this a cause for alarm? I'm not necessarily against the patch (although even a minor version change might cause problems in some places), I'm just curious about the justification for the concern.

[zaneduffield]

Imagine a scenario where the script is hooked into some automation process, where the library is used to parse the arguments provided from some untrusted source.

In this scenario you're setting the script as the trust boundary, granting permission for the script to be run with some arbitrary arguments, but no more.

In addition to the possibility of injection, the version with eval is simply less correct, in that there are valid strings you can provide that it cannot parse faithfully.

[akesterson]

Imagine a scenario where the script is hooked into some automation process, where the library is used to parse the arguments provided from some untrusted source.

I understand that scenario, but ensuring the safety of the inputs in that scenario is not cmdarg's job, any more than it's the SQL library's job to ensure that little Bobby Tables is properly handled. The script in this case is running inside of a privileged shell as the user; anything and everything cmdarg does (including the execution of validator functions) will happen with the privileges of the user running the current shell. It is the responsibility of the program calling the library to ensure the library is receiving sane inputs.

... the version with eval is simply less correct, in that there are valid strings you can provide that it cannot parse faithfully.

I find this justification a lot more compelling. What are some examples of this behavior?

[zaneduffield]

I find this justification a lot more compelling. What are some examples of this behavior?

In my original example the expected and correct behaviour would be for the array variable to be set to contain the provided input, like array=('"; whoami; #').

$ array=('"; whoami; #')
$ echo "${array[@]}"
"; whoami; #

but with eval, the array is actually set as array=('')

[akesterson]

Fair point. Thanks for indulging me.

There is a problem with CI but that doesn't appear related to your changes. The tests look good.

Merged