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

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

[Mateusz Lenik]

[-- Attachment #1: Type: text/plain, Size: 625 bytes --]

Hello everyone!

I just learned that bash fixed a vulnerability that also affects zsh. It
allowed to run arbitrary programs by crafting SHELLOPTS and PS4 variables
against setuid binaries using system/popen.

Steps to reproduce:
% 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
% zsh --version

zsh 5.2 (x86_64-pc-linux-gnu)
%

The solution that bash folks implemented is to drop PS4 from env when the
shell is ran as root.

Best,
mlen

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

[Daniel Shahaf]

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'?)

> % zsh --version
> 
> zsh 5.2 (x86_64-pc-linux-gnu)
> %
> 
> 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.

Thanks for the report,

Daniel

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

[Mateusz Lenik]

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

On Tue, Sep 27, 2016 at 9:54 AM Daniel Shahaf <d.s@daniel.shahaf.name>
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'?)
>
Thanks for looking into it.
It turned out that I didn't research it thoroughly, sorry about that.

glibc execs /bin/sh, so even when zsh is the root shell, it won't be used.
After pointing /bin/sh link to zsh, it works correctly, so after all it was
that bash bug.

Best,
mlen

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

[Peter Stephenson]

On Tue, 27 Sep 2016 07:53:47 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> 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                */

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

[Bart Schaefer]

On Sep 27, 10:02am, Peter Stephenson wrote:
} Subject: Re: BUG: crafting SHELLOPTS and PS4 allows to run arbitrary progr
}
} 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?

I don't know of anything else more appropriate.

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

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 767 bytes --]

On Tue, Sep 27, 2016 at 10:02:21AM +0100, Peter Stephenson wrote:
> 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?

I don't see a reason why zsh running as root shouldn't import
these variables. Only when running in a setuid context possible
security issues arise (ignoring the fact that any setuid program
calling a shell is broken anyway because we will always miss some
env-variable which can be abused).

I think the test should be changed to getuid() != geteuid() or
similar to trigger only in setuid cases.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Bart Schaefer]

On Sep 28, 12:37pm, Simon Ruderich wrote:
}
} I think the test should be changed to getuid() != geteuid() or
} similar to trigger only in setuid cases.

Upon consideration, I agree.

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

[Peter Stephenson]

On Wed, 28 Sep 2016 12:37:32 +0200
Simon Ruderich <simon@ruderich.org> wrote:
> I think the test should be changed to getuid() != geteuid() or
> similar to trigger only in setuid cases.

That seems to catch all the cases where there might be surprises.

Oliver pointed out there's already the PRIVILEGED option for this
purpose, which is set up before variables.  It's definition is a little
different,

    opts[PRIVILEGED] = (getuid() != geteuid() || getgid() != getegid());

but that's probably good enough.

pws

diff --git a/Src/params.c b/Src/params.c
index 87586a2..8271a8b 100644
--- a/Src/params.c
+++ b/Src/params.c
@@ -333,7 +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 IPDEF7R(A,B) {{NULL,A,PM_SCALAR|PM_SPECIAL|PM_DONTIMPORT_SUID},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),
@@ -705,8 +705,8 @@ static int dontimport(int flags)
     /* If value already exported */
     if (flags & PM_EXPORTED)
 	return 1;
-    /* If security issue when exporting as root */
-    if ((flags & PM_DONTIMPORT_ROOT) && (!getuid() || !geteuid()))
+    /* If security issue when importing and running with some privilege */
+    if ((flags & PM_DONTIMPORT_SUID) && isset(PRIVILEGED))
 	return 1;
     /* OK to import */
     return 0;
diff --git a/Src/zsh.h b/Src/zsh.h
index 052d754..79747d6 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -1802,7 +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_DONTIMPORT_SUID (1<<19) /* do not import if running setuid */
 #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                */

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

[Oliver Kiddle]

Mateusz Lenik wrote:
> I just learned that bash fixed a vulnerability that also affects zsh. It
> allowed to run arbitrary programs by crafting SHELLOPTS and PS4 variables
> against setuid binaries using system/popen.

Given that zsh doesn't support the SHELLOPTS variable at all, it doesn't
make sense for zsh to be apparently vulnerable.

> Steps to reproduce:
> % gcc -xc - -otest <<< 'int main() { setuid(0); system("/bin/date"); }'

This attack is directed against the shell that system() runs, i.e.
/bin/sh and not the shell from which the setuid binary is invoked. Did
you have /bin/sh linked to zsh. If it was linked to bash then these
steps are merely reproducing the bash bug in bash.

Zsh also needs the prompt_subst option to enable command substitution in
PS4. Perhaps there's an argument for not importing PS4 from the
environment in certain cases anyway but I can't see any security issue.

Oliver

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

[Peter Stephenson]

On Tue, 27 Sep 2016 10:56:47 +0200
Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> Zsh also needs the prompt_subst option to enable command substitution in
> PS4. Perhaps there's an argument for not importing PS4 from the
> environment in certain cases anyway but I can't see any security issue.

PROMPT_SUBST is enabled in any sh-style emulation, so that's an issue.

I can't offhand think of any way of turning on XTRACE from the
environment, though.  Note that $_ is already marked PM_DONTIMPORT.

pws