CVE-2014-6271 hardening...

[Stephane Chazelas]

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