REGRESSION: shellshock patch rejects valid function names

[Jay Freeman (saurik)]

(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)
saurik@saurik.com