Hi there, I'm reposting here an opinion piece of mine I sent to Chet and the various security lists after the patch was made and prior to the disclosure, for others to comment.
Several things discussed in here: - hardening to avoid exposing the parser to untrusted input - duplicate env var entries - impact of localisation on parsing. > I am strongly in favor of Florian's suggestion to only interpret > variables that are listed in some special BASH_FUNCDEFS variable as > functions. BASH_FUNCDEFS (which contains the names (and names only) of functions to be exported) would be hardening. It would be effective as hardening, but I'd argue it would not be the right fix. The problem is that the environment is a shared namespace. Those "foo=() { body;}" variables have a content that is bash specific, so should have a BASH_ name prefix especially considering that they can have any name. Those variables are not like other env vars (HOME, PATH...) that are shared by all applications, they are just for transfering information from one bash instance to another bash instance. Those variables are not useful to anything but bash. Now, even in bash, having "foo=() {...}" is inconsistent, and that raises possibly another security concern. In bash, functions and variables have different name spaces, but if they have to be exported to the environment, there's a clash. And bash handles it in a dangerous way: You can have: foo=bar foo() { echo "$foo"; } That's fine. Now, if you export the "foo" *function* and if the "foo" *variable* was already in the environment, bash puts *both* in the environment: $ foo=bar bash -c 'foo() { echo "$foo"; }; foo' bar fine. $ foo=bar bash -c 'foo() { echo "$foo"; }; export -f foo; env' | grep foo foo=bar foo=() { echo "$foo" (two env vars by the same name!). $ foo=bar bash -c 'foo() { echo "$foo"; }; export -f foo; bash -c foo' bar That works here because bash scans its environment and assigns the one with "() {" to a function and the one with "bar" to a scalar and bash is directly invoking bash. However, many other shells (mksh, ksh93, zsh, ash, fish, not (t)csh, yash) remove duplicate env vars from the environment (nor always the same depending on the shell), so things like: foo=bar bash -c 'foo() { echo x; }; export -f foo; sh -c "bash -c foo"' won't necessarily work (just because "foo" happened to be in the environment, which "export -f foo" did not overwrite) It's also arguably dangerous because it allows an attacker to hide his malicious function behind a scalar variable (though one might argue that it's not only a bash problem since when the environment contains "foo=bar" and "foo=baz" which one is picked seems to depend on what tool queries the environment). Typically, glibc's getenv will pick the first one. That would possibly defeat an environment sanitizing wrapper, or something that logs the content of some variables. Thankfully, sudo is not fooled here, it will preserve several instances of the same variable (like DISPLAY, TERM...) but will remove all the ones that start with "() {". I think a better fix would be to have all the function *definitions* in one env var like: BASH_FUNCDEFS='f(){ echo x;} g(){ echo y;} > $(date +%H)' I would not be against bash removing env entries with duplicate names like most other shells do (or even the glibc do that as I don't expect it being useful and it sounds to me like an avenue for more security vulnerabilities). Another consideration, and that was one of the aggravating aspects in CVE-2014-0475: Chet's patch restricts the name of exported functions to the "legal identifiers". That's required because with: var=value (where value starts with "() {"), bash runs: varvalue So variables with names like "some code; foo" would cause more problem. Now, what bash considers a legal identifier depends on the locale. CVE-2014-0475 was a glibc vulnerability giving attackers the ability to use locale definition files anywhere on the file systems (so malicious ones as well) with LC_ALL=../../path/to/it. The bash behaviour of deciding on /legal/ identifiers (and token separators!) based on the locale meant one could run arbitrary code (for instance by defining a locale where space was anything but "s" and "h" and relying on a command line in ~/.bashrc that contained something like blashbli (which happened to be true in Debian's default .bashrc)). It might be worth checking that Chet's patch cannot be bypassed in locales where for instance "(", ")" and ";" are in the "alpha" character class. -- Stephane