Re: [PATCH v2 properly indented] fix start_command() bug when stdin is closed

Paolo Bonzini [off-list ref] writes:

 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
 	int fdin[2], fdout[2], fderr[2];
 
 	/*
+	 * Make sure that all file descriptors <= 2 are open, otherwise we
+	 * mess them up when dup'ing pipes onto stdin/stdout/stderr.  Since
+	 * we are at it, save a file descriptor on /dev/null to use it later.
+	 */
+	if (devnull_fd == -1) {
+		devnull_fd = open("/dev/null", O_RDWR);
+		while (devnull_fd >= 0 && devnull_fd <= 2)
+			devnull_fd = dup(devnull_fd);
+		if (devnull_fd == -1)
+			die("opening /dev/null failed (%s)", strerror(errno));
+	}
+

I may be misreading the patch but, this logic always opens /dev/null, if
nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and
worse, it keeps fd=3 open.

Making sure low fds 0, 1 and 2 are open is a good thing.  I do not think
clobbering fd=3 is good.

Also shouldn't this be done only on the side that dup()s fds around,
i.e. in the child process after fork()?  Why is this done for the parent?

Junio C Hamano schrieb:

Paolo Bonzini [off-list ref] writes:

quoted

 int start_command(struct child_process *cmd)
 {
 	int need_in, need_out, need_err;
 	int fdin[2], fdout[2], fderr[2];
 
 	/*
+	 * Make sure that all file descriptors <= 2 are open, otherwise we
+	 * mess them up when dup'ing pipes onto stdin/stdout/stderr.  Since
+	 * we are at it, save a file descriptor on /dev/null to use it later.
+	 */
+	if (devnull_fd == -1) {
+		devnull_fd = open("/dev/null", O_RDWR);
+		while (devnull_fd >= 0 && devnull_fd <= 2)
+			devnull_fd = dup(devnull_fd);
+		if (devnull_fd == -1)
+			die("opening /dev/null failed (%s)", strerror(errno));
+	}
+

I may be misreading the patch but, this logic always opens /dev/null, if
nobody asked for *any* cmd->no_stdXXX and low 3 fds are occupied, and
worse, it keeps fd=3 open.

Making sure low fds 0, 1 and 2 are open is a good thing.  I do not think
clobbering fd=3 is good.

It is sometimes _unnecessary_, but I don't see why it should hurt. The
effect on performance will be in the noise.

Also shouldn't this be done only on the side that dup()s fds around,
i.e. in the child process after fork()?  Why is this done for the parent?

Because it must be done *before* the pipe()s are created so that they
don't occupy fds 0-2.

-- Hannes

Hannes already answered everything, but now that I think more about it I
would actually consider putting it in main, just in case an important
file ends up in file descriptor = 2 and is corrupted by a call to die().
 Just in case, this program shows that stderr always point to fd 2 even
if it is closed upon launch:

  #include <stdio.h>
  #include <fcntl.h>

  int main()
  {
    int fd = open ("/dev/tty", O_WRONLY);
    FILE *fp;
    fp = fdopen (fd, "w");
    fprintf (fp, "file descriptor %d\n", fd);
    fflush (fp);
    fprintf (stderr, "writing on stderr now\n");
  }

  bonzinip$ ./a.out 2<&-
  file descriptor 2
  writing on stderr now
  bonzinip$

Patch coming in a moment.

Paolo

It is in general unsafe to start git with one or more of file descriptors
0/1/2 closed.  Karl Chen for example noticed that stat_command does this
in order to rename a pipe file descriptor to 0:

    dup2(from, 0);
    close(from);

... but if stdin was closed (for example) from == 0, so that

    dup2(0, 0);
    close(0);

just ends up closing the pipe.  Another extremely rare but nasty problem
would occur if an "important" file ends up in file descriptor 2, and is
corrupted by a call to die().

This patch fixes these problems by opening all of the "low" descriptors
to /dev/null in main.

Signed-off-by: Paolo Bonzini <redacted>
---
 git.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/git.c b/git.c
index 89e4645..be227b2 100644
--- a/git.c
+++ b/git.c

@@ -420,6 +420,19 @@ int main(int argc, const char **argv)
 	const char *cmd = argv[0] && *argv[0] ? argv[0] : "git-help";
 	char *slash = (char *)cmd + strlen(cmd);
 	int done_alias = 0;
+	int devnull_fd;
+
+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids not messing up when the pipes are
+	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+	 */
+	devnull_fd = open("/dev/null", O_RDWR);
+	while (devnull_fd >= 0 && devnull_fd <= 2)
+		devnull_fd = dup(devnull_fd);
+	if (devnull_fd == -1)
+		die("opening /dev/null failed (%s)", strerror(errno));
+	close (devnull_fd);
 
 	/*
 	 * Take the basename of argv[0] as the command

-- 
1.5.5

Paolo Bonzini schrieb:

+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids not messing up when the pipes are
+	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+	 */

I see your point, but I don't have an opinion whether this stretch is
necessary.

However, *if* we do this, we must do it for all non-builtins as well!

-- Hannes

Johannes Sixt wrote:

Paolo Bonzini schrieb:

quoted

+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids not messing up when the pipes are
+	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+	 */

I see your point, but I don't have an opinion whether this stretch is
necessary.
However, *if* we do this, we must do it for all non-builtins as well!

Well, in general the policy I've used in all the tools I created is that:

a. If it's a setuid tool, then you need to make sure that you don't step
   on anything unintendedly.  I.e. for setuid-something programs this is
   desirable and necessary in order to prevent securityleaks.

b. Anything else is started in an environment controlled by the user,
   and if this environment is broken, then that is the user's fault.
   You get what you wish for.  It's a similar problem you get when you
   set PATH to wrong values and then start "make" for example; it has
   the potential to break a lot; but then again there are infinitely
   more ways to shoot yourself in the foot, than there are ways to
   prevent people from shooting in some particular way.

So I'd say, if the tools are setuid (which none of git's tools are) and
are therefore potentially started from a hostile and uncontrolled
environment, please make sure filedescriptors 0, 1 and 2 are sane.
But for the git utilities, it would be a non-watertight extra safeguard
which tries to prevent a situation which rarely occurs and if it does
occur, you probably are doing some other things wrong as well; so
actually exposing those problems to you by letting you feel the pain can
be considered a favour.
-- 
Sincerely,
           Stephen R. van den Berg.

