Re: binary-patching bash

[Solar Designer <solar () openwall com>]

On Mon, Sep 29, 2014 at 04:44:05AM +0400, Solar Designer wrote:
> The primary risk I see here is that some build of bash might include
> custom patches where this check had been changed to use something other
> than (an equivalent of) strncmp().  I am not aware of any such cases.

Another concern is that (previously custom-patched) code following the
check might jump over the constant string (or a portion of it) that it
thinks has just been matched.  I am not aware of any such cases, and this
is not true in at least bash 1.14.7 and bash 4.3.  In 1.14.7, we have:

      if (!privileged_mode && STREQN ("() {", string, 4))
        {
          SHELL_VAR *f;
          char *eval_string;

          eval_string = xmalloc (3 + string_length + strlen (name));
          sprintf (eval_string, "%s %s", name, string);

          parse_and_execute (eval_string, name, 0);

In 4.3, we have:

      if (privmode == 0 && read_but_dont_execute == 0 && STREQN ("() {", string, 4))
        {
          string_length = strlen (string);
          temp_string = (char *)xmalloc (3 + string_length + char_index);

          strcpy (temp_string, name);
          temp_string[char_index] = ' ';
          strcpy (temp_string + char_index + 1, string);

          if (posixly_correct == 0 || legal_identifier (name))
            parse_and_execute (temp_string, name, SEVAL_NONINT|SEVAL_NOHIST);

As we can see, "string" is used with no offset, despite of its 4 chars
having already been checked.  That's good.  I hope it's the same for all
other revisions of the code, which is natural because "() {" is part of
the needed input to the parser anyway, but some risk is there.

Alexander