REGRESSION: shellshock patch rejects valid function names
Hello! ;P Yesterday, I upgraded to a new version of bash (from Debian),
intended to fix "shellshock". Today, I'm finding that all of my build scripts
have stopped working :(. I managed to narrow the breaking change to yesterday's
patch: the problem is that the names of functions are allowed to contain
colons, but the new patch refuses to import these exported functions.
The code issue is that the patch uses valid_identifier(name), when
check_identifier(name, posixly_correct) is what is normally used for function
names. (In the case of export, the check is "does this function exist", not "is
this identifier valid".) The means that suddenly only a tiny subset of
functions all previous version of bash had supported are now exportable :(.
As far as I can tell, the goal is to avoid accidentally executing embedded
shell that might be in the name of the function? It would seem to me that a
better way to go about this would be to run the parser on the name and verify
that what comes back is a single WORD_DESC that can be given to
check_identifier (but of course, I am nowhere near as versed in the bash code
as any of you).
Below, I have included a code-oriented explanation of the incompatibility
between these checks (based on bash 4.2, which is the version I got today from
Debian; for the record, I had previously been running bash 3.2). I would be
happy to attempt to help in any other way that seems relevant (such as if a
maintainer has a general approach they agree with and would like to see
attempted).
(Regardless, thanks to anyone who chooses to look at this issue, or even just
read this e-mail ;P.)
>From execute_cmd.c:
5100 execute_intern_function (name, function)
5101 WORD_DESC *name;
5102 COMMAND *function;
5103 {
5104 SHELL_VAR *var;
5105
=> 5106 if (check_identifier (name, posixly_correct) == 0)
5107 {
5108 if (posixly_correct && interactive_shell == 0)
5109 {
5110 last_command_exit_value = EX_BADUSAGE;
5111 jump_to_top_level (ERREXIT);
5112 }
5113 return (EXECUTION_FAILURE);
5114 }
In the case where posixly_correct is false, check_identifier will allow colons
in the identifier names.
224 int
225 check_identifier (word, check_word)
226 WORD_DESC *word;
227 int check_word;
228 {
229 if ((word->flags & (W_HASDOLLAR|W_QUOTED)) || all_digits (word->word))
230 {
231 internal_error (_("`%s': not a valid identifier"), word->word);
232 return (0);
233 }
=> 234 else if (check_word && legal_identifier (word->word) == 0)
235 {
236 internal_error (_("`%s': not a valid identifier"), word->word);
237 return (0);
238 }
239 else
240 return (1);
241 }
>From the new shellshock patch:
350 /* Don't import function names that are invalid identifiers
from the
351 environment. */
=> 352 if (legal_identifier (name))
353 parse_and_execute (temp_string, name,
SEVAL_NONINT|SEVAL_NOHIST|SEVAL_FUNCDEF|SEVAL_ONECMD);
This check is thereby incompatible with existing behavior and scripts, and
leads to situations where both function and export -f are happy, but running
bash causes a function import error. As mentioned, I do not believe that this
is required to be secure, so it would seem a shame to break scripts that have
been functioning without issue for almost a decade.
$ bash --norc
$ function std:echo() { echo "$@"; }
$ std:echo hello world
hello world
$ export -f std:echo
$ printenv | grep std:echo -A 1
std:echo=() { echo "$@"
}
$ bash --norc
bash: error importing function definition for `std:echo'
$ std:echo hello world
bash: std:echo: command not found
Sincerely,
Jay Freeman (saurik)
sau...@saurik.com
REGRESSION: shellshock patch rejects valid function names
(I am terribly sorry if this e-mail message comes through twice; I normally use
a third-party SMTP relay service, but it marks my e-mail with various headers,
including precedence bulk, that I think might have caused my first e-mail to
have been eaten by your mailing list manager, as I did not see this e-mail
appear in the archive. I've bypassed that service for this attempt.)
Hello! ;P Yesterday, I upgraded to a new version of bash (from Debian),
intended to fix "shellshock". Today, I'm finding that all of my build scripts
have stopped working :(. I managed to narrow the breaking change to yesterday's
patch: the problem is that the names of functions are allowed to contain
colons, but the new patch refuses to import these exported functions.
The code issue is that the patch uses valid_identifier(name), when
check_identifier(name, posixly_correct) is what is normally used for function
names. (In the case of export, the check is "does this function exist", not "is
this identifier valid".) The means that suddenly only a tiny subset of
functions all previous version of bash had supported are now exportable :(.
As far as I can tell, the goal is to avoid accidentally executing embedded
shell that might be in the name of the function? It would seem to me that a
better way to go about this would be to run the parser on the name and verify
that what comes back is a single WORD_DESC that can be given to
check_identifier (but of course, I am nowhere near as versed in the bash code
as any of you).
Below, I have included a code-oriented explanation of the incompatibility
between these checks (based on bash 4.2, which is the version I got today from
Debian; for the record, I had previously been running bash 3.2). I would be
happy to attempt to help in any other way that seems relevant (such as if a
maintainer has a general approach they agree with and would like to see
attempted).
(Regardless, thanks to anyone who chooses to look at this issue, or even just
read this e-mail ;P.)
>From execute_cmd.c:
5100 execute_intern_function (name, function)
5101 WORD_DESC *name;
5102 COMMAND *function;
5103 {
5104 SHELL_VAR *var;
5105
=> 5106 if (check_identifier (name, posixly_correct) == 0)
5107 {
5108 if (posixly_correct && interactive_shell == 0)
5109 {
5110 last_command_exit_value = EX_BADUSAGE;
5111 jump_to_top_level (ERREXIT);
5112 }
5113 return (EXECUTION_FAILURE);
5114 }
In the case where posixly_correct is false, check_identifier will allow colons
in the identifier names.
224 int
225 check_identifier (word, check_word)
226 WORD_DESC *word;
227 int check_word;
228 {
229 if ((word->flags & (W_HASDOLLAR|W_QUOTED)) || all_digits (word->word))
230 {
231 internal_error (_("`%s': not a valid identifier"), word->word);
232 return (0);
233 }
=> 234 else if (check_word && legal_identifier (word->word) == 0)
235 {
236 internal_error (_("`%s': not a valid identifier"), word->word);
237 return (0);
238 }
239 else
240 return (1);
241 }
>From the new shellshock patch:
350 /* Don't import function names that are invalid identifiers
from the
351 environment. */
=> 352 if (legal_identifier (name))
353 parse_and_execute (temp_string, name,
SEVAL_NONINT|SEVAL_NOHIST|SEVAL_FUNCDEF|SEVAL_ONECMD);
This check is thereby incompatible with existing behavior and scripts, and
leads to situations where both function and export -f are happy, but running
bash causes a function import error. As mentioned, I do not believe that this
is required to be secure, so it would seem a shame to break scripts that have
been functioning without issue for almost a decade.
$ bash --norc
$ function std:echo() { echo "$@"; }
$ std:echo hello world
hello world
$ export -f std:echo
$ printenv | grep std:echo -A 1
std:echo=() { echo "$@"
}
$ bash --norc
bash: error importing function definition for `std:echo'
$ std:echo hello world
bash: std:echo: command not found
Sincerely,
Jay Freeman (saurik)
sau...@saurik.com
Re: REGRESSION: shellshock patch rejects valid function names
- "Eduardo A. Bustamante López" 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?
Re: REGRESSION: shellshock patch rejects valid function names
- "Ángel González" 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
[I am terribly sorry that my In-Reply-To is wrong :/]
- "Eric Blake" 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: backward-kill-word is not refreshing correctly
On Mar 18, 6:55 pm, m...@ice.filescope.com, zy...@ice.filescope.com wrote: ... > When I type a long string of text and start pressing ctrl-W to > backwards-kill words, bash deletes the words but doesn't visually refresh > (the words still appear on the command line). This was not occurring for me > in the 3.x series of Bash. > > Repeat-By: > Write this text on the command line: "sample text sample text sample > text sample text sample text". Start pressing ctrl-W and notice it takes a > few kill-words before the words start disappearing. > > --Matt Zyzik I having a problem similar to this. For me, kill-word /never/ refreshes the screen. Additionally, moving up/down through the command history doesn't always cause a refresh either (in a manner deterministic but stochastic). I am using 4.0.17 on arm-apple-darwin9. I also cannot reproduce this problem with bash 3.2. Unfortunately, I cannot reproduce this problem on x86_64-unknown-linux-gnu. I have tried building bash 4.x against ncurses 5.4 and 5.7, I have tried compiling it against a standalone readline 6.x and using a built- in copy, and I have tried compiling both for thumb and arm. I also have tried copying the terminfo database off of my working x86_64 box and I have even tried running without any terminfo database, in the hope of determining why I can't reproduce the problem on my server. Does anyone have any hints on what I could do to debug this? What sections of code this might be using? I know very little about how bash/readline/ncurses work together to handle this problem: even just saying "check out these couple functions" could be useful, so I could start seeing if they are getting called correctly.
Re: backward-kill-word is not refreshing correctly
> > When I type a long string of text and start pressing ctrl-W to > > backwards-kill words, bash deletes the words but doesn't visually refresh > > (the words still appear on the command line). This was not occurring for me > > in the 3.x series of Bash. > I having a problem similar to this. For me, kill-word /never/ > refreshes the screen. Additionally, moving up/down through the command > history doesn't always cause a refresh either (in a manner > deterministic but stochastic). ... > I have tried building bash 4.x against ncurses 5.4 and 5.7, I have > tried compiling it against a standalone readline 6.x and using a built- > in copy, and I have tried compiling both for thumb and arm. And, it turns out that had I tested a standalone readline 5.2, it would have worked fine ;P. The problem was introduced somewhere in readline between bash 3.2 and 4.0. I figured this out by playing with bash some to figure out where it was displaying things to the screen, and then started whittling away at the diff of readline's display.c from 3.2 to 4.0. In the end I managed to isolate it down to the single diff hunk that was causing the problem. It should be made clear that I have no clue what this code is doing, or why it was changed from 3.2 to 4.0: all I know is that the old version worked for me, and this one did not. I am therefore not advocating that removing it is the "correct" fix, and am only providing this information in the hope it will help debug the issue. So, this patch solves my problem, and maybe it will solve Matt's as well: http://svn.telesphoreo.org/trunk/data/readline/lendiff.diff While I'm at it, I highly recommend removing the extraneous CFLAGS overrides from readline's configure.in: they break autoconf's gcc -g detection, if nothing else. http://svn.telesphoreo.org/trunk/data/readline/cflags.diff Sincerely, Jay Freeman (saurik) sau...@saurik.com http://www.saurik.com/
Re: backward-kill-word is not refreshing correctly
And help I will definitely attempt. Here are the requested values: col_lendiff = 5 _rl_last_c_pos = 194 _rl_screenwidth = 126 Some random other variables, in case they end up being requested: lendiff = 5 old = 8403456 new = 8457216 ne = 8457410 oe = 8403655 temp = 0 My cursor is definitely not past the end of my screen. This seems to be a bug in the way prompts are handled. I have a ludicrously complex prompt that has a large number of non-printing characters. This issue then only occurs (as one would expect) if the math works out so the total number of characters in the prompt (/including non-printing ones/) plus my true cursor offset from the prompt is past the edge of the screen, but isn't /actually/. And yes, I am definitely being very careful about using \[ and \] to bound my non-printable characters. I even got it down to this very trivial test case, where ... indicates "more numbers to fill space here", so I could easily see "yes, the \[ and \] are placed correctly". ;P PS1=$'0123456789...0123456789\\[\e[0m\\]' However, even given this knowledge, the bug remains unreproducable on my amd64 box. I have, however, found someone using FreeBSD who said he was seeing related-sounding issues with Bash 4.x, and I know he's also using colored prompts. I am also now confident this is the same problem Matt was having, as once he had killed enough words to get out of the danger region (his prompt was probably not as ludicrous as mine), it started working again. Also, while looking into this (in the hope of reproducing it on Linux), I noticed that in builtins/Makefile.in, LDFLAGS_FOR_BUILD uses LDFLAGS which uses CFLAGS... which is the CFLAGS for the target, not for the build. This became a problem as I tried to do a 32-bit build of bash (using CFLAGS='-O2 -m32 -g') and mkbuiltins.o was compiled without -m32 (as it uses CCFLAGS_FOR_BUILD, which bypasses CFLAGS to use BASE_CCFLAGS) but was being linked with -m32. I do not yet have a correct fix for this unrelated issue. Sincerely, Jay Freeman (saurik) sau...@saurik.com http://www.saurik.com/ - Original Message ----- From: "Chet Ramey" To: "Jay Freeman (saurik)" Cc: ; ; Sent: Saturday, April 11, 2009 9:14 AM Subject: Re: backward-kill-word is not refreshing correctly ... I can't reproduce the problem, and it seems to only appear in builds for your iphone. I'm going to need help with it. :-) Please check the values of _rl_last_c_pos and _rl_screenwidth when this code is executed and the problem occurs. It's likely that the former is set incorrectly somewhere, but I can't tell that for sure yet. Chet -- ``The lyf so short, the craft so long to lerne.'' - Chaucer Chet Ramey, ITS, CWRUc...@case.edu http://cnswww.cns.cwru.edu/~chet/
Re: pipefail with SIGPIPE/EPIPE
> Errexit (a.k.a. set -e) is horrible, > and you should not be using it in any new shell scripts you write. > It exists solely for support of legacy scripts. Wow. For those of us who don't know this, what should we be using instead? Is using a trap on ERR any better? Is your suggestion that || exit 1 be added to the end of every command? (Potentially of mild interest is this thread on Hacker News from earlier today, where multiple people are suggesting the usage of "set -e" along with "set -u" and "set -o pipefile".) https://news.ycombinator.com/item?id=13940322