"Good moaning!"

On 8/26/08, Stephen R. van den Berg [off-list ref] wrote:

Well, in general the policy I've used in all the tools I created is that:

 a. If it's a setuid tool, then you need to make sure that you don't step
   on anything unintendedly.  I.e. for setuid-something programs this is
   desirable and necessary in order to prevent securityleaks.

 b. Anything else is started in an environment controlled by the user,
   and if this environment is broken, then that is the user's fault.

In general I'd mostly agree with you, but fd 0/1/2 are super-special
and I've personally been bitten by insane, rare problems that occur
when programs are started with one or more of those fds closed.

The usual case is that you're writing a new daemon.  The generally
accepted behaviour for a daemon is to chdir("/') and then close all
unnecessary open fds, in order to minimize the chance that it will be
holding open any directories or files that would prevent unmounting a
filesystem.  On the other hand, if the daemon then needs to run git
for some reason (who knows! maybe it's a git auto-commit daemon as was
discussed earlier on the list), it needs to open file descriptors
instead.  Such a program might work 99% of the time when git doesn't
happen to print any output.  But if there's ever an error, git would
print to fd#2 on die(), and that could corrupt some random file that
the daemon *or* git was using.  Remember, the situations where the
daemon leaves fd#2 open pointing at *the wrong thing* aren't the real
problem - you could easily say that's the daemon leaving the
environment in an insane state.  The problem situation is when git
opened some random file, and it *happened* to get assigned fd#2, and
then git incorrectly assumed that writing to fd#2 would not corrupt a
file that it opened.

Does this sound rare?  It is!  But it's also hellish to debug when it
happens, precisely because of its rarity.  For example, in one case, I
had this problem because an sfdisk process started by my custom
/sbin/init ran into a minor warning, and printed it to fd#2.
Unfortunately, because /sbin/init had opened sfdisk with fds 0/1/2
closed, fd#2 ended up being the very disk it was partitioning.  The
boot sector ended up getting overwritten with a warning message in
something like 1 out of 100 cases, and the computer wouldn't boot.
ARGH.  Easy to debug, once you think to read the boot sector as
plaintext.  But that's not the first thing you think to do.

Anyway, I personally think that given how incredibly cheap this
operation is to do, and how startlingly painful it is to debug when it
*is* a problem, that it would be nice if every program just did this
by default.  I would personally feel fine if such a thing ended up in
libc or the kernel, although presumably that would violate POSIX.

Yes, it would also be fine to have every *daemon* make sure it opens
/dev/null instead of just closing fd 0/1/2.  But it's harmless to have
both.

(As for the non-builtin git commands, isn't this an advantage of
having everything get run through the main /usr/bin/git wrapper?)

Have fun,

Avery

Avery Pennarun wrote:

On 8/26/08, Stephen R. van den Berg [off-list ref] wrote:

quoted

Well, in general the policy I've used in all the tools I created is that:

quoted

 a. If it's a setuid tool, then you need to make sure that you don't step
   on anything unintendedly.  I.e. for setuid-something programs this is
   desirable and necessary in order to prevent securityleaks.

quoted

 b. Anything else is started in an environment controlled by the user,
   and if this environment is broken, then that is the user's fault.

In general I'd mostly agree with you, but fd 0/1/2 are super-special
and I've personally been bitten by insane, rare problems that occur
when programs are started with one or more of those fds closed.

Key words: "insane, rare problems"

The usual case is that you're writing a new daemon.  The generally
accepted behaviour for a daemon is to chdir("/') and then close all

the daemon *or* git was using.  Remember, the situations where the
daemon leaves fd#2 open pointing at *the wrong thing* aren't the real
problem - you could easily say that's the daemon leaving the
environment in an insane state.  The problem situation is when git

Well, as you say, "you're writing a new daemon".  This means that you
need to make sure that *if* this daemon ever forks/execs it leaves the
environment in a sane state which does not open up security holes.
That means that you need to sanitise the environment, that you need to
keep tabs on all the descriptors that need to be closed on exec, and
that it needs to make sure that fd 0, 1 and 2 are pointing to somewhere
appriopriate (/dev/null, if nothing else).
The fact that you forgot to do some of those things means that it is
very helpful if you discover this fact as soon as possible.  By making
git "fix it for you" you will not notice any problems when running git
from your daemon.  Nonetheless this will not cause you to fix your
code.

Does this sound rare?  It is!  But it's also hellish to debug when it
happens, precisely because of its rarity.  For example, in one case, I
had this problem because an sfdisk process started by my custom

Thing is, by making git (and some other programs) hide this problem
from you, this problem will get even *harder* to debug.  Whereas as a
daemon author you should be thankful that something breaks and shows you
your daemon needs fixing.

Anyway, I personally think that given how incredibly cheap this
operation is to do, and how startlingly painful it is to debug when it
*is* a problem, that it would be nice if every program just did this
by default.  I would personally feel fine if such a thing ended up in
libc or the kernel, although presumably that would violate POSIX.

And then you'd have the situation that in some cases, where this
mechanism is bypassed or not present, various daemons might show security
holes in their filedescriptor management which nobody noticed before.

Yes, it would also be fine to have every *daemon* make sure it opens
/dev/null instead of just closing fd 0/1/2. 

It would not only be fine, it *is* required, since 1972.

But it's harmless to have
both.

Considering the fact that daemon authors might not get pointed at their
mistakes as soon as possible, it is harmful to try and hide those facts.

(As for the non-builtin git commands, isn't this an advantage of
having everything get run through the main /usr/bin/git wrapper?)

At best a program (e.g. git) could give off a warning when it finds
the filedescriptors in a bad state (and then fix it), but that would
mean that from within git we'd be trying to save the world, since anyone
not running git would not get those warnings.  You have to draw the line
somewhere, and for user-tools it ends here; for setuid-tools it's
different: they need to detect and fix it anyway, and therefore could easily
warn as well.
-- 
Sincerely,
           Stephen R. van den Berg.
"First, God created idiots.  That was just for practice.
 Then he created school boards."  --  Mark Twain

quoted

But it's harmless to have both.

Considering the fact that daemon authors might not get pointed at their
mistakes as soon as possible, it is harmful to try and hide those facts.

Agree.  OTOH what about opening fd's 0/1/2 to /dev/null only in
git-shell.c, now that it's not a builtin anymore?

Maybe it does not fix Karl's use case, but it seems sensible to me.

Paolo

It is in general unsafe to start a program with one or more of file
descriptors 0/1/2 closed.  Karl Chen for example noticed that stat_command
does this in order to rename a pipe file descriptor to 0:

    dup2(from, 0);
    close(from);

... but if stdin was closed (for example) from == 0, so that

    dup2(0, 0);
    close(0);

just ends up closing the pipe.  Another extremely rare but nasty problem
would occur if an "important" file ends up in file descriptor 2, and is
corrupted by a call to die().

Fixing this in git was considered to be overkill, so this patch works
around it only for git-shell.  The fix is simply to open all the "low"
descriptors to /dev/null in main.

Signed-off-by: Paolo Bonzini <redacted>
---
 shell.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/shell.c b/shell.c
index 0f6a727..e339369 100644
--- a/shell.c
+++ b/shell.c

@@ -48,6 +48,19 @@ int main(int argc, char **argv)
 {
 	char *prog;
 	struct commands *cmd;
+	int devnull_fd;
+
+	/*
+	 * Always open file descriptors 0/1/2 to avoid clobbering files
+	 * in die().  It also avoids not messing up when the pipes are
+	 * dup'ed onto stdin/stdout/stderr in the child processes we spawn.
+	 */
+	devnull_fd = open("/dev/null", O_RDWR);
+	while (devnull_fd >= 0 && devnull_fd <= 2)
+		devnull_fd = dup(devnull_fd);
+	if (devnull_fd == -1)
+		die("opening /dev/null failed (%s)", strerror(errno));
+	close (devnull_fd);
 
 	/*
 	 * Special hack to pretend to be a CVS server

-- 
1.5.5

Paolo Bonzini wrote:

Fixing this in git was considered to be overkill, so this patch works
around it only for git-shell.  The fix is simply to open all the "low"
descriptors to /dev/null in main.

Since git-shell is not setuid, this strictly is not necessary, however,
I concur that git-shell is potentially started in a partially broken
environment which is not always easily fixable by the user.
And since git-shell needs to sanitise the filedescriptors anyway before
launching other programs, it might as well cleanup at startup if needed.

Acked-by: Stephen R. van den Berg <redacted>
-- 
Sincerely,
           Stephen R. van den Berg.
"First, God created idiots.  That was just for practice.
 Then he created school boards."  --  Mark Twain

On Wed, Aug 27, 2008 at 5:18 AM, Stephen R. van den Berg [off-list ref] wrote:

Avery Pennarun wrote:

quoted

In general I'd mostly agree with you, but fd 0/1/2 are super-special
and I've personally been bitten by insane, rare problems that occur
when programs are started with one or more of those fds closed.

Key words: "insane, rare problems"

Yes, I used those words on purpose.

Well, as you say, "you're writing a new daemon".  This means that you
need to make sure that *if* this daemon ever forks/execs it leaves the
environment in a sane state which does not open up security holes.

Well, *I* know that.  But this is far from well-documented.

quoted

Does this sound rare?  It is!  But it's also hellish to debug when it
happens, precisely because of its rarity.  For example, in one case, I
had this problem because an sfdisk process started by my custom

Thing is, by making git (and some other programs) hide this problem
from you, this problem will get even *harder* to debug.  Whereas as a
daemon author you should be thankful that something breaks and shows you
your daemon needs fixing.

True enough, unless it was worked around in libc or the kernel as I
suggested.  That said, if git opens a file and writes random log
messages to it, I'd still consider that to be git's fault for doing
so.

I'm just feeling protective of the future sanity of other developers
here, hoping they don't have to go through what I did on a multi-week
bug hunt.  (We were even blaming reiserfs for a while for our boot
sector getting zapped...)  The fact that someone *other* than me has
suggested this change implies that I'm not the only one who has seen
such insanity in the wild.

It'd be fine if git simply died if fd 0, 1, or 2 isn't open when it
starts.  Printing a warning message wouldn't work, for hopefully
obvious reasons.  But it would be a shame to simply ignore this sort
of problem now that it's been brought up.

Have fun,

Avery

On Wed, Aug 27, 2008 at 02:22:39PM -0400, Avery Pennarun wrote:

I'm just feeling protective of the future sanity of other developers
here, hoping they don't have to go through what I did on a multi-week
bug hunt.  (We were even blaming reiserfs for a while for our boot
sector getting zapped...)  The fact that someone *other* than me has
suggested this change implies that I'm not the only one who has seen
such insanity in the wild.

You're not alone. I've been having trouble with a combination of
fetchmail, procmail and ssmtp, in which situation the ssmtp program
_somehow_ sometimes opens /dev/urandom as file descriptor 0 (while
calculating an SSL key?) and leaves it open, then reads the message
body from that file descriptor, resulting in an endless garbage message
being sent to the SMTP server.

I suspect the error originates in Debian's patch to ssmtp (which
added the SSL support) but I haven't been able to reproduce the bug
in controlled circumstances. It's possible that fetchmail or procmail
is doing something stupid - but a little more defensive programming
in ssmtp could avoid the total disaster area of sending an endless
binary stream to an SMTP server.

So although I'm not experiencing any problems with git due to incorrect
file descriptor usage, I'm sensitive to the general issue.

Nick.

Nick Andrew wrote:

On Wed, Aug 27, 2008 at 02:22:39PM -0400, Avery Pennarun wrote:

quoted

I'm just feeling protective of the future sanity of other developers
here, hoping they don't have to go through what I did on a multi-week

You're not alone. I've been having trouble with a combination of
fetchmail, procmail and ssmtp, in which situation the ssmtp program
_somehow_ sometimes opens /dev/urandom as file descriptor 0 (while

in controlled circumstances. It's possible that fetchmail or procmail
is doing something stupid - but a little more defensive programming
in ssmtp could avoid the total disaster area of sending an endless
binary stream to an SMTP server.

Procmail I can vouch for, it basically assumes your OS is broken and
fights it's way back to sanity (it can be setuid root, so it has to
be rather careful).
Nonetheless, I still maintain that hiding problems doesn't help, it
only makes the bugs even rarer and more difficult to find.

The filedescriptor problem is a programmer-error, not a user-error,
which is why not hiding it should be preferred.  If it were a
user-error, thing would be different, assisting the user is a Good
Thing.
-- 
Sincerely,
           Stephen R. van den Berg.

"Listen carefully, I shall say this only wence."