bash ShellShock patch from Florian Weimer

bash-shellshock-fix.patch

Date: Thu, 25 Sep 2014 14:37:26 +0200
From: Florian Weimer <[email protected]>
To: [email protected]
CC: [email protected]
Subject: Re: CVE-2014-6271: remote code execution through bash

On 09/24/2014 08:54 PM, Michal Zalewski wrote:
>> My main concern with the current patch is that still exposes the bash parser
>> and function definition printer to attacks from the network. Bugs in those
>> fairly large components could cause another critical issue.
>
> Yup, that surprised me when testing the patch, too - I can still get a
> function called HTTP_COOKIE, for example. I worry about potential side
> effects of parsing even in absence of parser bugs. In most
> object-oriented languages, such side effects are practically
> guaranteed. Bash may be saved by simplicity, but not sure how robust
> that assumption is.

The parser does make an effort to properly stage all operations for 
later execution, without executing them immediately.

There is certainly the question of incomplete state recovery on parse 
errors.

> I've written more code in bash than I should have and never used
> function exports, or even realized that they exist. I wonder if they
> can be made optional (e.g., gated by a flag on the subprocess) without
> breakage.

I've been told that there are users.  From what I can see, exported 
functions seem somewhat popular in test harnesses:

   <http://codesearch.debian.net/search?q=export\+-f>

Reportedly, some users even create the function definitions outside 
bash, so they rely function name and variable name being identical.  But 
I honestly cannot see a way to preserve such an assumption.

A subprocess flag is unlikely to be present when it is needed.  One 
common use case is to define and export functions in bash.profile, and 
expect them to exist in interactive shells created as grandchildren. 
The fix is to use bashrc instead of bash.profile and non-exported functions.

> Another option may be to export them through specially prefixed
> variables, which should be transparent but minimize the risk of
> interfering with web servers and such.

I added suffixes as well, see the attached patch.  This patch has seen 
some testing, but it certainly needs more.  There are some possibilities 
for simplification if it's acceptable to use asprintf.

What do you think about this approach?

(Chet, this patch is identical to the patch I sent to you a couple of 
minutes ago.)

-- 
Florian Weimer / Red Hat Product Security

--- ../bash-4.2-orig/variables.c	2014-09-25 13:07:59.313209541 +0200
+++ variables.c	2014-09-25 13:15:29.869420719 +0200
@@ -268,7 +268,7 @@
 static void propagate_temp_var __P((PTR_T));
 static void dispose_temporary_env __P((sh_free_func_t *));     
 
-static inline char *mk_env_string __P((const char *, const char *));
+static inline char *mk_env_string __P((const char *, const char *, int));
 static char **make_env_array_from_var_list __P((SHELL_VAR **));
 static char **make_var_export_array __P((VAR_CONTEXT *));
 static char **make_func_export_array __P((void));
@@ -301,6 +301,14 @@
 #endif
 }
 
+/* Prefix and suffix for environment variable names which contain
+   shell functions. */
+#define FUNCDEF_PREFIX "BASH_FUNC_"
+#define FUNCDEF_PREFIX_LEN (strlen (FUNCDEF_PREFIX))
+#define FUNCDEF_SUFFIX "()"
+#define FUNCDEF_SUFFIX_LEN (strlen (FUNCDEF_SUFFIX))
+
+
 /* Initialize the shell variables from the current environment.
    If PRIVMODE is nonzero, don't import functions from ENV or
    parse $SHELLOPTS. */
@@ -338,27 +346,39 @@
 
       /* If exported function, define it now.  Don't import functions from
 	 the environment in privileged mode. */
