Re: REGRESSION: shellshock patch rejects valid function names
On 09/29/2014 08:33 PM, David Korn wrote: I fixed the bug in ksh that allows you delete a special builtin. Thanks; here's another ksh bug: $ env 'a|b=' bash -c 'set | grep a.b' $ env 'a|b=' ksh -c 'set | grep a.b' a|b='' But per the documentation of set, If no options or arguments are specified, set shall write the names and values of all shell variables in the collation sequence of the current locale. And elsewhere, Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set and do not begin with a digit. Other characters may be permitted by an implementation; applications shall tolerate the presence of such names. Where this is a problem is that by exposing the name a|b through set, ksh is making the claim that it is a valid shell variable, even though: $ ksh -c 'unset a|b' ksh: unset: a|b: invalid variable name So ksh should be fixed to behave like bash to sanitize the environment and strip out any invalid names before populating the set of shell variables advertised through 'set' (you can still leave such name/value pairs in the environment handed to children, unmolested, the way bash does; it's just that you should not be able to get at or modify those names from the shell). https://bugzilla.redhat.com/show_bug.cgi?id=1147645 is also tracking this issue. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
On 09/30/2014 08:42 AM, Eric Blake wrote: Thanks; here's another ksh bug: $ env 'a|b=' bash -c 'set | grep a.b' $ env 'a|b=' ksh -c 'set | grep a.b' a|b='' But per the documentation of set, If no options or arguments are specified, set shall write the names and values of all shell variables in the collation sequence of the current locale. Or, reading further, The output shall be suitable for reinput to the shell, setting or resetting, as far as possible, the variables that are currently set; but with the current output of set, trying to invoke 'eval $(set)' will end up executing the command 'a' in the current working directory rather than setting the variable a|b. While the bash Shell Shock bug has already established that arbitrary environment variable names are a far less likely attack vector than arbitrary environment variable values, I still can't help but wonder if someone can come up with an exploit where a ksh script that runs with elevated privileges and tries to eval the output of set will end up running code of the caller's choice. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
2014-09-30 09:04:28 -0600, Eric Blake: On 09/30/2014 08:42 AM, Eric Blake wrote: Thanks; here's another ksh bug: $ env 'a|b=' bash -c 'set | grep a.b' $ env 'a|b=' ksh -c 'set | grep a.b' a|b='' But per the documentation of set, If no options or arguments are specified, set shall write the names and values of all shell variables in the collation sequence of the current locale. Or, reading further, The output shall be suitable for reinput to the shell, setting or resetting, as far as possible, the variables that are currently set; but with the current output of set, trying to invoke 'eval $(set)' will end up executing the command 'a' in the current working directory rather than setting the variable a|b. While the bash Shell Shock bug has already established that arbitrary environment variable names are a far less likely attack vector than arbitrary environment variable values, I still can't help but wonder if someone can come up with an exploit where a ksh script that runs with elevated privileges and tries to eval the output of set will end up running code of the caller's choice. [...] Same with export -p: $ env -i $'a\necho test\na=b' ksh -c 'export -p' | ksh test And bash is also vulnerable. $ env -i $'a\necho test\na=b' bash -c 'export -p' declare -x OLDPWD declare -x PWD=/home/stephane declare -x SHLVL=1 declare -x a echo test a (that output doesn't make much sense, suggesting it may also hide more bugs and vulnerabilities). -- Stephane
Re: REGRESSION: shellshock patch rejects valid function names
2014-09-30 17:06:22 +0100, Stephane Chazelas: [...] Same with export -p: $ env -i $'a\necho test\na=b' ksh -c 'export -p' | ksh test And bash is also vulnerable. $ env -i $'a\necho test\na=b' bash -c 'export -p' declare -x OLDPWD declare -x PWD=/home/stephane declare -x SHLVL=1 declare -x a echo test a (that output doesn't make much sense, suggesting it may also hide more bugs and vulnerabilities). [...] Sorry, it does make sense. bash just outputs: declare -x var-name when var-name is not a valid identifier in the current locale. Both ksh and bash's can be exploited using the LC_XXX with ssh ForceCommand vector (and the output of export -p being evaluated somehow). -- Stephane
Re: REGRESSION: shellshock patch rejects valid function names
2014-09-27 23:53:12 +0200, Arfrever Frehtes Taifersar Arahesis:
[...]
Ability to export/import functions with = in function names
could be achieved by not embedding function names in
environmental variables and using a single BASH_FUNCTIONS
environmental variable whose value would contain code of all
exported functions (in format similar to `declare -fpx` /
`export -fp`).
[...]
That's what I had in mind as well.
bash could align its function definition syntax with zsh's and
do
BASH_FUNCTIONS=f(){ echo f;}
'some function'(){ echo sf; }
'even with
newline and / and '\''quote
...'() { echo x; }
That would also be nicer with the environment as well
(BASH_FUNC_xxx()= needs 14 bytes, plus the pointer (4/8 bytes)
per exported function.
If bash were to implement that, sudo and possible others would
have to be updated at the same time to blacklist that variable
(at the moment sudo blacklists variables whose value starts with
() which is why the change to BASH_FUNC_xxx()=()... didn't
expose sudo).
--
Stephane
Re: REGRESSION: shellshock patch rejects valid function names
2014-09-29 09:04:00 -0600, Eric Blake: [...] I would expect something like It shall be an error if fname is the name of a special built-in utility, as _that_ would be a hard requirement on bash, not just the application. Maybe we have a bug in the POSIX spec for a missing requirement. [...] Such a requirement would break bash, zsh, mksh, ATT ksh (for disciplines), which currently all allow a function called a.b Cheers, Stephane
Re: REGRESSION: shellshock patch rejects valid function names
Just a few points to add.
On Monday, September 29, 2014 04:29:52 PM Stephane Chazelas wrote:
2014-09-29 09:04:00 -0600, Eric Blake:
[...]
The function is named fname; the application shall ensure that it is a
name (see XBD Name) and that it is not the name of a special built-in
utility.
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_05
This doesn't normally matter because POSIX requires special builtins to take
precedence over functions during command search, so even if you have such a
function defined it is impossible to call. Bash doesn't use the correct search
order however.
Mksh has the reverse bug. It allows defining the function (wrongly) but then
calls the special builtin anyway (correctly).
Another bug is in ksh93 whose `builtin` allows disabling special builtins
(which according to the manual, shouldn't work).
$ ksh -c 'builtin -d set; function set { echo test; }; set'
test
Bash's enable correctly disallows that.
I agree the requirement is on the application, and I can't see
why POSIX should force a shell to reject a function whose name
doesn't contain a valid identifier.
...
Another thing you can do in bash is bypass its command name check by using a
null zeroth word.
$ { function } { echo test; }; () }; }
test
Ordinarily } would be uncallable, but apparently since bash only checks the
command name of the first word, calling with e.g. `() }` or `$() }` works.
--
Dan Douglas
signature.asc
Description: This is a digitally signed message part.
Re: REGRESSION: shellshock patch rejects valid function names
Dan Douglas orm...@gmail.com writes:
Another thing you can do in bash is bypass its command name check by using a
null zeroth word.
$ { function } { echo test; }; () }; }
test
Ordinarily } would be uncallable,
Only literal } is special, you can use \} instead, or store it in a
variable.
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
Re: REGRESSION: shellshock patch rejects valid function names
On Tuesday, September 30, 2014 12:11:15 AM Andreas Schwab wrote:
Dan Douglas orm...@gmail.com writes:
Another thing you can do in bash is bypass its command name check by using
a
null zeroth word.
$ { function } { echo test; }; () }; }
test
Ordinarily } would be uncallable,
Only literal } is special, you can use \} instead, or store it in a
variable.
Yeah I forgot there are a few ways. `0 }` is yet another.
--
Dan Douglas
Re: REGRESSION: shellshock patch rejects valid function names
On 9/29/14, 5:25 PM, Dan Douglas wrote: This doesn't normally matter because POSIX requires special builtins to take precedence over functions during command search, so even if you have such a function defined it is impossible to call. Bash doesn't use the correct search order however. Bash searches for special builtins before shell functions when in Posix mode. -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
On 9/27/14, 10:03 PM, Eric Blake wrote: Yes, it's an application requirement. Regardless, all the versions of bash we're talking about here reject non-identifiers. I'm still trying to find that line in the actual POSIX spec. The function is named fname; the application shall ensure that it is a name (see XBD Name) and that it is not the name of a special built-in utility. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_05 -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
Hey Eduardo - Jay is one of many - the fix for the parser exploit is using the wrong code to decide if the identifier is valid for a function. And it doesn't have to. Jay should certainly not fix his working scripts - which, btw, could have been working for the last 20 years. i guess i'll submit a working patch if necessary. Chet, is that necessary? On Sep 26, 2014, at 1:08 PM, Jay Freeman (saurik) sau...@saurik.com wrote: - Eduardo A. Bustamante López dual...@gmail.com wrote: Well, what did you expect? You're relying on undocumented features. ... So, fix your scripts, perhaps? I am sorry I seem to have offended you so much to have warranted this form of response :(. It's common knowledge that if you rely on undocumented stuff, your code will eventually break, like it did now. It's not a regression though, nowhere in the manual you'll find that colons are allowed in function names. Please note that I am by far not the only person who uses colons in function names: Google's style guide for shell actually states that using :: in function names to separate library names from function names and package names within library names is their best practice. http://google-styleguide.googlecode.com/svn/trunk/shell.xml?showone=Function_Names#Function_Names Lower-case, with underscores to separate words. Separate libraries with ::. Parentheses are required after the function name. The keyword function is optional, but must be used consistently throughout a project. If you're writing a package, separate package names with ::. If we are going to be adamant that this functionality--functionality that many people have relied on since the dawn of bash (the earliest version of bash I have access to has always allowed this), functionality that the code went out of its way to support (that check_word flag exists SOLELY for this purpose: this isn't accidental), functionality that makes sense (as it allows function names to replace any normal executable command), functionality that one of the world's largest software companies not only uses but actively encourages third-parties to use--is a duh, you shouldn't have done that, fix your s**t scenario, can we at least 1) document this behavior change and 2) start a deprecation schedule on function/export supporting these identifiers in the first place? Thanks, Brian -- Brian J. Fox Technology Partner The Okori Group, LLC A: 901 Olive St., 93101 O: 805.617.4048 C: 805.317.4048
Re: REGRESSION: shellshock patch rejects valid function names
On 9/26/14, 4:43 PM, Brian J. Fox wrote: Hey Eduardo - Jay is one of many - the fix for the parser exploit is using the wrong code to decide if the identifier is valid for a function. And it doesn't have to. Jay should certainly not fix his working scripts - which, btw, could have been working for the last 20 years. i guess i'll submit a working patch if necessary. Chet, is that necessary? No, it's not necessary. I have a longer explanation which I'll post in a separate reply detailing why I did what I did and the path forward. (A preview: think of what could happen if someone figured out how to remotely specify function names instead of values. Bash allows, and has always allowed, shell function names containing slashes.) Chet -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
On 9/26/14, 4:17 PM, Eric Blake wrote:
At any rate, this seems like an inadvertent regression that could be
patched; are you willing to propose such a patch? The gist of the
matter is that the code base must use the same decision on what forms a
valid function name as it does in deciding what exported non-variable
names in the environment can be reinstated as functions. I'm fairly
certain that Chet will be reasonable about this, and after the worst
fires are put out, we can revisit this.
It wasn't inadvertent, but it is not intended to be permanent. Here is
the long explanation, cut from a discussion earlier today on a different
mailing list:
The original set of patches (bash43-025 and its siblings) tightened the
restrictions on allowable imported function names, forcing them to be
shell identifiers. The shell itself, when not in posix mode, allows
virtually any character that is not a shell metacharacter to appear in
a function name (that's basically the difference between an identifier
and a word in shell-grammar-speak). This results in the ability to
define functions like this:
$ function /bin/echo () { builtin echo whoops; }
and have this happen:
$ /bin/echo
whoops
along with exporting these functions and importing them without complaint.
This is obviously bad, and I removed the ability to do this in the first
patch in the event that someone figured out an easy way to remotely
specify an arbitrary variable name before we implemnted something to stop
it.
The problem is that it's too restrictive. There are folks who have taken
advantage of this flexibility to define, use, and export functions like
STD::what::does::this::do
which are no longer allowed. This is a pretty bad break with backwards
compatibility.
So what's your opinion on the appropriate set of restrictions? This is a
question that goes farther than what a particular shell will import,
since I'm going to align the restrictions on what functions a shell will
import from the environment with what functions that shell will let a
user define. That means that a posix-mode shell will require imported
functions to be valid identifiers, but a non-posix mode shell will allow
words. The original check that was in bash-4.3 does this. What additional
checks should there be? I can see starting with rejecting function names
that can be confused with pathnames.
(And I'm not interested in rehashing decisions that were made 25 years
ago, and I am completely aware that this violates Posix. That's why it
doesn't do this in posix mode.)
Chet
--
``The lyf so short, the craft so long to lerne.'' - Chaucer
``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
On 09/27/2014 12:53 PM, Chet Ramey wrote:
$ function /bin/echo () { builtin echo whoops; }
along with exporting these functions and importing them without complaint.
This is obviously bad, and I removed the ability to do this in the first
patch in the event that someone figured out an easy way to remotely
specify an arbitrary variable name before we implemnted something to stop
it.
Right now, we know of no way for an attacker to force an arbitrary
variable name - ONLY arbitrary variable contents. So I would prefer a
patch that allows the export/import of the exact same set of names as
what can be used as valid function names. Neither set should be larger
than the other, and for back-compat purposes, at least in bash mode, the
set needs to STILL allow for functions named 'foo::bar' or 'foo/bar'.
The problem is that it's too restrictive. There are folks who have taken
advantage of this flexibility to define, use, and export functions like
STD::what::does::this::do
which are no longer allowed. This is a pretty bad break with backwards
compatibility.
Exactly, this break is undesirable. Remember, the security hole of
Shell Shock is NOT what the function is named, but the fact that
arbitrary variable contents were being parsed. Once you plug that hole
by sticking function import/export in a separate namespace, then there
should be no problem in allowing ALL function names that have always
been previously allowed.
So what's your opinion on the appropriate set of restrictions? This is a
question that goes farther than what a particular shell will import,
since I'm going to align the restrictions on what functions a shell will
import from the environment with what functions that shell will let a
user define. That means that a posix-mode shell will require imported
functions to be valid identifiers, but a non-posix mode shell will allow
words. The original check that was in bash-4.3 does this. What additional
checks should there be? I can see starting with rejecting function names
that can be confused with pathnames.
I'm 50:50 between two options:
1: Have two sets of valid names. In /bin/sh POSIX mode, functions can
only be variable names, and when /bin/sh is importing from the env, it
silently ignores or discards any mangled names that would not fit that
restriction; meanwhile, invoking a function is only possible if the
function name is a variable name; then in bash mode, functions can have
any name and be invoked by that name
2. Have only a single rule on what forms a valid name. POSIX doesn't
forbid us from having an enlarged namespace for valid function names; a
strictly conforming application won't use such unusual names. You may
still want to ignore or scrub those out of the environment during
/bin/sh importing, but that brings us back to the question of whether
function imports should be automatic, or something the user has to
explicitly request. If ALL function imports are skipped unless
explicitly requested, then it doesn't matter WHAT function names are
exported, the incoming functions in a child shell (regardless of whether
the shell is in bash or /bin/sh mode) will have no impact to that child
unless the script programmer of the parent asked to have functions imported.
Thankfully, bash already forbids trying to name a function 'a=b' (having
to support such a name would make env-var export/import rather
difficult), so the main culprits that people seem to be worried about
are '-', '.', ':', and '/'.
(And I'm not interested in rehashing decisions that were made 25 years
ago, and I am completely aware that this violates Posix. That's why it
doesn't do this in posix mode.)
I don't see function export/import as a violation of posix; the only
violation I see is that the implementation is preventing the use of
arbitrary values of normal variables. And the proposed fixes that use
an alternate namespace to avoid collisions have already resolved that issue.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
2014-09-27 22:29 Eric Blake napisał(a):
Thankfully, bash already forbids trying to name a function 'a=b'
It works in bash 4.3.26:
$ function a=b() { echo A; }
$ a=b
A
$ a\=b
A
--
Arfrever Frehtes Taifersar Arahesis
signature.asc
Description: This is a digitally signed message part.
Re: REGRESSION: shellshock patch rejects valid function names
On 09/27/2014 02:29 PM, Eric Blake wrote:
On 09/27/2014 12:53 PM, Chet Ramey wrote:
$ function /bin/echo () { builtin echo whoops; }
along with exporting these functions and importing them without complaint.
This is obviously bad, and I removed the ability to do this in the first
patch in the event that someone figured out an easy way to remotely
specify an arbitrary variable name before we implemnted something to stop
it.
Right now, we know of no way for an attacker to force an arbitrary
variable name - ONLY arbitrary variable contents. So I would prefer a
patch that allows the export/import of the exact same set of names as
what can be used as valid function names. Neither set should be larger
than the other, and for back-compat purposes, at least in bash mode, the
set needs to STILL allow for functions named 'foo::bar' or 'foo/bar'.
After thinking a bit more, I would be in favor of a patch that forbids
'/' in function names, but opposed to a patch that forbids '.'. '/' has
special effects on PATH lookup, and it is just too confusing to think
that in bash mode, a function could override a command name containing a
slash (especially since that should NOT be possible in /bin/sh mode, for
POSIX compliance). ksh allows '.' in function names, so we would be
needlessly incompatible if we restrict that. As for other
non-metacharacters, I'm just fine with allowing pretty much anything
else (',', ':', '@', '-'), as none of them have special rules on shell
behavior.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
[I am terribly sorry that my In-Reply-To is wrong :/]
- Eric Blake ebl...@redhat.com 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.
$ env 'date;x=() { :;}' bash --norc -c :
Sat Sep 27 20:40:41 UTC 2014
Segmentation fault (core dumped)
$ bash --version | head -n 1
GNU bash, version 4.2.45(1)-release (x86_64-pc-linux-gnu)
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 to
verify it was a single word).
Re: REGRESSION: shellshock patch rejects valid function names
On 09/27/2014 02:57 PM, Arfrever Frehtes Taifersar Arahesis wrote:
2014-09-27 22:29 Eric Blake napisał(a):
Thankfully, bash already forbids trying to name a function 'a=b'
It works in bash 4.3.26:
$ function a=b() { echo A; }
$ a=b
A
Oh, you used the 'function' keyword, coupled with NOT quoting the name
of the function. I was trying to create the name via quotation:
$ bash -c 'a\=b () { echo hi; }; a\=b'
bash: `a\=b': not a valid identifier
bash: a=b: command not found
Still, since I strongly believe that import/export and valid function
names should be the same set of characters, but that it would be
extremely difficult to unambiguously encode = into environment variable
function names, I think that we SHOULD take the step of forbidding '='
in function names (the 'function' keyword should NOT be able to define
any function that cannot also be defined in isolation, nor which cannot
be imported from the environment using whatever improved implementation
we decide for function export/import).
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
On 09/27/2014 03:10 PM, Jay Freeman (saurik) wrote:
[I am terribly sorry that my In-Reply-To is wrong :/]
- Eric Blake ebl...@redhat.com 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
Re: REGRESSION: shellshock patch rejects valid function names
2014-09-27 23:12 Eric Blake napisał(a):
On 09/27/2014 02:57 PM, Arfrever Frehtes Taifersar Arahesis wrote:
2014-09-27 22:29 Eric Blake napisał(a):
Thankfully, bash already forbids trying to name a function 'a=b'
It works in bash 4.3.26:
$ function a=b() { echo A; }
$ a=b
A
Oh, you used the 'function' keyword, coupled with NOT quoting the name
of the function. I was trying to create the name via quotation:
$ bash -c 'a\=b () { echo hi; }; a\=b'
bash: `a\=b': not a valid identifier
bash: a=b: command not found
Still, since I strongly believe that import/export and valid function
names should be the same set of characters, but that it would be
extremely difficult to unambiguously encode = into environment variable
function names, I think that we SHOULD take the step of forbidding '='
in function names (the 'function' keyword should NOT be able to define
any function that cannot also be defined in isolation, nor which cannot
be imported from the environment using whatever improved implementation
we decide for function export/import).
Ability to export/import functions with = in function names could be achieved
by not embedding
function names in environmental variables and using a single BASH_FUNCTIONS
environmental
variable whose value would contain code of all exported functions (in format
similar to
`declare -fpx` / `export -fp`).
--
Arfrever Frehtes Taifersar Arahesis
signature.asc
Description: This is a digitally signed message part.
Re: REGRESSION: shellshock patch rejects valid function names
On 9/27/14, 4:29 PM, Eric Blake wrote: On 09/27/2014 12:53 PM, Chet Ramey wrote: Right now, we know of no way for an attacker to force an arbitrary variable name - ONLY arbitrary variable contents. Sure, but we didn't know that at the time. We still don't, really. So I would prefer a patch that allows the export/import of the exact same set of names as what can be used as valid function names. Neither set should be larger than the other, and for back-compat purposes, at least in bash mode, the set needs to STILL allow for functions named 'foo::bar' or 'foo/bar'. Yes, I don't think anyone disagrees that the set of allowed imported functions should not be different than the set of functions that a shell allows the user to define. I am not certain that a shell running in Posix mode should import a shell function with a name that it wouldn't let a user define. That's the point behind the original bash-4.3 check. The question is what *other* characters to disallow. I think there's a good case to be made for including `/', since it can be used to defeat the usual command lookup order. There might be others. Exactly, this break is undesirable. Remember, the security hole of Shell Shock is NOT what the function is named, but the fact that arbitrary variable contents were being parsed. Once you plug that hole by sticking function import/export in a separate namespace, then there should be no problem in allowing ALL function names that have always been previously allowed. We have an opportunity to close up a potential problem here, at least with respect to function names containing `/'. I'm 50:50 between two options: 1: Have two sets of valid names. In /bin/sh POSIX mode, functions can only be variable names, and when /bin/sh is importing from the env, it silently ignores or discards any mangled names that would not fit that restriction; meanwhile, invoking a function is only possible if the function name is a variable name; then in bash mode, functions can have any name and be invoked by that name That's what the original bash-4.3 code did, with the difference that it created `normal' shell variables, as would be done for any environment variable, for names that it wouldn't allow as shell functions. 2. Have only a single rule on what forms a valid name. POSIX doesn't forbid us from having an enlarged namespace for valid function names; a strictly conforming application won't use such unusual names. You may still want to ignore or scrub those out of the environment during /bin/sh importing, but that brings us back to the question of whether function imports should be automatic, or something the user has to explicitly request. If ALL function imports are skipped unless explicitly requested, then it doesn't matter WHAT function names are exported, the incoming functions in a child shell (regardless of whether the shell is in bash or /bin/sh mode) will have no impact to that child unless the script programmer of the parent asked to have functions imported. I'm not really interested in the function-importing-by-default discussion right now. Maybe sometime later. It's more of a break with backwards compatibility than I'm willing to go at this point. Thankfully, bash already forbids trying to name a function 'a=b' (having to support such a name would make env-var export/import rather difficult), so the main culprits that people seem to be worried about are '-', '.', ':', and '/'. The only one of these that really can cause a problem is `/', by `perverting' (heh) the usual command lookup. Even a script that thinks it's protecting itself by using full pathnames can be circumvented by a function name containing a slash, as in the example I gave. (And I'm not interested in rehashing decisions that were made 25 years ago, and I am completely aware that this violates Posix. That's why it doesn't do this in posix mode.) I don't see function export/import as a violation of posix; Yeah, but what I was talking about was allowing non-identifiers as valid function names. Chet -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
On 9/27/14, 4:57 PM, Arfrever Frehtes Taifersar Arahesis wrote:
2014-09-27 22:29 Eric Blake napisał(a):
Thankfully, bash already forbids trying to name a function 'a=b'
It works in bash 4.3.26:
$ function a=b() { echo A; }
$ a=b
A
$ a\=b
A
Yeah, and it even attempts to export it, but that doesn't work.
--
``The lyf so short, the craft so long to lerne.'' - Chaucer
``Ars longa, vita brevis'' - Hippocrates
Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
On 9/27/14, 5:23 PM, Eric Blake wrote: 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. We already do. That's the point of the call to legal_identifier(). -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
On 09/27/2014 05:59 PM, Chet Ramey wrote:
On 9/27/14, 4:29 PM, Eric Blake wrote:
On 09/27/2014 12:53 PM, Chet Ramey wrote:
Right now, we know of no way for an attacker to force an arbitrary
variable name - ONLY arbitrary variable contents.
Sure, but we didn't know that at the time. We still don't, really.
On the bright side: _if_ we find a way for an attacker to generate an
arbitrary variable name in a parent process before calling into bash,
that is more likely to be raised as a security bug against THAT program,
and not bash. The reason bash is getting the limelight for
CVE-2014-6271 is because there are so many existing programs that pass
untrusted input to environment variable values, where fixing bash solves
ALL of those cases at once; but there are a lot fewer (if any) programs
stupid enough to create arbitrary environment variable names.
The regression with 'at' failing to work is because 'at' is making
unportable assumptions that the environment it is passed will always be
parsable in shell. But this assumption is wrong whether or not bash
uses an alternate namespace for function exports - ANY C program that
uses execve can tickle that bug in 'at'. So that particular fix is not
bash's problem, but 'at's problem.
Exactly, this break is undesirable. Remember, the security hole of
Shell Shock is NOT what the function is named, but the fact that
arbitrary variable contents were being parsed. Once you plug that hole
by sticking function import/export in a separate namespace, then there
should be no problem in allowing ALL function names that have always
been previously allowed.
We have an opportunity to close up a potential problem here, at least
with respect to function names containing `/'.
As I said in a later mail, I'm now definitely leaning towards your
desire to exclude '/', and may I also add '=', as two characters that
will be blacklisted from valid function names both in the shell and
during import/export, because they are just too risky. I haven't yet
come up with any reason to blacklist any other non-metacharacter, and
you already reject any metacharacter that requires quoting.
I'm not really interested in the function-importing-by-default discussion
right now. Maybe sometime later. It's more of a break with backwards
compatibility than I'm willing to go at this point.
That's a fair stance. I don't mind waiting and revisiting that
discussion later; I agree that the first priority is to plug the
namespace issue.
(And I'm not interested in rehashing decisions that were made 25 years
ago, and I am completely aware that this violates Posix. That's why it
doesn't do this in posix mode.)
I don't see function export/import as a violation of posix;
Yeah, but what I was talking about was allowing non-identifiers as valid
function names.
Where does POSIX forbid the use of a non-identifier as a valid function
name? My understanding is that:
a:b () { ... }
is unspecified by POSIX (and NOT expressly required to be a syntax
error), which has two implications: 1. Strictly-conforming shell scripts
will not attempt to use it, and 2: we are allowed to support it as an
extension while still remaining POSIX compliant. I'd need to see an
example of some script that would be parsed one way with bash's function
name extensions and a different well-defined way by a strict reading of
POSIX before stating that /bin/sh mode must behave differently at having
a smaller subset of valid function names.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
On 9/27/14, 8:49 PM, Eric Blake wrote: On 09/27/2014 05:59 PM, Chet Ramey wrote: On 9/27/14, 4:29 PM, Eric Blake wrote: On 09/27/2014 12:53 PM, Chet Ramey wrote: We have an opportunity to close up a potential problem here, at least with respect to function names containing `/'. As I said in a later mail, I'm now definitely leaning towards your desire to exclude '/', and may I also add '=', as two characters that will be blacklisted from valid function names both in the shell and during import/export, because they are just too risky. I haven't yet come up with any reason to blacklist any other non-metacharacter, and you already reject any metacharacter that requires quoting. I'm just going to include slash for right now, since `=' doesn't cause any real problems, because you're effectively not able to export it with the current version. Yeah, but what I was talking about was allowing non-identifiers as valid function names. Where does POSIX forbid the use of a non-identifier as a valid function name? Yes, it's an application requirement. Regardless, all the versions of bash we're talking about here reject non-identifiers. -- ``The lyf so short, the craft so long to lerne.'' - Chaucer ``Ars longa, vita brevis'' - Hippocrates Chet Ramey, ITS, CWRUc...@case.eduhttp://cnswww.cns.cwru.edu/~chet/
Re: REGRESSION: shellshock patch rejects valid function names
On 09/27/2014 07:47 PM, Chet Ramey wrote:
On 9/27/14, 8:49 PM, Eric Blake wrote:
On 09/27/2014 05:59 PM, Chet Ramey wrote:
On 9/27/14, 4:29 PM, Eric Blake wrote:
On 09/27/2014 12:53 PM, Chet Ramey wrote:
We have an opportunity to close up a potential problem here, at least
with respect to function names containing `/'.
As I said in a later mail, I'm now definitely leaning towards your
desire to exclude '/', and may I also add '=', as two characters that
will be blacklisted from valid function names both in the shell and
during import/export, because they are just too risky. I haven't yet
come up with any reason to blacklist any other non-metacharacter, and
you already reject any metacharacter that requires quoting.
I'm just going to include slash for right now, since `=' doesn't cause
any real problems, because you're effectively not able to export it with
the current version.
Bash is not able to export it _as a function_, and likewise, cannot
import it as a function. But the problem is that bash DOES try to
export it, and results in instead POLLUTING the child's namespace.
Consider this behavior in bash 4.3.24:
$ bash -c 'function a=b(){ echo oops;};export -f a=b;export a=hi; bash
-c echo \$a'
b=() { echo oops }
or in Fedora bash-4.2.48-2.fc20.x86_64:
$ bash -c 'function a=b(){ echo oops;};export -f a=b;export
BASH_FUNC_a=hi; bash -c echo \$BASH_FUNC_a'
b()=() { echo oops }
In both cases, your attempt to export an invalid function name ended up
clobbering a regular variable.
Please reconsider, and prohibit the use of = in function names both for
the 'function' keyword and on imports from the environment.
Yeah, but what I was talking about was allowing non-identifiers as valid
function names.
Where does POSIX forbid the use of a non-identifier as a valid function
name?
Yes, it's an application requirement. Regardless, all the versions of bash
we're talking about here reject non-identifiers.
I'm still trying to find that line in the actual POSIX spec.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
On Fri, Sep 26, 2014 at 08:08:23PM +, Jay Freeman (saurik) wrote: Google's style guide for shell actually states that using :: in function names to separate library names from function names and package names within library names is their best practice. My respect for Google just dropped. More.
Re: REGRESSION: shellshock patch rejects valid function names
On 09/26/2014 02:08 PM, Jay Freeman (saurik) wrote: [can you convince your mailer to wrap long lines?] Lower-case, with underscores to separate words. Separate libraries with ::. Parentheses are required after the function name. The keyword function is optional, but must be used consistently throughout a project. If you're writing a package, separate package names with ::. Lower-case, with underscores to separate words. Separate libraries with ::. Parentheses are required after the function name. The keyword function is optional, but must be used consistently throughout a project. If you're writing a package, separate package names with ::. Ugg. Anyone advising people to write bash functions while using the obsolete keyword 'function' ought to be questioned about their advice in general. At any rate, this seems like an inadvertent regression that could be patched; are you willing to propose such a patch? The gist of the matter is that the code base must use the same decision on what forms a valid function name as it does in deciding what exported non-variable names in the environment can be reinstated as functions. I'm fairly certain that Chet will be reasonable about this, and after the worst fires are put out, we can revisit this. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: REGRESSION: shellshock patch rejects valid function names
Eric Blake wrote: At any rate, this seems like an inadvertent regression that could be patched; are you willing to propose such a patch? The gist of the matter is that the code base must use the same decision on what forms a valid function name as it does in deciding what exported non-variable names in the environment can be reinstated as functions. I'm fairly certain that Chet will be reasonable about this, and after the worst fires are put out, we can revisit this. The patch seems straightforward: diff --git a/variables.c b/variables.c index 92a5a10..6552e69 100644 --- a/variables.c +++ b/variables.c @@ -361,7 +361,7 @@ initialize_shell_variables (env, privmode) /* Don't import function names that are invalid identifiers from the environment, though we still allow them to be defined as shell variables. */ - if (legal_identifier (name)) + if (check_identifier (name)) parse_and_execute (temp_string, name, SEVAL_NONINT|SEVAL_NOHIST|SEVAL_FUNCDEF|SEVAL_ONECMD); if (temp_var = find_function (name))
Re: REGRESSION: shellshock patch rejects valid function names
- Ángel González an...@16bits.net wrote:
The patch seems straightforward:
diff --git a/variables.c b/variables.c
index 92a5a10..6552e69 100644
--- a/variables.c
+++ b/variables.c
@@ -361,7 +361,7 @@ initialize_shell_variables (env, privmode)
...
- if (legal_identifier (name))
+ if (check_identifier (name))
...
This patch would not work (it would not even compile if you tried it, in fact
;P) because check_identifier takes two arguments: the second argument is
whether to internally run legal_identifier on the name. Additionally,
check_argument takes a WORD_DESC, not a string.
If these two issues were fixed (by using make_word or something, and then
passing 0 or preferably posixly_correct), I am concerned that this might
undermine the security fix itself, as check_identifier could potentially allow
ludicrous things to be pasted in the name?
It seems, however, like SEVAL_FUNCDEF is sufficient to keep most things that
should not happen from actually happening? I tested the attached patch against
the following test case. It might be that I went overboard and don't actually
need quote_escapes (I'm new to bash).
(function std:echo() { echo $@; }; export -f std:echo; env 'x$(date)=() {
:;}' 'x`date`=() { :;}' 'date;x=() { date;}' ./bash --norc -c 'std:echo hello
world')
./bash: `x$(date)': not a valid identifier
./bash: error importing function definition for `x$(date)'
./bash: `x`date`': not a valid identifier
./bash: error importing function definition for `x`date`'
./bash: warning: date;x: ignoring function definition attempt
./bash: error importing function definition for `date;x'
hello world
colon-functions-attempt-1.diff
Description: Binary data
Re: REGRESSION: shellshock patch rejects valid function names
On Friday, September 26, 2014 02:17:03 PM Eric Blake wrote: Ugg. Anyone advising people to write bash functions while using the obsolete keyword 'function' ought to be questioned about their advice in general. It's mainly the combination of `function` and `()` that make no sense in any context. I usually prefer the function keyword especially for bash functions, but it's entirely for portability reasons that not a lot of people care about. (this opinion could also change if the ksh dynamic scope option becomes less buggy before the next version, or dash were to make some improvements to make me care about its locals, among other things.) -- Dan Douglas signature.asc Description: This is a digitally signed message part.
