zsh-workers
 help / color / mirror / code / Atom feed
* Zsh - Multiple DoS Vulnerabilities
@ 2019-05-10 15:03 David Wells
  2019-05-10 16:37 ` Bart Schaefer
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
  0 siblings, 2 replies; 34+ messages in thread
From: David Wells @ 2019-05-10 15:03 UTC (permalink / raw)
  To: zsh-workers

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

Hello Zsh-Workers,

Tenable has discovered multiple DoS vulnerabilities in Zsh. The root cause
appears to be Invalid Memory Access issues that crash the Zsh runtime. We
believe these have the following Cvss v2 vector: AV:L/AC:L/Au:S/C:N/I:N/A:C
and verified this is present when installed on Arch Linux 5.0.7 x64. We've
internally assigned this vulnerability TRA-221.

Here is a link where you'll find a proof of concept (PoC) called
*zsh_poc.tar.bz2: *
https://tenable.box.com/s/mi7vlmqgq5zpqhadlr90u2hit2r1hjwe.
The PoC contains the 7 different invalid memory access issues in their
respective directory. Each directory will contain a gdb stack trace as well
as the Zsh script which can trigger the bug.

    #1 Invalid read from *taddrstr *call in *text.c*
    POC folder: *01_taddstr_(text.c_148)*

    #2 Invalid read from *execcmd_analyse *in *exec.c*
    POC folder: *02_execcmd_analyse_(exec.c_3653)*

    #3 Invalid read from *dupstring *in *string.c*
    POC folder:  *03_dupstring_(string.c_39)*

    #4 Invalid read from *bin_print *in *builtin.c*
    POC folder: *04_bin_print_(builtin.c_5009)*

    #5 Invalid read from *untokenize *in *exec.c*
    POC folder: *05_untokenize_(exec.c_1994)*

    #6 Invalid read from *getjob *in *jobs.c*
    POC folder: *06_getjob_(jobs.c_1935)*

    #7 Invalid read from *hasher *in *hashtable.c*
    POC folder: *07_hasher_(hashtable.c_85)*

Tenable follows a 90-day vulnerability disclosure policy. That means, even
though we prefer coordinated disclosure, we'll issue an advisory on *August
8, 2019 *with or without a patch. Alternatively, any uncoordinated patch
publicly released before the 90-day deadline will be considered public
disclosure, and Tenable may release an early advisory. You can read the
full details of our policy here:
https://static.tenable.com/research/tenable-vulnerability-disclosure-policy.pdf

Thank you for taking the time to read this. We'd greatly appreciate it if
you'd acknowledge receipt of this report. If you have any questions we'd be
happy to address them.

Thanks again,
David

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-10 15:03 Zsh - Multiple DoS Vulnerabilities David Wells
@ 2019-05-10 16:37 ` Bart Schaefer
  2019-05-12 16:21   ` Stephane Chazelas
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
  1 sibling, 1 reply; 34+ messages in thread
From: Bart Schaefer @ 2019-05-10 16:37 UTC (permalink / raw)
  To: David Wells; +Cc: zsh-workers

It would be helpful if you could explain how this would be exploited
by someone who is not already able to cause the zsh user to execute
some other arbitrary commands.  What's the point of crashing
somebody's shell if you can instead make it remove all their files or
email you their private ssh keys or something?

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-10 15:03 Zsh - Multiple DoS Vulnerabilities David Wells
  2019-05-10 16:37 ` Bart Schaefer
@ 2019-05-10 20:27 ` Bart Schaefer
  2019-05-11  1:45   ` #7 (typeset -Tp) (was Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
                     ` (5 more replies)
  1 sibling, 6 replies; 34+ messages in thread
From: Bart Schaefer @ 2019-05-10 20:27 UTC (permalink / raw)
  To: David Wells; +Cc: zsh-workers

On Fri, May 10, 2019 at 8:04 AM David Wells <bughunters@tenable.com> wrote:
>
>     #1 Invalid read from *taddrstr *call in *text.c*
>     POC folder: *01_taddstr_(text.c_148)*

This has literal NUL bytes embedded in the body of an if/then.  Run
from an interactive shell, it gives:

 text.c:995: unknown word code in gettext2()
 text.c:995: unknown word code in gettext2()
 text.c:72: attempting to decrement tindent below zero
 text.c:72: attempting to decrement tindent below zero

and then (several seconds later) a crash.

The following minimal subset of their test will put the shell into an
infinite loop, without (at least for as long as I was willing to wait)
crashing it:

if true; then me > you || !
:
fi

>     #2 Invalid read from *execcmd_analyse *in *exec.c*
>     POC folder: *02_execcmd_analyse_(exec.c_3653)*

The test case is 3kb of a mangled shell script (missing closing
quotes, random bytes inserted) so I'm not going to attempt to reduce
it to a minimal case.  Feeding it to "zsh -nf" yields:

11: exec.c:2655: BUG: miscounted typeset assignments
11: exec.c:2655: BUG: miscounted typeset assignments
11: exec.c:2655: BUG: miscounted typeset assignments
11: exec.c:2655: BUG: miscounted typeset assignments

and then after several seconds a crash.  I did not attempt feeding
this (or #3 - #5) through a shell that does not have the -n option,
because I don't have a secure sandbox in which to run scripts I can't
visually verify.

>     #3 Invalid read from *dupstring *in *string.c*
>     POC folder:  *03_dupstring_(string.c_39)*

This gives exactly the same errors as #2, and then exits with

[long ugly filename]:87: parse error near `}'

>     #4 Invalid read from *bin_print *in *builtin.c*
>     POC folder: *04_bin_print_(builtin.c_5009)*

This produces no error output at all except for:

[long ugly filename]:41: parse error near `)'

>     #5 Invalid read from *untokenize *in *exec.c*
>     POC folder: *05_untokenize_(exec.c_1994)*

Again no error except:

[long ugly filename]:94: parse error near `}'

>     #6 Invalid read from *getjob *in *jobs.c*
>     POC folder: *06_getjob_(jobs.c_1935)*

This one I fed to "zsh -xf" and got (file name removed for readability):