-      if (privmode == 0 && read_but_dont_execute == 0 && STREQN ("() {", string, 4))
-	{
-	  string_length = strlen (string);
-	  temp_string = (char *)xmalloc (3 + string_length + char_index);
+      if (privmode == 0 && read_but_dont_execute == 0
+	  && STREQN (FUNCDEF_PREFIX, name, FUNCDEF_PREFIX_LEN)
+	  && STREQ (name + char_index - FUNCDEF_SUFFIX_LEN, FUNCDEF_SUFFIX)
+	  && STREQN ("() {", string, 4))
+	{
+	  size_t name_length
+	    = char_index - (FUNCDEF_PREFIX_LEN + FUNCDEF_SUFFIX_LEN);
+	  char *temp_name = name + FUNCDEF_PREFIX_LEN;
+	  /* Temporarily remove the suffix. */
+	  temp_name[name_length] = '\0';
 
-	  strcpy (temp_string, name);
-	  temp_string[char_index] = ' ';
-	  strcpy (temp_string + char_index + 1, string);
+	  string_length = strlen (string);
+	  temp_string = (char *)xmalloc (name_length + 1 + string_length + 1);
+	  memcpy (temp_string, temp_name, name_length);
+	  temp_string[name_length] = ' ';
+	  memcpy (temp_string + name_length + 1, string, string_length + 1);
 
 	  /* Don't import function names that are invalid identifiers from the
 	     environment. */
-	  if (legal_identifier (name))
-	    parse_and_execute (temp_string, name, SEVAL_NONINT|SEVAL_NOHIST|SEVAL_FUNCDEF|SEVAL_ONECMD);
+	  if (legal_identifier (temp_name))
+	    parse_and_execute (temp_string, temp_name,
+			       SEVAL_NONINT|SEVAL_NOHIST|SEVAL_FUNCDEF|SEVAL_ONECMD);
 
-	  if (temp_var = find_function (name))
+	  if (temp_var = find_function (temp_name))
 	    {
 	      VSETATTR (temp_var, (att_exported|att_imported));
 	      array_needs_making = 1;
 	    }
 	  else
 	    report_error (_("error importing function definition for `%s'"), name);
+
+	  /* Restore the original suffix. */
+	  temp_name[name_length] = FUNCDEF_SUFFIX[0];
 	}
 #if defined (ARRAY_VARS)
 #  if 0
@@ -2537,7 +2557,7 @@
   var->context = variable_context;	/* XXX */
 
   INVALIDATE_EXPORTSTR (var);
-  var->exportstr = mk_env_string (name, value);
+  var->exportstr = mk_env_string (name, value, 0);
 
   array_needs_making = 1;
 
@@ -3388,22 +3408,43 @@
 /*								    */
 /* **************************************************************** */
 
+/* Returns the string NAME=VALUE if !FUNCTIONP or if VALUE == NULL (in
+   which case it is treated as empty).  Otherwise, decorate NAME with
+   FUNCDEF_PREFIX and FUNCDEF_SUFFIX, and return a string of the form
+   FUNCDEF_PREFIX NAME FUNCDEF_SUFFIX = VALUE (without spaces).  */
 static inline char *
-mk_env_string (name, value)
+mk_env_string (name, value, functionp)
      const char *name, *value;
+     int functionp;
 {
-  int name_len, value_len;
-  char	*p;
+  size_t name_len, value_len;
+  char *p, *q;
 
   name_len = strlen (name);
   value_len = STRLEN (value);
-  p = (char *)xmalloc (2 + name_len + value_len);
-  strcpy (p, name);
-  p[name_len] = '=';
+  if (functionp && value != NULL)
+    {
+      p = (char *)xmalloc (FUNCDEF_PREFIX_LEN + name_len + FUNCDEF_SUFFIX_LEN
+			   + 1 + value_len + 1);
+      q = p;
+      memcpy (q, FUNCDEF_PREFIX, FUNCDEF_PREFIX_LEN);
+      q += FUNCDEF_PREFIX_LEN;
+      memcpy (q, name, name_len);
+      q += name_len;
+      memcpy (q, FUNCDEF_SUFFIX, FUNCDEF_SUFFIX_LEN);
+      q += FUNCDEF_SUFFIX_LEN;
+    }
+  else
+    {
+      p = (char *)xmalloc (name_len + 1 + value_len + 1);
+      memcpy (p, name, name_len);
+      q = p + name_len;
+    }
+  q[0] = '=';
   if (value && *value)
-    strcpy (p + name_len + 1, value);
+    memcpy (q + 1, value, value_len + 1);
   else
-    p[name_len + 1] = '\0';
+    q[1] = '\0';
   return (p);
 }
 
@@ -3489,7 +3530,7 @@
 	  /* Gee, I'd like to get away with not using savestring() if we're
 	     using the cached exportstr... */
 	  list[list_index] = USE_EXPORTSTR ? savestring (value)
-					   : mk_env_string (var->name, value);
+	    : mk_env_string (var->name, value, function_p (var));
 
 	  if (USE_EXPORTSTR == 0)
 	    SAVE_EXPORTSTR (var, list[list_index]);