Re: REGRESSION: shellshock patch rejects valid function names

[Eric Blake]

On 09/27/2014 03:10 PM, Jay Freeman (saurik) wrote:
> [I am terribly sorry that my In-Reply-To is wrong :/]
> 
> ----- "Eric Blake" <address@hidden> wrote:
> 
>> ... Remember, the security hole of
>> Shell Shock is NOT what the function is named, but the fact that
>> arbitrary variable contents were being parsed. ...
> 
> Not quite: the point of exploit can be in the variable name.

That is NOT the same exploit as Shell Shock.  It is indeed a
(local-only) issue, that we should be careful to avoid, but the point of
Shell Shock is that attackers were able to influence contents, but NOT
names, of data stuck into the environment.

> 
> $ env 'date;x=() { :;}' bash --norc -c :
> Sat Sep 27 20:40:41 UTC 2014
> Segmentation fault (core dumped)

Yes, I agree that bash should be robust against garbage in the
environment.  And that's true whether or not we stick with the 4.2.26
behavior of (ab)using 'x=() {...}' for function imports, or go with
Florian's patch to use 'BASH_FUNC_x()=() {...}'.  Thus, we want to make
sure that even with Florian's patch, 'BASH_FUNC_date;x()=() {...}' does
NOT case an arbitrary execution of 'date'.  But, as you pointed out, it
is NOT possible for bash to directly export a bogus function name
containing metacharacters, and therefore, the only way to inject
metacharacters into a name that might cause the parser to invoke a
command is via the local environment outside of bash, whereas Shell
Shock is about the problem of arbitrary contents of REGULAR variables on
export being misinterpreted on input.

> While AFAIK you can't create functions with names that eventually lead to 
> dangerous variable names, this is due to a quoting requirement (to place that 
> ";", for example, in a function definition statement, I'd have to quote it, 
> and check_identifier refuses to allow any quoted characters) that happens 
> when parsing the function as it is created "in the first place" that are not 
> quite so easily replicated when trying to use check_identifier to validate 
> the name before importing the function (hence why, even with the patch I 
> provided yesterday that verifies function names before importing using a 
> mechanism similar to that used for definitions, I still rely on SEVAL_FUNCDEF 
> to catch "date;x": the ; has not been "quoted", so check_identifier considers 
> it not to be a problem; incidentally, this is why my original patch concept, 
> before I understood the new SEVAL_FUNCDEF and verified it was sufficient as 
> in the patch I provided yesterday, involved running the name itself through 
> the parser t
o verify it was a single word).

It should be fairly easy to validate whether an incoming environment
name=value pair names a valid shell function name, without having to
resort to the full power (and possible unknown bugs) of the parser.  And
it is only common sense that we do so.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature