Re: BUG: crafting SHELLOPTS and PS4 allows to run arbitrary programs in setuid binaries using system

[Peter Stephenson <p.stephenson@xxxxxxxxxxx>]

On Tue, 27 Sep 2016 07:53:47 +0000
Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> Mateusz Lenik wrote on Tue, Sep 27, 2016 at 06:59:18 +0000:
> > % gcc -xc - -otest <<< 'int main() { setuid(0); system("/bin/date"); }'
> > % sudo chown root:root test
> > % sudo chmod 4755 test
> > % env -i SHELLOPTS=xtrace PS4='$(id)' ./test
> > uid=0(root) gid=... groups=.../bin/date
> > Tue Sep 27 08:49:16 CEST 2016
> 
> I can't reproduce that either either 5.0.7 or latest master, even with
> «setopt promptsubst» in effect.  (Does it reproduce in 'zsh -f'?)

While I can believe there's a problem here, since there's pretty similar
logic around in zsh, SHELLOPTS does nothing with zsh, so I suspect the
case is different if it does show up.

> > The solution that bash folks implemented is to drop PS4 from env when the
> > shell is ran as root.
> 
> 34015 (89012cf94ca) stopped importing non-ASCII envvars.  There may have
> been other changes in this area but I couldn't quickly find them.

I don't think we make any special arrangements for import as root,
currently.  It's straightforward to do so.

I've attempted to tidy up the logic to the point where I think I
understand it.  Does the test "(!getuid() || !geteuid())" make sense or
should that be something else?

pws

diff --git a/Src/params.c b/Src/params.c
index 384c30a..a85e5a5 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -333,6 +333,7 @@ IPDEF6("TRY_BLOCK_ERROR", &try_errflag, varinteger_gsu),
 IPDEF6("TRY_BLOCK_INTERRUPT", &try_interrupt, varinteger_gsu),
 
 #define IPDEF7(A,B) {{NULL,A,PM_SCALAR|PM_SPECIAL},BR((void *)B),GSU(varscalar_gsu),0,0,NULL,NULL,NULL,0}
+#define IPDEF7R(A,B) {{NULL,A,PM_SCALAR|PM_SPECIAL|PM_DONTIMPORT_ROOT},BR((void *)B),GSU(varscalar_gsu),0,0,NULL,NULL,NULL,0}
 #define IPDEF7U(A,B) {{NULL,A,PM_SCALAR|PM_SPECIAL|PM_UNSET},BR((void *)B),GSU(varscalar_gsu),0,0,NULL,NULL,NULL,0}
 IPDEF7("OPTARG", &zoptarg),
 IPDEF7("NULLCMD", &nullcmd),
@@ -345,7 +346,7 @@ IPDEF7("PS2", &prompt2),
 IPDEF7U("RPS2", &rprompt2),
 IPDEF7U("RPROMPT2", &rprompt2),
 IPDEF7("PS3", &prompt3),
-IPDEF7("PS4", &prompt4),
+IPDEF7R("PS4", &prompt4),
 IPDEF7("SPROMPT", &sprompt),
 
 #define IPDEF8(A,B,C,D) {{NULL,A,D|PM_SCALAR|PM_SPECIAL},BR((void *)B),GSU(colonarr_gsu),0,0,NULL,C,NULL,0}
@@ -689,7 +690,28 @@ split_env_string(char *env, char **name, char **value)
     } else
 	return 0;
 }
-    
+
+/**
+ * Check parameter flags to see if parameter shouldn't be imported
+ * from environment at start.
+ *
+ * return 1: don't import: 0: ok to import.
+ */
+static int dontimport(int flags)
+{
+    /* If explicitly marked as don't export */
+    if (flags & PM_DONTIMPORT)
+	return 1;
+    /* If value already exported */
+    if (flags & PM_EXPORTED)
+	return 1;
+    /* If security issue when exporting as root */
+    if ((flags & PM_DONTIMPORT_ROOT) && (!getuid() || !geteuid()))
+	return 1;
+    /* OK to import */
+    return 0;
+}
+
 /* Set up parameter hash table.  This will add predefined  *
  * parameter entries as well as setting up parameter table *
  * entries for environment variables we inherit.           */
@@ -781,8 +803,13 @@ createparamtable(void)
 	    envp2 = environ; *envp2; envp2++) {
 	if (split_env_string(*envp2, &iname, &ivalue)) {
 	    if (!idigit(*iname) && isident(iname) && !strchr(iname, '[')) {
+		/*
+		 * Parameters that aren't already in the parameter table
+		 * aren't special to the shell, so it's always OK to
+		 * import.  Otherwise, check parameter flags.
+		 */
 		if ((!(pm = (Param) paramtab->getnode(paramtab, iname)) ||
-		     !(pm->node.flags & PM_DONTIMPORT || pm->node.flags & PM_EXPORTED)) &&
+		     !dontimport(pm->node.flags)) &&
 		    (pm = assignsparam(iname, metafy(ivalue, -1, META_DUP),
 				       ASSPM_ENV_IMPORT))) {
 		    pm->node.flags |= PM_EXPORTED;
diff --git a/Src/zsh.h b/Src/zsh.h
index bb8ce13..052d754 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1802,6 +1802,7 @@ struct tieddata {
 #define PM_ZSHSTORED	(1<<18) /* function stored in zsh form              */
 
 /* Remaining flags do not correspond directly to command line arguments */
+#define PM_DONTIMPORT_ROOT (1<<19) /* do not import if running as root */
 #define PM_SINGLE       (1<<20) /* special can only have a single instance  */
 #define PM_LOCAL	(1<<21) /* this parameter will be made local        */
 #define PM_SPECIAL	(1<<22) /* special builtin parameter                */