[PATCH] avoid closed stdin() in zle widgets

[PATCH] avoid closed stdin() in zle widgets

[Stephane Chazelas]

Hello,

in zle widgets, stdin currently appears to be closed. That's
generally not a good idea to close fds 0, 1, 2 as many commands
are confused when the files they open suddenly become their
stdin/stdout/stderr because the first free fd is 0/1/2.

See

https://unix.stackexchange.com/q/370475

for instance.

diff --git a/Doc/Zsh/builtins.yo b/Doc/Zsh/builtins.yo
index 81687c7..0e26da3 100644
--- a/Doc/Zsh/builtins.yo
+++ b/Doc/Zsh/builtins.yo
@@ -2012,7 +2012,8 @@ enditem()
 Attribute flags that transform the final value (tt(-L), tt(-R), tt(-Z),
 tt(-l), tt(-u)) are only applied to the expanded value at the point
 of a parameter expansion expression using `tt($)'.  They are not applied
-when a parameter is retrieved internally by the shell for any purpose. 
+when a parameter is retrieved internally by the shell for any purpose.
+They have no effect on array or associative array parameters.
 
 The following attribute flags may be specified:
 
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 6c271b5..be2b062 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1485,6 +1485,13 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	    int inuse = w->flags & WIDGET_INUSE;
 	    w->flags |= WIDGET_INUSE;
 
+	    if (osi > 0) {
+		/*
+		 * Many commands don't like having a closed stdin, open on
+		 * /dev/null instead
+		 */
+		open("/dev/null", O_RDWR | O_NOCTTY); /* ignore failure */
+	    }
 	    if (*args) {
 		largs = newlinklist();
 		addlinknode(largs, dupstring(w->u.fnnam));

Re: [PATCH] avoid closed stdin() in zle widgets

[Eric Cook]

On 06/11/2017 02:20 PM, Stephane Chazelas wrote:
> Hello,
> 
> in zle widgets, stdin currently appears to be closed. That's
> generally not a good idea to close fds 0, 1, 2 as many commands
> are confused when the files they open suddenly become their
> stdin/stdout/stderr because the first free fd is 0/1/2.
> 
> See
> 
> https://unix.stackexchange.com/q/370475

It is documented to work like that in zshzle

---
USER-DEFINED WIDGETS
       User-defined  widgets,  being implemented as shell functions, can execute any normal shell command.  They can also run other
       widgets (whether built-in or user-defined) using the zle builtin command.  The standard input of the function is  closed  to
       prevent external commands from unintentionally blocking ZLE by reading from the terminal, but read -k or read -q can be used
       to read characters.  Finally, they can examine and edit the ZLE buffer being edited  by  reading  and  setting  the  special
       parameters described below.

[PATCH2] Re: avoid closed stdin() in zle widgets

[Stephane Chazelas]

2017-06-12 00:15:20 -0400, Eric Cook:
[...]
> > in zle widgets, stdin currently appears to be closed. That's
> > generally not a good idea to close fds 0, 1, 2 as many commands
> > are confused when the files they open suddenly become their
> > stdin/stdout/stderr because the first free fd is 0/1/2.
[...]
> It is documented to work like that in zshzle
[...]

Well spotted. Thanks. (I also left a diff by mistake for an
unrelated issue discussed earlier on. Sorry about that).

New patch:

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index b65e3be..bd0252f 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -750,12 +750,12 @@ sect(User-Defined Widgets)
 cindex(widgets, user-defined)
 User-defined widgets, being implemented as shell functions,
 can execute any normal shell command.  They can also run other widgets
-(whether built-in or user-defined) using the tt(zle) builtin command.
-The standard input of the function is closed to prevent external commands
-from unintentionally blocking ZLE by reading from the terminal, but
-tt(read -k) or tt(read -q) can be used to read characters.  Finally,
-they can examine and edit the ZLE buffer being edited by
-reading and setting the special parameters described below.
+(whether built-in or user-defined) using the tt(zle) builtin command. The
+standard input of the function is redirected from /dev/null to prevent
+external commands from unintentionally blocking ZLE by reading from the
+terminal, but tt(read -k) or tt(read -q) can be used to read characters.
+Finally, they can examine and edit the ZLE buffer being edited by reading
+and setting the special parameters described below.
 
 cindex(parameters, editor)
 cindex(parameters, zle)
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 6c271b5..be2b062 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1485,6 +1485,13 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	    int inuse = w->flags & WIDGET_INUSE;
 	    w->flags |= WIDGET_INUSE;
 
+	    if (osi > 0) {
+		/*
+		 * Many commands don't like having a closed stdin, open on
+		 * /dev/null instead
+		 */
+		open("/dev/null", O_RDWR | O_NOCTTY); /* ignore failure */
+	    }
 	    if (*args) {
 		largs = newlinklist();
 		addlinknode(largs, dupstring(w->u.fnnam));

Re: [PATCH2] Re: avoid closed stdin() in zle widgets

[Daniel Shahaf]

Stephane Chazelas wrote on Mon, 12 Jun 2017 07:05 +0100:
> 2017-06-12 00:15:20 -0400, Eric Cook:
> [...]
> > > in zle widgets, stdin currently appears to be closed. That's
> > > generally not a good idea to close fds 0, 1, 2 as many commands
> > > are confused when the files they open suddenly become their
> > > stdin/stdout/stderr because the first free fd is 0/1/2.
> [...]
> > It is documented to work like that in zshzle
> [...]
> 
> Well spotted. Thanks. (I also left a diff by mistake for an
> unrelated issue discussed earlier on. Sorry about that).
> 
> New patch:
> 
> diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
> index b65e3be..bd0252f 100644
> --- a/Doc/Zsh/zle.yo
> +++ b/Doc/Zsh/zle.yo
> @@ -750,12 +750,12 @@ sect(User-Defined Widgets)

Incompatible changes should be mentioned in NEWS or README.

I'm not convinced that the change should be made.  Devil's advocate
argues that this is a bug in dircolors(1), so (a) as an immediate
workaround, people with a buggy dircolors(1) command should redirect
stdin from /dev/null when invoking it, (b) those people should fix their
dircolors(1) command to not assume fd 0 is open.

Daniel

Re: [PATCH2] Re: avoid closed stdin() in zle widgets

[Stephane Chazelas]

2017-06-12 14:34:49 +0000, Daniel Shahaf:
[...]
> Incompatible changes should be mentioned in NEWS or README.
[...]
> I'm not convinced that the change should be made.  Devil's advocate
> argues that this is a bug in dircolors(1), so (a) as an immediate
> workaround, people with a buggy dircolors(1) command should redirect
> stdin from /dev/null when invoking it, (b) those people should fix their
> dircolors(1) command to not assume fd 0 is open.
[...]

While I agree there's a bug in dircolors, closing stdin, stdout
or stderr is widely regarded as antisocial and bad practice
(a number of security vulnerabilities could be exploited in the
past by closing 0,1,2. glibc does reopen stdin on /dev/full and
stdout/stderr on /dev/null (apparently) when they are closed
when running in setid/setgid (AT_SECURE) executables to guard
against this type of abuse).

-- 
Stephane

[PATCH3] Re: avoid closed stdin() in zle widgets

[Stephane Chazelas]

Even though I can't imagine previous relying on stdin being
closed (and consider myself the previous behaviour a bug),
please find a 3rd version of the patch below with the requested
change:

diff --git a/Doc/Zsh/zle.yo b/Doc/Zsh/zle.yo
index b65e3be..bd0252f 100644
--- a/Doc/Zsh/zle.yo
+++ b/Doc/Zsh/zle.yo
@@ -745,22 +745,22 @@ User-defined widgets are defined using `tt(zle -N)', and implemented
 as shell functions.  When the widget is executed, the corresponding
 shell function is executed, and can perform editing (or other) actions.
 It is recommended that user-defined widgets should not have names
 starting with `tt(.)'.
 sect(User-Defined Widgets)
 cindex(widgets, user-defined)
 User-defined widgets, being implemented as shell functions,
 can execute any normal shell command.  They can also run other widgets
-(whether built-in or user-defined) using the tt(zle) builtin command.
-The standard input of the function is closed to prevent external commands
-from unintentionally blocking ZLE by reading from the terminal, but
-tt(read -k) or tt(read -q) can be used to read characters.  Finally,
-they can examine and edit the ZLE buffer being edited by
-reading and setting the special parameters described below.
+(whether built-in or user-defined) using the tt(zle) builtin command. The
+standard input of the function is redirected from /dev/null to prevent
+external commands from unintentionally blocking ZLE by reading from the
+terminal, but tt(read -k) or tt(read -q) can be used to read characters.
+Finally, they can examine and edit the ZLE buffer being edited by reading
+and setting the special parameters described below.
 
 cindex(parameters, editor)
 cindex(parameters, zle)
 These special parameters are always available in widget functions, but
 are not in any way special outside ZLE.  If they have some normal value
 outside ZLE, that value is temporarily inaccessible, but will return
 when the widget function exits.  These special parameters in fact have
 local scope, like parameters created in a function using tt(local).
diff --git a/NEWS b/NEWS
index 568b160..e745750 100644
--- a/NEWS
+++ b/NEWS
@@ -9,16 +9,22 @@ Changes from 5.3.1 to 5.4
 
 The 'exec' and 'command' precommand modifiers, and options to them, are
 now parsed after parameter expansion.  Previously, both the modifier and
 any options to it were parsed between alias expansion and parameter
 expansion (see zshexpn(1)), so they could neither be quoted nor be the
 result of parameter expansion.  Examples: 's=command; $s -V ls' and
 '\command -V ls' now work as expected.
 
+Functions executed by ZLE widgets no longer have they standard input
+closed, but is now redirected from /dev/null instead. That still guards
+against user defined widgets inadvertently reading from the tty device,
+and addresses the antisocial behaviour of running a command with its
+stdin closed.
+
 
 Changes from 5.2 to 5.3.1
 -------------------------
 
 There are only minor compatibility fixes between 5.3 and 5.3.1.
 
 It is possible to enable character width support for Unicode 9 by
 configuring with `--enable-unicode9'; this compiles in some additional