+1> bg $'%\M-\C-?' $'\C-VI7'
bg:1: no job control in this shell.
+1> disown $'%777777777777777\M-^'
+1> $'\C-['
+1> $'\C-X\C-@\C-@\C-@@\C-@7'
1: command not found: ^[
1: command not found: ^X

followed eventually by a crash.  The input has multiple NUL bytes
following the ^X, and then some other misc. garbage, so the input
processing may have a generic problem with NULs.

>     #7 Invalid read from *hasher *in *hashtable.c*
>     POC folder: *07_hasher_(hashtable.c_85)*

For this one "zsh -xf" says:

+1> foset :print $'\C-@\C-@\C-@hree'
1: command not found: foset
+1> set -E e
+2> typeset -priTt CeE e

and then just goes away until killed.  Only that final typeset is
necessary to reproduce the bug, the rest is irrelevant.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* #7 (typeset -Tp) (was Re: Zsh - Multiple DoS Vulnerabilities)
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
@ 2019-05-11  1:45   ` Oliver Kiddle
  2019-05-13  9:01     ` Peter Stephenson
  2019-05-13 21:11   ` PATCH: #6 negative job id (Re: " Oliver Kiddle
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 34+ messages in thread
From: Oliver Kiddle @ 2019-05-11  1:45 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: David Wells, zsh-workers

Bart wrote:
> >     #7 Invalid read from *hasher *in *hashtable.c*
> >     POC folder: *07_hasher_(hashtable.c_85)*

> +2> typeset -priTt CeE e
>
> and then just goes away until killed.  Only that final typeset is
> necessary to reproduce the bug, the rest is irrelevant.

Actually all it needs is the combination of -T and -p along with
variables that aren't already tied.

With other relevant branches of bin_typeset() for things like
typeset -pi, we check for OPT_ISSET(ops, 'p') early and don't call
typeset_single(). (-m is an exception to this).

An alternative to the following patch would be to simply print an error.
This would just involve the -p test joining the -m test on the next
lines after the final bit of context. However, the behaviour as per this
patch is more consistent with other meaningless typeset combinations
like -pi and -pF.

I'm fairly certain that the second part of the patch renders the lines
removed in the first part as dead code but it'd be good if someone else
could check my logic which is as follows: Given -p, typeset_single() is
only now called when -m is set. usepm will always then be true because
with -m, pm will always be set and never with PM_UNSET. So we go into
the if (usepm) block on line 2193 which has early returns on every
branch.

Oliver


diff --git a/Src/builtin.c b/Src/builtin.c
index 49f017046..ca0ce35f5 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -2583,9 +2583,6 @@ typeset_single(char *cname, char *pname, Param pm, UNUSED(int func),
     }
     pm->node.flags |= (on & PM_READONLY);
 
-    if (OPT_ISSET(ops,'p'))
-	paramtab->printnode(&pm->node, PRINT_TYPESET);
-
     return pm;
 }
 
@@ -2714,7 +2711,7 @@ bin_typeset(char *name, char **argv, LinkList assigns, Options ops, int func)
 	(!isset(GLOBALEXPORT) && !OPT_ISSET(ops,'g')))
 	on |= PM_LOCAL;
 
-    if (on & PM_TIED) {
+    if ((on & PM_TIED) && !OPT_ISSET(ops, 'p')) {
 	Param apm;
 	struct asgment asg0, asg2;
 	char *oldval = NULL, *joinstr;

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-10 16:37 ` Bart Schaefer
@ 2019-05-12 16:21   ` Stephane Chazelas
  2019-05-13 16:29     ` David Wells
  2019-05-31 12:05     ` [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities) Stephane Chazelas
  0 siblings, 2 replies; 34+ messages in thread
From: Stephane Chazelas @ 2019-05-12 16:21 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: David Wells, zsh-workers

2019-05-10 09:37:15 -0700, Bart Schaefer:
> It would be helpful if you could explain how this would be exploited
> by someone who is not already able to cause the zsh user to execute
> some other arbitrary commands.  What's the point of crashing
> somebody's shell if you can instead make it remove all their files or
> email you their private ssh keys or something?

I'd second that.

I suspect it's shellshock aftermath.

Before shellshock, the bash parser was invoked on the content of any
environment variable that started with (), so on attacker
supplied data as opposed to code of script authors.

bash initially didn't check that the content of the variable was
limited to only a function body. After that bug was fixed, the
bash parser was still exposed to arbitrary env vars, and so
people started "fuzzing" the bash parser to discover
vulnerabilities that could be exploited. Quickly, bash was fixed
so that it was not exposed to arbitrary variables, so most of
those vulnerabilities in the parser became minor bugs.

A few of those were still found long after shellshock was fixed
though.

I can't see David's reports, but they do sound like similar
fuzzing test reports.

After shellshock a few related vulnerabilities were found in
zsh, where for instance the zsh arithmetic expression parser
(and by extension the full parser) was invoked on the content of
*some* specific env vars (like SHLVL). That's Oliver's (IIRC)

$ SHLVL='psvar[0$(uname>&2)]' zsh -c ''
Linux

But those were fixed. Nowadays, I don't think zsh's parser is
invoked on attacker supplied-data, and if it were, the fact that
the code crashes the shell would be, as you say, the least of
our worries.

There is one exception I can think of though: the restricted
mode.

That's one mode where the attacker can be the shell code author.

If those bugs allow the user to escape the restricted shell
jail, then it would be a concern. If however the only problem is
the crashing of the shell, then it would not be a concern, as
the user can always do "kill -s SEGV $$" to crash the shell (or
set and reach some limits, or do a deep recursion...).

Now, I don't think we need those bugs to escape a restricted
shell jails as those are almost impossible to implement
reliably and are an anachronism nowadays IMO.

I would be of the opinion that that feature should be removed
(I'd be curious to know if anybody has ever used it).

At least, we should give more warning about it and recommend
alternatives. Here's an attempt below:

diff --git a/Doc/Zsh/restricted.yo b/Doc/Zsh/restricted.yo
index 6cf9b36b5..121e2ae8d 100644
--- a/Doc/Zsh/restricted.yo
+++ b/Doc/Zsh/restricted.yo
@@ -37,3 +37,46 @@ Restricted mode can also be activated any time by setting the
 tt(RESTRICTED) option.  This immediately enables all the restrictions
 described above even if the shell still has not processed all startup
 files.
+
+A shell em(Restricted Mode) is an ancient way to restrict what users may
+do. However modern systems have better, safer and more reliable ways to
+confine user actions like em(chroot jails), em(containers) or em(zones).
+
+A restricted shell is very difficult to implement safely. That feature
+may be removed in a future version of zsh.
+
+It's important to realise the restrictions only apply to the shell and
+not to the commands it runs (except for some of its builtins). While a
+restricted shell can only run the restricted list of commands accessible
+via the predefined `tt(PATH)` variable, it doesn't prevent those
+commands from running any other command.
+
+As an example, if `tt(env)' is among the list of em(allowed) commands,
+then it allows the user to run any command as `tt(env)` is not a shell
+builtin command and can run arbitrary executables.
+
+So when implementing a restricted shell framework it's important to be
+fully aware of what actions each of the em(allowed) commands or features
+(think em(modules)) can perform.
+
+Many commands can have their behaviour affected by environment
+variables. Except for the few listed above, zsh doesn't restrict setting
+environment variables.
+
+Having a `tt(perl)', `tt(python)', `tt(bash)` script as a restricted
+command probably means the user can work around the restriction by
+setting specially crafted `tt(PERL5LIB)', `tt(PYTHONPATH)',
+`tt(BASHENV)' environment variables. On GNU systems, one can have any
+command doing character set conversion (which includes zsh itself) run
+arbitrary code by setting a `tt(GCONV_PATH)' environment variable, those
+are only a few examples.
+
+Bear in mind that contrary to some other shells, `tt(readonly)' is not a
+security feature in zsh as it can be undone and so cannot be used to
+mitigate the above.
+
+A restricted shell is only going to work if the allowed commands are few
+and carefully written so as not to grant more access to users than
+intended. It's also important to restrict what zsh module the user may
+load as some of them like `tt(zsh/system)', `tt(zsh/mapfile)' or
+`tt(zsh/files)' would allow bypassing most of the restrictions.

-- 
Stephane

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: #7 (typeset -Tp) (was Re: Zsh - Multiple DoS Vulnerabilities)
  2019-05-11  1:45   ` #7 (typeset -Tp) (was Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
@ 2019-05-13  9:01     ` Peter Stephenson
  0 siblings, 0 replies; 34+ messages in thread
From: Peter Stephenson @ 2019-05-13  9:01 UTC (permalink / raw)
  To: zsh-workers

On Sat, 2019-05-11 at 03:45 +0200, Oliver Kiddle wrote:
> I'm fairly certain that the second part of the patch renders the lines
> removed in the first part as dead code but it'd be good if someone else
> could check my logic which is as follows: Given -p, typeset_single() is
> only now called when -m is set. usepm will always then be true because
> with -m, pm will always be set and never with PM_UNSET. So we go into
> the if (usepm) block on line 2193 which has early returns on every
> branch.

That certainly looks plausible but you could always put a DPUTS() check
there.

pws



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-12 16:21   ` Stephane Chazelas
@ 2019-05-13 16:29     ` David Wells
  2019-05-13 22:02       ` Bart Schaefer
  2019-05-14 18:10       ` Stephane Chazelas
  2019-05-31 12:05     ` [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities) Stephane Chazelas
  1 sibling, 2 replies; 34+ messages in thread
From: David Wells @ 2019-05-13 16:29 UTC (permalink / raw)
  To: David Wells, Bart Schaefer, zsh-workers

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

Thanks for taking a look at these bugs. As Stephanie mentioned, security
related risk may depend more on Zsh usage, and being that these crashes are
Invalid Memory Access issues, they might allow an attacker to disclose
parts of memory to help with a pre-exploitation process. It looks like
there is patch activity on this thread, would you be able to provide me
update on expected patch date and issues you are patching? Thank you.

On Sun, May 12, 2019 at 9:21 AM Stephane Chazelas <
stephane.chazelas@gmail.com> wrote:

> 2019-05-10 09:37:15 -0700, Bart Schaefer:
> > It would be helpful if you could explain how this would be exploited
> > by someone who is not already able to cause the zsh user to execute
> > some other arbitrary commands.  What's the point of crashing
> > somebody's shell if you can instead make it remove all their files or
> > email you their private ssh keys or something?
>
> I'd second that.
>
> I suspect it's shellshock aftermath.
>
> Before shellshock, the bash parser was invoked on the content of any
> environment variable that started with (), so on attacker
> supplied data as opposed to code of script authors.
>
> bash initially didn't check that the content of the variable was
> limited to only a function body. After that bug was fixed, the
> bash parser was still exposed to arbitrary env vars, and so
> people started "fuzzing" the bash parser to discover
> vulnerabilities that could be exploited. Quickly, bash was fixed
> so that it was not exposed to arbitrary variables, so most of
> those vulnerabilities in the parser became minor bugs.
>
> A few of those were still found long after shellshock was fixed
> though.
>
> I can't see David's reports, but they do sound like similar
> fuzzing test reports.
>
> After shellshock a few related vulnerabilities were found in
> zsh, where for instance the zsh arithmetic expression parser
> (and by extension the full parser) was invoked on the content of
> *some* specific env vars (like SHLVL). That's Oliver's (IIRC)
>
> $ SHLVL='psvar[0$(uname>&2)]' zsh -c ''
> Linux
>
> But those were fixed. Nowadays, I don't think zsh's parser is
> invoked on attacker supplied-data, and if it were, the fact that
> the code crashes the shell would be, as you say, the least of
> our worries.
>
> There is one exception I can think of though: the restricted
> mode.
>
> That's one mode where the attacker can be the shell code author.
>
> If those bugs allow the user to escape the restricted shell
> jail, then it would be a concern. If however the only problem is
> the crashing of the shell, then it would not be a concern, as
> the user can always do "kill -s SEGV $$" to crash the shell (or
> set and reach some limits, or do a deep recursion...).
>
> Now, I don't think we need those bugs to escape a restricted
> shell jails as those are almost impossible to implement
> reliably and are an anachronism nowadays IMO.
>
> I would be of the opinion that that feature should be removed
> (I'd be curious to know if anybody has ever used it).
>
> At least, we should give more warning about it and recommend
> alternatives. Here's an attempt below:
>
> diff --git a/Doc/Zsh/restricted.yo b/Doc/Zsh/restricted.yo
> index 6cf9b36b5..121e2ae8d 100644
> --- a/Doc/Zsh/restricted.yo
> +++ b/Doc/Zsh/restricted.yo
> @@ -37,3 +37,46 @@ Restricted mode can also be activated any time by
> setting the
>  tt(RESTRICTED) option.  This immediately enables all the restrictions
>  described above even if the shell still has not processed all startup
>  files.
> +
> +A shell em(Restricted Mode) is an ancient way to restrict what users may
> +do. However modern systems have better, safer and more reliable ways to
> +confine user actions like em(chroot jails), em(containers) or em(zones).
> +
> +A restricted shell is very difficult to implement safely. That feature
> +may be removed in a future version of zsh.
> +
> +It's important to realise the restrictions only apply to the shell and
> +not to the commands it runs (except for some of its builtins). While a
> +restricted shell can only run the restricted list of commands accessible
> +via the predefined `tt(PATH)` variable, it doesn't prevent those
> +commands from running any other command.
> +
> +As an example, if `tt(env)' is among the list of em(allowed) commands,
> +then it allows the user to run any command as `tt(env)` is not a shell
> +builtin command and can run arbitrary executables.
> +
> +So when implementing a restricted shell framework it's important to be
> +fully aware of what actions each of the em(allowed) commands or features
> +(think em(modules)) can perform.
> +
> +Many commands can have their behaviour affected by environment
> +variables. Except for the few listed above, zsh doesn't restrict setting
> +environment variables.
> +
> +Having a `tt(perl)', `tt(python)', `tt(bash)` script as a restricted
> +command probably means the user can work around the restriction by
> +setting specially crafted `tt(PERL5LIB)', `tt(PYTHONPATH)',
> +`tt(BASHENV)' environment variables. On GNU systems, one can have any
> +command doing character set conversion (which includes zsh itself) run
> +arbitrary code by setting a `tt(GCONV_PATH)' environment variable, those
> +are only a few examples.
> +
> +Bear in mind that contrary to some other shells, `tt(readonly)' is not a
> +security feature in zsh as it can be undone and so cannot be used to
> +mitigate the above.
> +
> +A restricted shell is only going to work if the allowed commands are few
> +and carefully written so as not to grant more access to users than
> +intended. It's also important to restrict what zsh module the user may
> +load as some of them like `tt(zsh/system)', `tt(zsh/mapfile)' or
> +`tt(zsh/files)' would allow bypassing most of the restrictions.
>
> --
> Stephane
>
>

^ permalink raw reply	[flat|nested] 34+ messages in thread

* PATCH: #6 negative job id (Re: Zsh - Multiple DoS Vulnerabilities)
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
  2019-05-11  1:45   ` #7 (typeset -Tp) (was Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
@ 2019-05-13 21:11   ` Oliver Kiddle
  2019-05-13 21:44   ` Zsh - Multiple DoS Vulnerabilities Oliver Kiddle
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 34+ messages in thread
From: Oliver Kiddle @ 2019-05-13 21:11 UTC (permalink / raw)
  To: David Wells, zsh-workers

On 10 May, Bart wrote:
> >     #6 Invalid read from *getjob *in *jobs.c*
> >     POC folder: *06_getjob_(jobs.c_1935)*
>
> This one I fed to "zsh -xf" and got (file name removed for readability):
>
> +1> bg $'%\M-\C-?' $'\C-VI7'
> bg:1: no job control in this shell.
> +1> disown $'%777777777777777\M-^'

This can be reproduced with just %777777777777777
or %2147483648 for that matter. Seems the value returned from atoi()
wraps to negative values if it doesn't fit in an int.

This patch prevents the crash but perhaps atoi() should be replaced with
something that does better error handling to cover numbers that are too
big but get truncated to something positive.

Oliver

diff --git a/Src/jobs.c b/Src/jobs.c
index 73d7f26da..50751decb 100644
--- a/Src/jobs.c
+++ b/Src/jobs.c
@@ -1932,7 +1932,7 @@ getjob(const char *s, const char *prog)
     /* a digit here means we have a job number */
     if (idigit(*s)) {
 	jobnum = atoi(s);
-	if (jobnum && jobnum <= mymaxjob && myjobtab[jobnum].stat &&
+	if (jobnum > 0 && jobnum <= mymaxjob && myjobtab[jobnum].stat &&
 	    !(myjobtab[jobnum].stat & STAT_SUBJOB) &&
 	    /*
 	     * If running jobs in a subshell, we are allowed to

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
  2019-05-11  1:45   ` #7 (typeset -Tp) (was Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
  2019-05-13 21:11   ` PATCH: #6 negative job id (Re: " Oliver Kiddle
@ 2019-05-13 21:44   ` Oliver Kiddle
  2019-05-13 22:36   ` #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 34+ messages in thread
From: Oliver Kiddle @ 2019-05-13 21:44 UTC (permalink / raw)
  To: David Wells, zsh-workers

> On Fri, May 10, 2019 at 8:04 AM David Wells <bughunters@tenable.com> wrote:
> >     #4 Invalid read from *bin_print *in *builtin.c*
> >     POC folder: *04_bin_print_(builtin.c_5009)*

This seems to be very similar to #6: string to int conversion
overflowing to a negative number. In this case you can reproduce it
with just:
  printf '%4444444444444$'

Note that narg below is of type int despite the use of strtoul(). 

Oliver

diff --git a/Src/builtin.c b/Src/builtin.c
index ca0ce35f5..a8f054c8a 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -4990,8 +4990,7 @@ bin_print(char *name, char **args, Options ops, int func)
 	    	narg = strtoul(c, &endptr, 0);
 		if (*endptr == '$') {
 		    c = endptr + 1;
-		    DPUTS(narg <= 0, "specified zero or negative arg");
-		    if (narg > argc) {
+		    if (narg <= 0 || narg > argc) {
 		    	zwarnnam(name, "%d: argument specifier out of range",
 				 narg);
 			if (fout != stdout)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-13 16:29     ` David Wells
@ 2019-05-13 22:02       ` Bart Schaefer
  2019-05-14 18:10       ` Stephane Chazelas
  1 sibling, 0 replies; 34+ messages in thread
From: Bart Schaefer @ 2019-05-13 22:02 UTC (permalink / raw)
  To: David Wells; +Cc: zsh-workers

On Mon, May 13, 2019 at 9:29 AM David Wells <bughunters@tenable.com> wrote:
>
> Thanks for taking a look at these bugs. As Stephanie mentioned, security related risk may depend more on Zsh usage, and being that these crashes are Invalid Memory Access issues, they might allow an attacker to disclose parts of memory to help with a pre-exploitation process. It looks like there is patch activity on this thread, would you be able to provide me update on expected patch date and issues you are patching? Thank you.

It's Stephane, not Stephanie. :-)

Zsh support is entirely by volunteers, there's no one with time
dedicated to this.  It looks like Oliver may be tackling a number of
these, but there's no way for any of us to assert or predict a date
when any particular bug will get worked on or when a release will be
made.  (Please note that you're already testing a pre-release version
as it is.)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities)
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
                     ` (2 preceding siblings ...)
  2019-05-13 21:44   ` Zsh - Multiple DoS Vulnerabilities Oliver Kiddle
@ 2019-05-13 22:36   ` Oliver Kiddle
  2019-05-14  0:13     ` Mikael Magnusson
  2019-05-14 10:50     ` Peter Stephenson
  2019-05-14 16:38   ` Zsh - Multiple DoS Vulnerabilities Peter Stephenson
  2019-05-14 20:30   ` Oliver Kiddle
  5 siblings, 2 replies; 34+ messages in thread
From: Oliver Kiddle @ 2019-05-13 22:36 UTC (permalink / raw)
  To: zsh-workers

On 10 May, Bart wrote:
> >     #3 Invalid read from *dupstring *in *string.c*
> >     POC folder:  *03_dupstring_(string.c_39)*
>
> This gives exactly the same errors as #2, and then exits with
>
> [long ugly filename]:87: parse error near `}'

I've cut this one down to just:

  typeset Q= {X}

That reliably seg faults for me. But that's about as far as I've
been able to get - I'm not especially familiar with zsh's parsing
code.

Oliver

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities)
  2019-05-13 22:36   ` #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
@ 2019-05-14  0:13     ` Mikael Magnusson
  2019-05-14  5:38       ` Bart Schaefer
  2019-05-14 10:50     ` Peter Stephenson
  1 sibling, 1 reply; 34+ messages in thread
From: Mikael Magnusson @ 2019-05-14  0:13 UTC (permalink / raw)
  To: Oliver Kiddle; +Cc: zsh-workers

On 5/14/19, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> On 10 May, Bart wrote:
>> >     #3 Invalid read from *dupstring *in *string.c*
>> >     POC folder:  *03_dupstring_(string.c_39)*
>>
>> This gives exactly the same errors as #2, and then exits with
>>
>> [long ugly filename]:87: parse error near `}'
>
> I've cut this one down to just:
>
>   typeset Q= {X}
>
> That reliably seg faults for me. But that's about as far as I've
> been able to get - I'm not especially familiar with zsh's parsing
> code.

Yeah it looks like some stuff is not exactly going right here,

Breakpoint 2, taddassign (code=0, state=0x7fffffffcfc0, typeset=1) at text.c:187
187	    char *s = ecgetstr(state, EC_NODUP, NULL);
(gdb) p *state
$11 = {prog = 0x7ffff7ff23a8, pc = 0x7ffff7ff240c, strs =
0x7ffff7ff240c "typeset"}
(gdb) step
ecgetstr (s=0x7fffffffcfc0, dup=0, tokflag=0x0) at parse.c:2772
2772	    wordcode c = *s->pc++;
(gdb)
2775	    if (c == 6 || c == 7)
(gdb)
2777	    else if (c & 2) {
(gdb) p c
$12 = 1701869940
(gdb) p *s
$13 = {prog = 0x7ffff7ff23a8, pc = 0x7ffff7ff2410, strs =
0x7ffff7ff240c "typeset"}
(gdb) p *s->pc
$14 = 7628147
(gdb) p prog
No symbol "prog" in current context.
(gdb) p s->prog
$15 = (Eprog) 0x7ffff7ff23a8
(gdb) p *s->prog
$16 = {flags = 2, len = 52, npats = 0, nref = -1, pats = 0x7ffff7ff23e0,
  prog = 0x7ffff7ff23e0, strs = 0x7ffff7ff240c "typeset", shf = 0x0, dump = 0x0}
(gdb) p *s->prog->prog
$17 = 577
(gdb) p s->strs
$18 = 0x7ffff7ff240c "typeset"
(gdb) p s->strs+1
$19 = 0x7ffff7ff240d "ypeset"
(gdb) list
2772	    wordcode c = *s->pc++;
2773	    char *r;
2774	
2775	    if (c == 6 || c == 7)
2776		r = "";
2777	    else if (c & 2) {
2778		buf[0] = (char) ((c >>  3) & 0xff);
2779		buf[1] = (char) ((c >> 11) & 0xff);
2780		buf[2] = (char) ((c >> 19) & 0xff);
2781		buf[3] = '\0';
(gdb) p c
$21 = 1701869940
(gdb) p c&2
$22 = 0
(gdb) step
2785		r = s->strs + (c >> 2);
(gdb) p s->strs
$23 = 0x7ffff7ff240c "typeset"
(gdb) p c>>2
$24 = 425467485
(gdb) p c
$25 = 1701869940
(gdb) list
2780		buf[2] = (char) ((c >> 19) & 0xff);
2781		buf[3] = '\0';
2782		r = dupstring(buf);
2783		dup = EC_NODUP;
2784	    } else {
2785		r = s->strs + (c >> 2);
2786	    }
2787	    if (tokflag)
2788		*tokflag = (c & 1);
2789	
(gdb) step
2787	    if (tokflag)
(gdb) p r
$26 = 0x8000115b4269 <error: Cannot access memory at address 0x8000115b4269>


-- 
Mikael Magnusson

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities)
  2019-05-14  0:13     ` Mikael Magnusson
@ 2019-05-14  5:38       ` Bart Schaefer
  0 siblings, 0 replies; 34+ messages in thread
From: Bart Schaefer @ 2019-05-14  5:38 UTC (permalink / raw)
  To: Mikael Magnusson; +Cc: Oliver Kiddle, zsh-workers

On Mon, May 13, 2019 at 5:13 PM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 5/14/19, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> > On 10 May, Bart wrote:
> >> >     #3 Invalid read from *dupstring *in *string.c*
> >> >     POC folder:  *03_dupstring_(string.c_39)*
> >
> > I've cut this one down to just:
> >
> >   typeset Q= {X}
>
> Yeah it looks like some stuff is not exactly going right here,

It's got something to do with handling it as a reserved word.

BartMAC2014% disable -r typeset
BartMAC2014% typeset Q= {X}
typeset: not valid in this context: {X}

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities)
  2019-05-13 22:36   ` #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
  2019-05-14  0:13     ` Mikael Magnusson
@ 2019-05-14 10:50     ` Peter Stephenson
  1 sibling, 0 replies; 34+ messages in thread
From: Peter Stephenson @ 2019-05-14 10:50 UTC (permalink / raw)
  To: zsh-workers

On Tue, 2019-05-14 at 00:36 +0200, Oliver Kiddle wrote:
> On 10 May, Bart wrote:
> > 
> > > 
> > >     #3 Invalid read from *dupstring *in *string.c*
> > >     POC folder:  *03_dupstring_(string.c_39)*
> > This gives exactly the same errors as #2, and then exits with
> > 
> > [long ugly filename]:87: parse error near `}'
> I've cut this one down to just:
> 
>   typeset Q= {X}
> 
> That reliably seg faults for me. But that's about as far as I've
> been able to get - I'm not especially familiar with zsh's parsing
> code.

Stepping through the parsing code when intypeset is set (with the
optimiser turned off) made it fairly obvious where it was doing
something it shouldn't, and the fix is to adapt code from below to this
case...  This is an obscure case we'd be very unlikely to pick up
normally.

The new parse case isn't actually useful and is bound to fail in the
typeset, but the rational solution seems to be let the normal typeset
code figure that out the same as if the Q= was missing (which I've also
added a test for).

pws

diff --git a/Src/parse.c b/Src/parse.c
index 22e553a16..27234497b 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -1899,6 +1899,14 @@ par_simple(int *cmplx, int nr)
 			    p += nrediradd;
 			    sr += nrediradd;
 			}
+			else if (postassigns)
+			{
+			    /* C.f. normal case below */
+			    postassigns++;
+			    ecadd(WCB_ASSIGN(WC_ASSIGN_SCALAR, WC_ASSIGN_INC, 0));
+			    ecstr(toksave);
+			    ecstr("");	/* TBD can possibly optimise out */
+			}
 			else
 			{
 			    ecstr(toksave);
diff --git a/Test/B02typeset.ztst b/Test/B02typeset.ztst
index ac86e0ad1..e7bf93794 100644
--- a/Test/B02typeset.ztst
+++ b/Test/B02typeset.ztst
@@ -1101,3 +1101,10 @@
 >export zsh_exported_readonly_scalar=1
 >readonly zsh_exported_readonly_array=( 2 )
 >readonly zsh_exported_readonly_scalar=1
+
+  # The second case was buggy as it needs special handling in postassigns
+  (typeset {X})
+  (typeset Q= {X})
+1:Regression test for {...} parsing in typeset
+?(eval):typeset:2: not valid in this context: {X}
+?(eval):typeset:3: not valid in this context: {X}


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
                     ` (3 preceding siblings ...)
  2019-05-13 22:36   ` #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
@ 2019-05-14 16:38   ` Peter Stephenson
  2019-05-14 20:30   ` Oliver Kiddle
  5 siblings, 0 replies; 34+ messages in thread
From: Peter Stephenson @ 2019-05-14 16:38 UTC (permalink / raw)
  To: zsh-workers

On Fri, 2019-05-10 at 13:27 -0700, Bart Schaefer wrote:
> On Fri, May 10, 2019 at 8:04 AM David Wells <bughunters@tenable.com> wrote:
> > 
> > 
> >     #1 Invalid read from *taddrstr *call in *text.c*
> >     POC folder: *01_taddstr_(text.c_148)*
> This has literal NUL bytes embedded in the body of an if/then.  Run
> from an interactive shell, it gives:
> 
>  text.c:995: unknown word code in gettext2()
>  text.c:995: unknown word code in gettext2()
>  text.c:72: attempting to decrement tindent below zero
>  text.c:72: attempting to decrement tindent below zero
> 
> and then (several seconds later) a crash.
> 
> The following minimal subset of their test will put the shell into an
> infinite loop, without (at least for as long as I was willing to wait)
> crashing it:
> 
> if true; then me > you || !
> :
> fi

So the best guess at the moment is the embedded NUL bytes are being
misinterpreted by whatever causes the text to be handled wrongly, so
they are only tangentially relevant?

That would fit with what I'm seeing, which is the infinite loop is in
gettext2(), before anything is executed.  This function tries to decode
wordcode set up by the parser, which is hard to debug because of the
strong correlation between the two completely separate bits of code (and
its own internal structure is a bit head-scratching, too).  Might be
interesting to perturb it until it just doesn't fail any more...

The parsing phase seemed to finish normally, as far as I could see.

pws



^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-13 16:29     ` David Wells
  2019-05-13 22:02       ` Bart Schaefer
@ 2019-05-14 18:10       ` Stephane Chazelas
  2019-05-14 21:24         ` Daniel Shahaf
  2019-05-14 21:39         ` Daniel Shahaf
  1 sibling, 2 replies; 34+ messages in thread
From: Stephane Chazelas @ 2019-05-14 18:10 UTC (permalink / raw)
  To: David Wells; +Cc: Bart Schaefer, zsh-workers

2019-05-13 09:29:36 -0700, David Wells:
> Thanks for taking a look at these bugs. As Stephanie mentioned, security
> related risk may depend more on Zsh usage, and being that these crashes are
> Invalid Memory Access issues, they might allow an attacker to disclose
> parts of memory to help with a pre-exploitation process.
[...]

Thanks David,

I think it's fair to say you've flagged those as DoS
vulnerabilities solely on the ground they were "invalid memory
access issues" irrespective of how exploitable they are.

I'd say those reports are useful as bug reports as they point to
areas where zsh isn't doing the right thing, and I'm sure zsh
developers are grateful for it.

However I think it does a disservice (to the sysadmin and
security community (your customers as I understand your company
creates vulnerability scanning software), not so much to zsh
developers) to call them DoS vulnerabilities as they are not.

It's a shell's (or any interpreter's for that matters) expected
behaviour to fail when provided with unexpected input, so, that
the shell crashes upon invalid input is not really a problem.

A shell will exit when given "exit" or "<()>" as input, that
doesn't mean it's a DoS.

Also, it's a shells job to execute arbitrary command. A
:(){ :|:&};: input (the infamous fork bomb) will cripple a
system, but that's as intended by whoever wrote that code.

From what I can see, those reported issues would only be
vulnerabilities if they were a way to escape the "restricted
mode" of zsh, a "security" feature inherited from the Bourne
shell which hopefully nobody uses these days, but not as DoS
vulnerabilities.

I think you should change the vectors.

IMO, from a security standpoint, it's not very useful to fuzz
"code" input provided to zsh, as anyway any "code" allows zsh to
run any arbitrary command (except for the restricted mode). In
other words, the "code" is generally not the attacker supplied
data. You could fuzz environment variables (the ones zsh cares
about) or other attacker-controlled data fed to zsh scripts like
"limits" instead.

-- 
Stephane

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
                     ` (4 preceding siblings ...)
  2019-05-14 16:38   ` Zsh - Multiple DoS Vulnerabilities Peter Stephenson
@ 2019-05-14 20:30   ` Oliver Kiddle
  2019-05-15 16:50     ` Mikael Magnusson
  2019-05-16 20:37     ` Peter Stephenson
  5 siblings, 2 replies; 34+ messages in thread
From: Oliver Kiddle @ 2019-05-14 20:30 UTC (permalink / raw)
  To: Zsh workers

On 10 May, Bart wrote:
> On Fri, May 10, 2019 at 8:04 AM David Wells <bughunters@tenable.com> wrote:
> >
> >     #1 Invalid read from *taddrstr *call in *text.c*
> >     POC folder: *01_taddstr_(text.c_148)*
>
> and then (several seconds later) a crash.
>
> The following minimal subset of their test will put the shell into an
> infinite loop, without (at least for as long as I was willing to wait)
> crashing it:
>
> if true; then me > you || !
> :
> fi

I'm finding this one will crash on Linux but hang on FreeBSD. And not
crash with true as the condition. A variety of things can be used in the
condition. while .. do .. done can be used in place of if .. then .. fi,
&& or ||. The me > you part can be cut down to :. Try the following:

  if [[ m -eq y ]]; then
    : && !
    :
  fi

Where I had a crash, it was interpreting the wordcode in ecgetstr().
Where it does r = s->strs + (c >> 2), c had an infeasibly large value
causing it to index well beyond the range of s->strs. I'd be inclined to
suspect the problem comes earlier when parsing this into wordcode.

Issues #2, #3 and #5 are not separate issues but slight variations all
leading to the typeset followed by braces bug. So thanks to Peter, I think
those are all now fixed leaving this (#1) as the only one outstanding.

Oliver

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-14 18:10       ` Stephane Chazelas
@ 2019-05-14 21:24         ` Daniel Shahaf
  2019-05-14 21:38           ` Bart Schaefer
  2019-05-14 21:39         ` Daniel Shahaf
  1 sibling, 1 reply; 34+ messages in thread
From: Daniel Shahaf @ 2019-05-14 21:24 UTC (permalink / raw)
  To: David Wells; +Cc: zsh-workers

Stephane Chazelas wrote on Tue, 14 May 2019 18:11 +00:00:
> IMO, from a security standpoint, it's not very useful to fuzz
> "code" input provided to zsh, as anyway any "code" allows zsh to
> run any arbitrary command (except for the restricted mode). In
> other words, the "code" is generally not the attacker supplied
> data.

Sounds right.  There might be some corner case here 

> You could fuzz environment variables (the ones zsh cares
> about) or other attacker-controlled data fed to zsh scripts like
> "limits" instead.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-14 21:24         ` Daniel Shahaf
@ 2019-05-14 21:38           ` Bart Schaefer
  0 siblings, 0 replies; 34+ messages in thread
From: Bart Schaefer @ 2019-05-14 21:38 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: David Wells, zsh-workers

On Tue, May 14, 2019 at 2:25 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Stephane Chazelas wrote on Tue, 14 May 2019 18:11 +00:00:
> > IMO, from a security standpoint, it's not very useful to fuzz
> > "code" input provided to zsh, as anyway any "code" allows zsh to
> > run any arbitrary command (except for the restricted mode). In
> > other words, the "code" is generally not the attacker supplied
> > data.
>
> Sounds right.  There might be some corner case here

The other interesting case would be one where the zsh code enabled
privilege escalation, i.e., where the coder is the attacker and the
shell is the vector to a different security issue.  A zsh script to
exploit ZombieLoad, for example.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-14 18:10       ` Stephane Chazelas
  2019-05-14 21:24         ` Daniel Shahaf
@ 2019-05-14 21:39         ` Daniel Shahaf
  2019-05-14 22:25           ` Bart Schaefer
  1 sibling, 1 reply; 34+ messages in thread
From: Daniel Shahaf @ 2019-05-14 21:39 UTC (permalink / raw)
  To: David Wells; +Cc: zsh-workers

[sorry for double send]

Stephane Chazelas wrote on Tue, 14 May 2019 18:11 +00:00:
> IMO, from a security standpoint, it's not very useful to fuzz
> "code" input provided to zsh, as anyway any "code" allows zsh to
> run any arbitrary command (except for the restricted mode). In
> other words, the "code" is generally not the attacker supplied
> data.

Sounds right.

I've been trying to come up with counterexamples.  What if somebody
installed a /etc/zshenv that does, say, 'disable zmodload enable'?
If that actually prevents zmodload from being run,¹ then a bug that
allows zmodload to be run would be interesting.

Cheers,

Daniel

¹ I'm not sure it does because there might be some other way to run
zmodload — an assignment to $modules, maybe?  (Don't have time to test
this, sorry.)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-14 21:39         ` Daniel Shahaf
@ 2019-05-14 22:25           ` Bart Schaefer
  2019-05-15 10:48             ` Daniel Shahaf
  0 siblings, 1 reply; 34+ messages in thread
From: Bart Schaefer @ 2019-05-14 22:25 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: David Wells, zsh-workers

On Tue, May 14, 2019 at 2:39 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> I've been trying to come up with counterexamples.  What if somebody
> installed a /etc/zshenv that does, say, 'disable zmodload enable'?

You can bypass /etc/zshenv by, for example, invoking zsh as "sh" and
then running "emulate -R" and/or otherwise futzing with setopts.  So
either THAT is a security flaw, or your example isn't one either.

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-14 22:25           ` Bart Schaefer
@ 2019-05-15 10:48             ` Daniel Shahaf
  0 siblings, 0 replies; 34+ messages in thread
From: Daniel Shahaf @ 2019-05-15 10:48 UTC (permalink / raw)
  To: zsh-workers; +Cc: David Wells

Bart Schaefer wrote on Tue, 14 May 2019 22:26 +00:00:
> On Tue, May 14, 2019 at 2:39 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > I've been trying to come up with counterexamples.  What if somebody
> > installed a /etc/zshenv that does, say, 'disable zmodload enable'?
> 
> You can bypass /etc/zshenv by, for example, invoking zsh as "sh" and
> then running "emulate -R" and/or otherwise futzing with setopts.

I don't think there's an easy solution here, since sourcing /etc/zshenv
in mid-session could be a can of worms, too.

> So either THAT is a security flaw, or your example isn't one either.

I suppose my example was a security flaw _in the sysadmin's setup_.  If someone
wants to make the case that it's a bug in zsh, I'm all ears.

Cheers,

Daniel

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-14 20:30   ` Oliver Kiddle
@ 2019-05-15 16:50     ` Mikael Magnusson
  2019-05-16 20:37     ` Peter Stephenson
  1 sibling, 0 replies; 34+ messages in thread
From: Mikael Magnusson @ 2019-05-15 16:50 UTC (permalink / raw)
  To: Oliver Kiddle; +Cc: Zsh workers

On 5/14/19, Oliver Kiddle <okiddle@yahoo.co.uk> wrote:
> On 10 May, Bart wrote:
>> On Fri, May 10, 2019 at 8:04 AM David Wells <bughunters@tenable.com>
>> wrote:
>> >
>> >     #1 Invalid read from *taddrstr *call in *text.c*
>> >     POC folder: *01_taddstr_(text.c_148)*
>>
>> and then (several seconds later) a crash.
>>
>> The following minimal subset of their test will put the shell into an
>> infinite loop, without (at least for as long as I was willing to wait)
>> crashing it:
>>
>> if true; then me > you || !
>> :
>> fi
>
> I'm finding this one will crash on Linux but hang on FreeBSD. And not
> crash with true as the condition. A variety of things can be used in the
> condition. while .. do .. done can be used in place of if .. then .. fi,
> && or ||. The me > you part can be cut down to :. Try the following:
>
>   if [[ m -eq y ]]; then
>     : && !
>     :
>   fi
>
> Where I had a crash, it was interpreting the wordcode in ecgetstr().
> Where it does r = s->strs + (c >> 2), c had an infeasibly large value
> causing it to index well beyond the range of s->strs. I'd be inclined to
> suspect the problem comes earlier when parsing this into wordcode.
>
> Issues #2, #3 and #5 are not separate issues but slight variations all
> leading to the typeset followed by braces bug. So thanks to Peter, I think
> those are all now fixed leaving this (#1) as the only one outstanding.

Might it be worth adding some type of check to the ecgetstr() code, so
we get a DPUTS instead of a crash if c>>2 is incredibly large? I'm not
sure atm how this would be determined, or what typical values are, but
I think two of these issues led to a crash here. We could also get
arbitrary bytecode from a modified .zwc file although of course in
that case you've already los tany security. Still would be nice to not
crash from misparsing it though.

-- 
Mikael Magnusson

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-14 20:30   ` Oliver Kiddle
  2019-05-15 16:50     ` Mikael Magnusson
@ 2019-05-16 20:37     ` Peter Stephenson
  2019-05-17 13:41       ` Mikael Magnusson
  1 sibling, 1 reply; 34+ messages in thread
From: Peter Stephenson @ 2019-05-16 20:37 UTC (permalink / raw)
  To: zsh-workers

On Tue, 2019-05-14 at 22:30 +0200, Oliver Kiddle wrote:
> I'm finding this one will crash on Linux but hang on FreeBSD. And not
> crash with true as the condition. A variety of things can be used in the
> condition. while .. do .. done can be used in place of if .. then .. fi,
> && or ||. The me > you part can be cut down to :. Try the following:
> 
>   if [[ m -eq y ]]; then
>     : && !
>     :
>   fi
> 
> Where I had a crash, it was interpreting the wordcode in ecgetstr().
> Where it does r = s->strs + (c >> 2), c had an infeasibly large value
> causing it to index well beyond the range of s->strs. I'd be inclined to
> suspect the problem comes earlier when parsing this into wordcode.

I'm starting to wonder if this is an allocation rather than a parsing
problem --- the parsing is OK but something goes wrong with the final
pointer / afterwards / in building or copying the word code, so
that gettext2() or the exec code ends up trying to interpret garbage at
the end.

pws


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-16 20:37     ` Peter Stephenson
@ 2019-05-17 13:41       ` Mikael Magnusson
  2019-05-17 13:51         ` Mikael Magnusson
  0 siblings, 1 reply; 34+ messages in thread
From: Mikael Magnusson @ 2019-05-17 13:41 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: zsh-workers

On 5/16/19, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
> On Tue, 2019-05-14 at 22:30 +0200, Oliver Kiddle wrote:
>> I'm finding this one will crash on Linux but hang on FreeBSD. And not
>> crash with true as the condition. A variety of things can be used in the
>> condition. while .. do .. done can be used in place of if .. then .. fi,
>> && or ||. The me > you part can be cut down to :. Try the following:
>>
>>   if [[ m -eq y ]]; then
>>     : && !
>>     :
>>   fi
>>
>> Where I had a crash, it was interpreting the wordcode in ecgetstr().
>> Where it does r = s->strs + (c >> 2), c had an infeasibly large value
>> causing it to index well beyond the range of s->strs. I'd be inclined to
>> suspect the problem comes earlier when parsing this into wordcode.
>
> I'm starting to wonder if this is an allocation rather than a parsing
> problem --- the parsing is OK but something goes wrong with the final
> pointer / afterwards / in building or copying the word code, so
> that gettext2() or the exec code ends up trying to interpret garbage at
> the end.

FWIW I ran this under valgrind, and the first invalid read is the one
that causes the segfault, so no help there.

-- 
Mikael Magnusson

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-17 13:41       ` Mikael Magnusson
@ 2019-05-17 13:51         ` Mikael Magnusson
  2019-05-17 14:28           ` Mikael Magnusson
  2019-05-18 10:31           ` Oliver Kiddle
  0 siblings, 2 replies; 34+ messages in thread
From: Mikael Magnusson @ 2019-05-17 13:51 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: zsh-workers

On 5/17/19, Mikael Magnusson <mikachu@gmail.com> wrote:
> On 5/16/19, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
>> On Tue, 2019-05-14 at 22:30 +0200, Oliver Kiddle wrote:
>>> I'm finding this one will crash on Linux but hang on FreeBSD. And not
>>> crash with true as the condition. A variety of things can be used in the
>>> condition. while .. do .. done can be used in place of if .. then .. fi,
>>> && or ||. The me > you part can be cut down to :. Try the following:
>>>
>>>   if [[ m -eq y ]]; then
>>>     : && !
>>>     :
>>>   fi
>>>
>>> Where I had a crash, it was interpreting the wordcode in ecgetstr().
>>> Where it does r = s->strs + (c >> 2), c had an infeasibly large value
>>> causing it to index well beyond the range of s->strs. I'd be inclined to
>>> suspect the problem comes earlier when parsing this into wordcode.
>>
>> I'm starting to wonder if this is an allocation rather than a parsing
>> problem --- the parsing is OK but something goes wrong with the final
>> pointer / afterwards / in building or copying the word code, so
>> that gettext2() or the exec code ends up trying to interpret garbage at
>> the end.
>
> FWIW I ran this under valgrind, and the first invalid read is the one
> that causes the segfault, so no help there.

Played with gdb reverse debugging a bit and found that at one point
before the crash, we have this somewhat incorrect string built up:
(gdb) p tptr-48
$28 = 0x6e7560 <jbuf> "if [[ m -eq y ]]; then; : && ! :; select G\305\305 in "


-- 
Mikael Magnusson

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-17 13:51         ` Mikael Magnusson
@ 2019-05-17 14:28           ` Mikael Magnusson
  2019-05-18 10:31           ` Oliver Kiddle
  1 sibling, 0 replies; 34+ messages in thread
From: Mikael Magnusson @ 2019-05-17 14:28 UTC (permalink / raw)
  To: Peter Stephenson; +Cc: zsh-workers

On 5/17/19, Mikael Magnusson <mikachu@gmail.com> wrote:
> On 5/17/19, Mikael Magnusson <mikachu@gmail.com> wrote:
>> On 5/16/19, Peter Stephenson <p.w.stephenson@ntlworld.com> wrote:
>>> On Tue, 2019-05-14 at 22:30 +0200, Oliver Kiddle wrote:
>>>> I'm finding this one will crash on Linux but hang on FreeBSD. And not
>>>> crash with true as the condition. A variety of things can be used in
>>>> the
>>>> condition. while .. do .. done can be used in place of if .. then ..
>>>> fi,
>>>> && or ||. The me > you part can be cut down to :. Try the following:
>>>>
>>>>   if [[ m -eq y ]]; then
>>>>     : && !
>>>>     :
>>>>   fi
>>>>
>>>> Where I had a crash, it was interpreting the wordcode in ecgetstr().
>>>> Where it does r = s->strs + (c >> 2), c had an infeasibly large value
>>>> causing it to index well beyond the range of s->strs. I'd be inclined
>>>> to
>>>> suspect the problem comes earlier when parsing this into wordcode.
>>>
>>> I'm starting to wonder if this is an allocation rather than a parsing
>>> problem --- the parsing is OK but something goes wrong with the final
>>> pointer / afterwards / in building or copying the word code, so
>>> that gettext2() or the exec code ends up trying to interpret garbage at
>>> the end.
>>
>> FWIW I ran this under valgrind, and the first invalid read is the one
>> that causes the segfault, so no help there.
>
> Played with gdb reverse debugging a bit and found that at one point
> before the crash, we have this somewhat incorrect string built up:
> (gdb) p tptr-48
> $28 = 0x6e7560 <jbuf> "if [[ m -eq y ]]; then; : && ! :; select G\305\305 in
> "

If I save the above code in a file, named crash.zsh and run zsh -fc
'source crash.zsh' then it will crash. If I run zcompile on it, and
then run the same command, I instead get the infinite loop in text.c:

420		if (stack) {
(gdb)
421		    if (!(s = tstack))
(gdb)
423		    if (s->pop) {
(gdb)
428		    code = s->code;
(gdb)
429		    stack = 0;
(gdb)
434		switch (wc_code(code)) {
(gdb)
458		    if (!s) {
(gdb)
468			if (!(stack = (WC_SUBLIST_TYPE(code) == WC_SUBLIST_END))) {
(gdb)
479		    if (stack < 1 && (WC_SUBLIST_FLAGS(s->code) & WC_SUBLIST_SIMPLE))
(gdb)
481		    break;
(gdb)
420		if (stack) {


-- 
Mikael Magnusson

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-17 13:51         ` Mikael Magnusson
  2019-05-17 14:28           ` Mikael Magnusson
@ 2019-05-18 10:31           ` Oliver Kiddle
  2019-05-21 14:43             ` Oliver Kiddle
  1 sibling, 1 reply; 34+ messages in thread
From: Oliver Kiddle @ 2019-05-18 10:31 UTC (permalink / raw)
  To: zsh-workers

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

Mikael Magnusson wrote:
> Played with gdb reverse debugging a bit and found that at one point
> before the crash, we have this somewhat incorrect string built up:
> (gdb) p tptr-48
> $28 = 0x6e7560 <jbuf> "if [[ m -eq y ]]; then; : && ! :; select G\305\305 in "

The point at which the code starts to go wrong is these lines in text.c:

450		    s->code = *state->pc++;
451		    s->pop = (WC_LIST_TYPE(s->code) & Z_END);

code, which is what was popped from the stack is WC_LIST(type=SYNC, skip=0) -
Z_END is not set. A comment in parse.c states:
 *     - if not (type & Z_END), followed by next WC_LIST

s->code is WC_END not WC_LIST. It doesn't seem valid to check WC_LIST_TYPE for
that. We now iterate back round the loop picking up the garbage
WC_SELECT(type=pparam, skip=27625473) value for the next code.
That explains the select text that Mikael mentions.
But I still have little idea what has led to the code doing this.

After briefly trying to manually decode wordcode values, I hacked together a
gdb pretty printer for them (attached) which makes it rather easier.

Anyway the full wordcode looks like the following. I've manually tried to
substitute codes corresponding to strings and indented for some of the skips
but am not certain: especially the first string which might be a redirection
code.

  List(type=SYNC|END, skip=0),
  SubList(type=END, skip=19),
  Pipe(type=end, line=4),
  If(type=head, skip=17),
    If(type=if, skip=16),
	List(type=SYNC|END|SIMPLE, skip=4),
	    string
	    Cond(-eq, skip=0),
	    string
	    string
	List(type=SYNC, skip=0),
	SubList(type=AND, skip=3),
	    Pipe(type=end, line=5),
	    Simple(argc=1),
	    string
	SubList(type=END, flags=NOT, skip=0),
	List(type=SYNC|END, skip=0),
	SubList(type=END, skip=3),
	    Pipe(type=end, line=6),
	    Simple(argc=1),
	    string
  End

Without delving further into this, I'm somewhat unsure as to the meaning of the
END types on lists and how start/end matching works along with the stack used
in gettext2(). Maybe someone else knows it better?

To enable the gdb pretty printer, just dump the file in the current
directory and at the gdb prompt, type:
  python execfile("wordcode.py")
To confirm, this worked: info pretty-printer
To get line based output it can also be useful to do:
  set print array on
And to watch the parse stage:
  watch *ecbuf@22

Note that it can't discern the placeholders for strings and I've not
tested it exhaustively so there may be errors.

There may be nicer ways to handle enabling the pretty-printer; you
may have seen this if you've ever enabled to C++ STL pretty printers.
Would it make sense to include things like this somewhere in the git
repository? It is essentially a duplicate of C code so might bitrot
relative to it. Also, I'm not especially fluent in Python, so it could
perhaps be better. But it can be useful. I also have a gdb macro for the
zle undo stack.

Another aside, playing around with bit flags reminded me of a zsh annoyance:
printing a negative number in binary gives you a minus sign followed by the
positive representation of it rather than the two's complement form. Does it
make sense to allow some form of unsigned output besides printf
(which doesn't do binary)? And perhaps a Java style >>> operator.

Oliver


[-- Attachment #2: wordcode.py --]
[-- Type: text/plain, Size: 5091 bytes --]

# Gdb pretty printer for zsh wordcode values.

# python execfile("wordcode.py")
# set print array on
# print *ecbuf@22
# note that you'll get garbage values for codes that reference strings

from operator import itemgetter

class WordcodePrinter:
    WC_END     = 0   
    WC_LIST    = 1
    WC_SUBLIST = 2
    WC_PIPE    = 3
    WC_REDIR   = 4
    WC_ASSIGN  = 5
    WC_SIMPLE  = 6
    WC_TYPESET = 7
    WC_SUBSH   = 8
    WC_CURSH   = 9
    WC_TIMED   = 10
    WC_FUNCDEF = 11
    WC_FOR     = 12
    WC_SELECT  = 13
    WC_WHILE   = 14
    WC_REPEAT  = 15
    WC_CASE    = 16
    WC_IF      = 17
    WC_COND    = 18
    WC_ARITH   = 19
    WC_AUTOFN  = 20
    WC_TRY     = 21

    WC_CODEBITS  = 5
    Z_END        = (1<<4) 
    Z_SIMPLE     = (1<<5)
    WC_LIST_FREE = (6)

    NAME = [ "End", "List", "SubList", "Pipe", "Redir", "Assign", "Simple",
             "Typeset", "Subsh", "Cursh", "Timed", "Funcdef", "For",
             "Select", "While", "Repeat", "Case", "If", "Cond", "Arith",
             "Autofn", "Try" ]
    
    def __init__(self, val):
        self.val = val

    def wc_code(self, code):
        return int(code) & ((1 << self.WC_CODEBITS) - 1)

    def wc_data(self, code):
        return int(code) >> self.WC_CODEBITS

    def wc_list_type(self, data):
        return '|'.join(map(itemgetter(1), filter(lambda (i,s): data & (1<<i),
            enumerate(["TIMED", "SYNC", "ASYNC", "DISOWN", "END", "SIMPLE"]))))

    def wc_sublist_type(self, data):
        return [ "END", "AND", "OR" ][data & 3]

    def wc_redir_type(self, data):
        return [">", ">|", ">>", ">>|", "&>", ">&|", ">>&", ">>&|", "<>",
            "<", "<<", "<<-", "<<<", "<&n", ">&n", ">&-, <&-", "< <(...)",
            "> >(...)" ][data & 0x1f]

    def wc_sublist_flags(self, data):
        return '|'.join(map(itemgetter(1), filter(lambda (i,s): data & (1<<i),
            enumerate(["COPROC", "NOT", "SIMPLE" ], start=2))))

    def wc_if_type(self, data):
        return [ "head", "if", "elif", "else" ][data & 3]

    def wc_for_type(self, data):
        return [ "pparam", "list", "cond" ][data & 3]

    def wc_case_type(self, data):
        return [ "head", "or", "and", "testand" ][data & 7]

    def wc_cond_type(self, data):
        return [ "!", "&&", "||", "=", "==", "!=", "<",
                 ">", "-nt", "-ot", "-ef", "-eq", "-ne", "-lt", "-gt", "-le",
                 "-ge", "=~", "MOD", "MOD(infix)" ][data & 127]

    def to_string(self):
        try:
            code = self.wc_code(self.val)
            data = self.wc_data(self.val)
            name = self.NAME[code]
            if (code == self.WC_LIST):
                name += '(type={}, skip={})'.format(self.wc_list_type(data), data >> 6)
            elif (code == self.WC_SUBLIST and data & 0x1c):
                name += '(type={}, flags={}, skip={})'.format(
                    self.wc_sublist_type(data), self.wc_sublist_flags(data), data >> 5)
            elif (code == self.WC_SUBLIST):
                name += '(type={}, skip={})'.format(self.wc_sublist_type(data),
                    data >> 5)
            elif (code == self.WC_REDIR):
              name += '(type={}{})'.format(self.wc_redir_type(data),
                  ", varid=true" if (data & 0x20) else "")
            elif (code == self.WC_ASSIGN):
              if data & 2: name += "+="
              name += "(array[{}])".format(data >> 2) if data & 1 else "(scalar)"
            elif (code == self.WC_SUBSH or code == self.WC_CURSH
                    or code == self.WC_REPEAT or code == self.WC_TRY
                    or code == self.WC_FUNCDEF):
                name += '(skip={})'.format(data)
            elif (code == self.WC_TIMED):
                name += '(type={})'.format("pipe" if data else "empty")
            elif (code == self.WC_PIPE):
                name += '(type={}, line={})'.format(
		    "mid" if (data & 1) else "end", (data >> 1))
            elif (code == self.WC_FOR):
                name += '(type={}, skip={})'.format(self.wc_for_type(data), data >> 2)
            elif (code == self.WC_SELECT):
                name += '(type={}, skip={})'.format(
		    "pparam" if (data & 1) else "list", (data >> 1))
            elif (code == self.WC_WHILE):
                if data & 1: name += '/until'
                name += '(skip={})'.format(data >> 1)
            elif (code == self.WC_CASE):
                name += '(type={}, skip={})'.format(self.wc_case_type(data), data >> 3)
            elif (code == self.WC_SIMPLE or code == self.WC_TYPESET):
                name += '(argc={})'.format(data)
            elif (code == self.WC_IF):
                name += '(type={}, skip={})'.format(self.wc_if_type(data), data >> 2)
            elif (code == self.WC_COND):
                name += '({}, skip={})'.format(self.wc_cond_type(data), data >> 7)
        except IndexError:
            name = int(code) # any error likely means it isn't a wordcode

        return name

def zsh(val):
    if str(val.type) == 'wordcode':
        return WordcodePrinter(val)
    return None

gdb.pretty_printers.append(zsh)

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
  2019-05-18 10:31           ` Oliver Kiddle
@ 2019-05-21 14:43             ` Oliver Kiddle
       [not found]               ` <CGME20190521154256eucas1p1f0816d2467abd8bf4a0c31058af2983a@eucas1p1.samsung.com>
  0 siblings, 1 reply; 34+ messages in thread
From: Oliver Kiddle @ 2019-05-21 14:43 UTC (permalink / raw)
  To: zsh-workers

The following patch is one approach to fixing the last of these bugs.

There may be a cleaner approach relying on the WC_SUBLIST_END tags,
probably involving removing this whole block which is looking ahead to
the next wordcode rather than leaving it for the next iteration of the
big loop. But that would be a much bigger change with a greater chance
of breaking things.

The choice of "!" or "! " is a bit tricky and there may be some odd
cases remaining wherever ! is used without something to negate.

The code in exec.c that checks WC_SUBLIST_SKIP on the next code before
knowing that it is a sublist is harmless because it only assigns the
result to the next variable for later use. It still feels somewhat
impure to my taste but I've left it.

The patch also adds tests.

Oliver

diff --git a/Src/text.c b/Src/text.c
index 3658b1bc6..a4191bf1a 100644
--- a/Src/text.c
+++ b/Src/text.c
@@ -470,8 +470,13 @@ gettext2(Estate state)
 			    " || " : " && ");
 		    s->code = *state->pc++;
 		    s->pop = (WC_SUBLIST_TYPE(s->code) == WC_SUBLIST_END);
-		    if (WC_SUBLIST_FLAGS(s->code) & WC_SUBLIST_NOT)
-			taddstr("! ");
+		    if (WC_SUBLIST_FLAGS(s->code) & WC_SUBLIST_NOT) {
+			if (WC_SUBLIST_SKIP(s->code) == 0)
+			    stack = 1;
+			taddstr((stack || (!(WC_SUBLIST_FLAGS(s->code) &
+			        WC_SUBLIST_SIMPLE) && wc_code(*state->pc) !=
+			        WC_PIPE)) ? "!" : "! ");
+		    }
 		    if (WC_SUBLIST_FLAGS(s->code) & WC_SUBLIST_COPROC)
 			taddstr("coproc ");
 		}
diff --git a/Test/A01grammar.ztst b/Test/A01grammar.ztst
index 1ed3cb6b7..c8600d4cb 100644
--- a/Test/A01grammar.ztst
+++ b/Test/A01grammar.ztst
@@ -76,6 +76,39 @@
 0:Basic current shell list with error
 >false
 
+  fn() { : && ! ; : }
+  functions -x3 fn
+  fn
+0:End of sublist containing ! with no command
+>fn () {
+>   : && !
+>   :
+>}
+
+  if [[ m -eq y ]]; then
+    : && !
+    :
+  fi
+0:! followed by no further commands
+
+  fn() { ! {!} && ! (!) || ! {!} }
+  functions -x2 fn
+  fn
+0:exclamation marks without following commands
+>fn () {
+>  ! {
+>    !
+>  } && ! (
+>    !
+>  ) || ! {
+>    !
+>  }
+>}
+
+  ! | true
+1:! followed by no command but by a pipe
+?(eval):1: parse error near `|'
+
 #
 # Tests for `Precommand Modifiers'
 #

^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: Zsh - Multiple DoS Vulnerabilities
       [not found]               ` <CGME20190521154256eucas1p1f0816d2467abd8bf4a0c31058af2983a@eucas1p1.samsung.com>
@ 2019-05-21 15:42                 ` Peter Stephenson
  0 siblings, 0 replies; 34+ messages in thread
From: Peter Stephenson @ 2019-05-21 15:42 UTC (permalink / raw)
  To: zsh-workers

On Tue, 2019-05-21 at 16:43 +0200, Oliver Kiddle wrote:
> The following patch is one approach to fixing the last of these bugs.
> 
> There may be a cleaner approach relying on the WC_SUBLIST_END tags,
> probably involving removing this whole block which is looking ahead to
> the next wordcode rather than leaving it for the next iteration of the
> big loop. But that would be a much bigger change with a greater chance
> of breaking things.

OK, so this takes account of the fact that "!" on its own (no following
command line) is allowed and just means negate the status.  That
certainly seems a reasonable way to go.

I was wondering whether this actually shouldn't be a special case in the
parser, but it's not obvious what to do there --- there actually is
nothing following the "!" and pretending there is something isn't a
great fix.  Telling the wordcode handler it can work this way is probably
a better idea.

Cheers
pws


^ permalink raw reply	[flat|nested] 34+ messages in thread

* [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities)
  2019-05-12 16:21   ` Stephane Chazelas
  2019-05-13 16:29     ` David Wells
@ 2019-05-31 12:05     ` Stephane Chazelas
  2019-06-03  9:35       ` Peter Stephenson
  2019-06-04  2:39       ` dana
  1 sibling, 2 replies; 34+ messages in thread
From: Stephane Chazelas @ 2019-05-31 12:05 UTC (permalink / raw)
  To: Bart Schaefer, David Wells, zsh-workers

2019-05-12 17:21:49 +0100, Stephane Chazelas:
[...]
> At least, we should give more warning about it and recommend
> alternatives. Here's an attempt below:

I'm bringing this up again as it looks like it has stayed mostly
unnoticed (except maybe by Chet ;-))

IMO, the restricted mode should be deprecated. In any case, we
should at least warn against using it. Below was my attempt
which could definitely be improved, I hope it can serve as a
basic for discussio.

See also what Chet recently added about it in the bash
documentation at
https://git.savannah.gnu.org/cgit/bash.git/commit/doc/bash.info?id=52e469696418877c5b7cd7f752f5f8309ef65a38
which is more concise.


diff --git a/Doc/Zsh/restricted.yo b/Doc/Zsh/restricted.yo
index 6cf9b36b5..121e2ae8d 100644
--- a/Doc/Zsh/restricted.yo
+++ b/Doc/Zsh/restricted.yo
@@ -37,3 +37,46 @@ Restricted mode can also be activated any time by setting the
 tt(RESTRICTED) option.  This immediately enables all the restrictions
 described above even if the shell still has not processed all startup
 files.
+
+A shell em(Restricted Mode) is an ancient way to restrict what users may
+do. However modern systems have better, safer and more reliable ways to
+confine user actions like em(chroot jails), em(containers) or em(zones).
+
+A restricted shell is very difficult to implement safely. That feature
+may be removed in a future version of zsh.
+
+It's important to realise the restrictions only apply to the shell and
+not to the commands it runs (except for some of its builtins). While a
+restricted shell can only run the restricted list of commands accessible
+via the predefined `tt(PATH)` variable, it doesn't prevent those
+commands from running any other command.
+
+As an example, if `tt(env)' is among the list of em(allowed) commands,
+then it allows the user to run any command as `tt(env)` is not a shell
+builtin command and can run arbitrary executables.
+
+So when implementing a restricted shell framework it's important to be
+fully aware of what actions each of the em(allowed) commands or features
+(think em(modules)) can perform.
+
+Many commands can have their behaviour affected by environment
+variables. Except for the few listed above, zsh doesn't restrict setting
+environment variables.
+
+Having a `tt(perl)', `tt(python)', `tt(bash)` script as a restricted
+command probably means the user can work around the restriction by
+setting specially crafted `tt(PERL5LIB)', `tt(PYTHONPATH)',
+`tt(BASHENV)' environment variables. On GNU systems, one can have any
+command doing character set conversion (which includes zsh itself) run
+arbitrary code by setting a `tt(GCONV_PATH)' environment variable, those
+are only a few examples.
+
+Bear in mind that contrary to some other shells, `tt(readonly)' is not a
+security feature in zsh as it can be undone and so cannot be used to
+mitigate the above.
+
+A restricted shell is only going to work if the allowed commands are few
+and carefully written so as not to grant more access to users than
+intended. It's also important to restrict what zsh module the user may
+load as some of them like `tt(zsh/system)', `tt(zsh/mapfile)' or
+`tt(zsh/files)' would allow bypassing most of the restrictions.

-- 
Stephane


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities)
  2019-05-31 12:05     ` [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities) Stephane Chazelas
@ 2019-06-03  9:35       ` Peter Stephenson
  2019-06-04  2:39       ` dana
  1 sibling, 0 replies; 34+ messages in thread
From: Peter Stephenson @ 2019-06-03  9:35 UTC (permalink / raw)
  To: zsh-workers

On Fri, 2019-05-31 at 13:05 +0100, Stephane Chazelas wrote:
> 2019-05-12 17:21:49 +0100, Stephane Chazelas:
> [...]
> > 
> > At least, we should give more warning about it and recommend
> > alternatives. Here's an attempt below:
> I'm bringing this up again as it looks like it has stayed mostly
> unnoticed (except maybe by Chet ;-))
> 
> IMO, the restricted mode should be deprecated. In any case, we
> should at least warn against using it. Below was my attempt
> which could definitely be improved, I hope it can serve as a
> basic for discussio.

It does seems reasonable to have something like this, but I'm not sure
I'm feeling like I can do any particularly useful editing myself.  If
nobody has any immediate comments we can simply put the whole thing in,
where it obviously remains available for any follow up changes.

pws


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities)
  2019-05-31 12:05     ` [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities) Stephane Chazelas
  2019-06-03  9:35       ` Peter Stephenson
@ 2019-06-04  2:39       ` dana
  2019-06-04  7:34         ` dana
  1 sibling, 1 reply; 34+ messages in thread
From: dana @ 2019-06-04  2:39 UTC (permalink / raw)
  To: Stephane Chazelas; +Cc: zsh-workers

Without bike-shedding too much, i did notice some trivial formatting and
punctuation issues:

On 31 May 2019, at 07:05, Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> +via the predefined `tt(PATH)` variable, it doesn't prevent those

The quote usage isn't consistent on these. AFAIR, the convention is for all of
them to be `tt(...)' (tilde to open, single-quote to close)

On 31 May 2019, at 07:05, Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> +arbitrary code by setting a `tt(GCONV_PATH)' environment variable, those
> +are only a few examples.

'Those are...' should start a new sentence

On 31 May 2019, at 07:05, Stephane Chazelas <stephane.chazelas@gmail.com> wrote:
> +Bear in mind that contrary to some other shells, `tt(readonly)' is not a

'Bear in mind that, contrary to other shells, ...' (add comma)

dana


^ permalink raw reply	[flat|nested] 34+ messages in thread

* Re: [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities)
  2019-06-04  2:39       ` dana
@ 2019-06-04  7:34         ` dana
  0 siblings, 0 replies; 34+ messages in thread
From: dana @ 2019-06-04  7:34 UTC (permalink / raw)
  To: Stephane Chazelas; +Cc: zsh-workers

On 3 Jun 2019, at 21:39, dana <dana@dana.is> wrote:
> The quote usage isn't consistent on these. AFAIR, the convention is for all of
> them to be `tt(...)' (tilde to open, single-quote to close)

I meant back-tick instead of tilde here, obv. Couldn't let that go

dana


^ permalink raw reply	[flat|nested] 34+ messages in thread

end of thread, other threads:[~2019-06-04  7:35 UTC | newest]

Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-10 15:03 Zsh - Multiple DoS Vulnerabilities David Wells
2019-05-10 16:37 ` Bart Schaefer
2019-05-12 16:21   ` Stephane Chazelas
2019-05-13 16:29     ` David Wells
2019-05-13 22:02       ` Bart Schaefer
2019-05-14 18:10       ` Stephane Chazelas
2019-05-14 21:24         ` Daniel Shahaf
2019-05-14 21:38           ` Bart Schaefer
2019-05-14 21:39         ` Daniel Shahaf
2019-05-14 22:25           ` Bart Schaefer
2019-05-15 10:48             ` Daniel Shahaf
2019-05-31 12:05     ` [PATCH] [doc] [repost] warnings about restricted shell (Was: Zsh - Multiple DoS Vulnerabilities) Stephane Chazelas
2019-06-03  9:35       ` Peter Stephenson
2019-06-04  2:39       ` dana
2019-06-04  7:34         ` dana
2019-05-10 20:27 ` Zsh - Multiple DoS Vulnerabilities Bart Schaefer
2019-05-11  1:45   ` #7 (typeset -Tp) (was Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
2019-05-13  9:01     ` Peter Stephenson
2019-05-13 21:11   ` PATCH: #6 negative job id (Re: " Oliver Kiddle
2019-05-13 21:44   ` Zsh - Multiple DoS Vulnerabilities Oliver Kiddle
2019-05-13 22:36   ` #3 typeset and braces (Re: Zsh - Multiple DoS Vulnerabilities) Oliver Kiddle
2019-05-14  0:13     ` Mikael Magnusson
2019-05-14  5:38       ` Bart Schaefer
2019-05-14 10:50     ` Peter Stephenson
2019-05-14 16:38   ` Zsh - Multiple DoS Vulnerabilities Peter Stephenson
2019-05-14 20:30   ` Oliver Kiddle
2019-05-15 16:50     ` Mikael Magnusson
2019-05-16 20:37     ` Peter Stephenson
2019-05-17 13:41       ` Mikael Magnusson
2019-05-17 13:51         ` Mikael Magnusson
2019-05-17 14:28           ` Mikael Magnusson
2019-05-18 10:31           ` Oliver Kiddle
2019-05-21 14:43             ` Oliver Kiddle
     [not found]               ` <CGME20190521154256eucas1p1f0816d2467abd8bf4a0c31058af2983a@eucas1p1.samsung.com>
2019-05-21 15:42                 ` Peter Stephenson

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).