diff --git a/README b/README
index 432a35e..30023da 100644
--- a/README
+++ b/README
@@ -69,16 +69,22 @@ patch-format string, to prevent literal `%' signs in the interpolated
 value from being interpreted as prompt escape sequences.  If you use
 ${vcs_info_msg_0_} in a context other than the shell prompt, you may need
 to undo the escaping with:
 
     print -v vcs_info_msg_0_ -Pr -- "${vcs_info_msg_0_}"
 
 This is also needed if $vcs_info_msg_0_ is used to set $psvar.
 
+4) functions executed by ZLE widgets no longer have they standard input
+closed, but is now redirected from /dev/null instead. That still guards
+against user defined widgets inadvertently reading from the tty device,
+and addresses the antisocial behaviour of running a command with its
+stdin closed.
+
 Incompatibilities between 5.0.8 and 5.3
 ----------------------------------------
 
 1) In character classes delimited by "[" and "]" within patterns, whether
 used for filename generation (globbing) or other forms of pattern
 matching, it used not to be possible to quote "-" when used for a range,
 or "^" and "!" when used for negating a character set.  The characters can
 now be quoted by any of the standard shell means, but note that
diff --git a/Src/Zle/zle_main.c b/Src/Zle/zle_main.c
index 6c271b5..be2b062 100644
--- a/Src/Zle/zle_main.c
+++ b/Src/Zle/zle_main.c
@@ -1480,16 +1480,23 @@ execzlefunc(Thingy func, char **args, int set_bindk)
 	    ret = execimmortal(func, args);
 	} else {
 	    int osc = sfcontext, osi = movefd(0);
 	    int oxt = isset(XTRACE);
 	    LinkList largs = NULL;
 	    int inuse = w->flags & WIDGET_INUSE;
 	    w->flags |= WIDGET_INUSE;
 
+	    if (osi > 0) {
+		/*
+		 * Many commands don't like having a closed stdin, open on
+		 * /dev/null instead
+		 */
+		open("/dev/null", O_RDWR | O_NOCTTY); /* ignore failure */
+	    }
 	    if (*args) {
 		largs = newlinklist();
 		addlinknode(largs, dupstring(w->u.fnnam));
 		while (*args)
 		    addlinknode(largs, dupstring(*args++));
 	    }
 	    startparamscope();
 	    makezleparams(0);

Re: [PATCH3] Re: avoid closed stdin() in zle widgets

[Daniel Shahaf]

Stephane Chazelas wrote on Mon, 12 Jun 2017 16:19 +0100:
> Even though I can't imagine previous relying on stdin being
> closed (and consider myself the previous behaviour a bug),
> please find a 3rd version of the patch below with the requested
> change:

Thanks for adding the README hunk.  I assumed the behaviour was
intentional because it was documented (and it didn't seem
"antisocial" to me, however surprised a main() function might be to
find its stdin closed).

Cheers,

Daniel

Re: [PATCH3] Re: avoid closed stdin() in zle widgets

[Bart Schaefer]

On Mon, Jun 12, 2017 at 8:34 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Stephane Chazelas wrote on Mon, 12 Jun 2017 16:19 +0100:
>> please find a 3rd version of the patch below with the requested
>> change:
>
> it didn't seem
> "antisocial" to me, however surprised a main() function might be

Yeah, maybe I'm just getting crotchety but that remark seemed a bit
snarky for a NEWS entry.

Re: [PATCH3] Re: avoid closed stdin() in zle widgets

[Stephane Chazelas]

2017-06-12 09:09:23 -0700, Bart Schaefer:
> On Mon, Jun 12, 2017 at 8:34 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Stephane Chazelas wrote on Mon, 12 Jun 2017 16:19 +0100:
> >> please find a 3rd version of the patch below with the requested
> >> change:
> >
> > it didn't seem
> > "antisocial" to me, however surprised a main() function might be
> 
> Yeah, maybe I'm just getting crotchety but that remark seemed a bit
> snarky for a NEWS entry.

Please feel free to adjust to "proper English", English is not
my mother tongue. That's inspired from the Unix v7 (1979)
wording for "rm" where rm won't accept to remove ".." because of
the "antisocial behaviour" of "rm -rf .*".

My point is that a command should be able to reasonably make some
assumptions, like:

- stdin should be open for at least reading
- stdout, stderr should be open for at least writing
- argv[0] should be set (argc > 0)
- the SIGCHLD handler should not be SIGIGN
- some reasonable limits (as in, not too low) on stacksize,
  datasize, descriptors, filesize
- no dups in the environment
- we could even maybe extend that to some variables like
  FIGNORE, IFS, CDPATH, PS1 not in the environment
- and all the possible corner cases you can imagine that only
  perverted applications would ever subject you to.

If you code can't handle any of those, arguably, you've got a
bug, but one may also argue that the environment is to
blame and not the application.

Like if you call an application with no argv[] or no stdin and
it segfaults, you asked for it, you've been antisocial. 

(of course, there's the problem of those bugs being exploited in
setuid/setgid contexts but here the problem is the
setuid/setgid that is simply not workable. And yes, we have to
deal with it, we can't just ignore it).

$ (limit stacksize 100k; zsh)
zsh: segmentation fault
zsh: segmentation fault

(twice!?). Is that a bug?

-- 
Stephane

Re: [PATCH3] Re: avoid closed stdin() in zle widgets

[Bart Schaefer]

On Jun 12,  8:02pm, Stephane Chazelas wrote:
}
} My point is that a command should be able to reasonably make some
} assumptions, like:
} 
} - stdin should be open for at least reading
} - stdout, stderr should be open for at least writing

Although I understand the security implication of accidentally opening
some other file onto fd 0/1/2, I can't agree with the above statements.
Taken to the logical conclusion, the >&- <&- or 2>&- operators would
be required always to fail with an error.

It should not be the shell's job to plug this hole.  I will agree that
a valid argument is that the shell should not implicitly *open* this
hole, which one could also argue is what the completion system had
been doing in spite of the behavior being documented.  However, with
these two likely exceptions --

} - argv[0] should be set (argc > 0)
} - no dups in the environment

-- there is nothing else on your list where I would agree that the
shell should ignore the user's directives in the name of protecting
an external command from itself.

} $ (limit stacksize 100k; zsh)
} zsh: segmentation fault
} zsh: segmentation fault
} 
} (twice!?). Is that a bug?

I believe what's happening is that both the zsh inside the subshell
and the parent handling the subshell exit are reporting the error,
so one failure / two messages.

However, I can't test directly because I can start zsh -f with a hard
limit stack size of *zero*, so I'm quite curious as to why you get a
crash on 100k.

Re: [PATCH3] Re: avoid closed stdin() in zle widgets

[Stephane Chazelas]

2017-06-14 15:44:25 -0700, Bart Schaefer:
> On Jun 12,  8:02pm, Stephane Chazelas wrote:
> }
> } My point is that a command should be able to reasonably make some
> } assumptions, like:
> } 
> } - stdin should be open for at least reading
> } - stdout, stderr should be open for at least writing
> 
> Although I understand the security implication of accidentally opening
> some other file onto fd 0/1/2, I can't agree with the above statements.
> Taken to the logical conclusion, the >&- <&- or 2>&- operators would
> be required always to fail with an error.
> 
> It should not be the shell's job to plug this hole.  I will agree that
> a valid argument is that the shell should not implicitly *open* this
> hole, which one could also argue is what the completion system had
> been doing in spite of the behavior being documented.  However, with
> these two likely exceptions --
> 
> } - argv[0] should be set (argc > 0)
> } - no dups in the environment
> 
> -- there is nothing else on your list where I would agree that the
> shell should ignore the user's directives in the name of protecting
> an external command from itself.

I think you misinterpreted what I said, I did not imply that the
shell should take upon itself to prevent users from creating
those pathological conditions, but that it should not take upon
itself to creating those pathological conditions itself.

That's the "Be conservative in what you do" in the "Be
conservative in what you do, be liberal in what you accept from
others" (and yes, it's a case where "dircolors" did not fully
apply the "be liberal in what you accept").

In other words, I would certainly not want zsh to refuse to <&-
just like I would not want close(0) in C to fail.

I would even welcome new options to the "env" utility to execute
a command without arguments or with arbitrary argv[0], or with
duplicate env vars or with env strings without = characters so
one can test applications in those pathological conditions (and
possibly raise awareness on the security implications), but
if we put aside those testing cases, an application like zsh
should not intentionaly (by itself) cause those pathological
conditions.

The case of zle widgets running commands with stdin close was
not a case where the user requested stdin to be closed.

> } $ (limit stacksize 100k; zsh)
> } zsh: segmentation fault
> } zsh: segmentation fault
> } 
> } (twice!?). Is that a bug?
> 
> I believe what's happening is that both the zsh inside the subshell
> and the parent handling the subshell exit are reporting the error,
> so one failure / two messages.
> 
> However, I can't test directly because I can start zsh -f with a hard
> limit stack size of *zero*, so I'm quite curious as to why you get a
> crash on 100k.

I suspect your system won't bring the stack size below some
threshold (or just ignores that  limit). With a null stack size,
you woudn't even be able to call execve()

My several segvs were probably down to some child processes
spawned by my ~/.zshrc dying there upon stack overflow.

The point was just an illustration that you can't always deal
with all pathological conditions, you have to put a limit on
the amount of effort you're willing to put into covering for all
possible pathological cases, and IMO a closed stdin is one such
pathological case (even if not as much as a very small stack
size, or random memory bit flips), and one you can blame on the
calling application.

-- 
Stephane

Re: [PATCH3] Re: avoid closed stdin() in zle widgets

[Bart Schaefer]

On Jun 15,  9:42am, Stephane Chazelas wrote:
}
} In other words, I would certainly not want zsh to refuse to <&-
} just like I would not want close(0) in C to fail.
} 
} The case of zle widgets running commands with stdin close was
} not a case where the user requested stdin to be closed.

So, just being contrarian at this point, if _main_complete were to have
done "exec <&-" that would be OK, but because the C code does it we
have to patch it.

The point being that the completion system does LOTS of things that the
user didn't explicitly request, but requested implicitly by choosing to
use it.  It traps various signals, throws away error output, for some
commands even runs "su" or "sudo" -- although the latter are probably
the best analogy for this because we added _comp_priv_prefix.