Re: [PATCH] be paranoid about closed stdin/stdout/stderr

From: Stephen R. van den Berg <srb@cuci.nl>
Date: Aug 27, 2008, 09:18 UTC
Message-ID: <20080827091800.GB484@cuci.nl>
In-Reply-To: <32541b130808262201v4d7c1aa5r781720a80b79fcd0@mail.gmail.com>

Avery Pennarun wrote:

>On 8/26/08, Stephen R. van den Berg <srb@cuci.nl> 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.

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.

Show 5 quoted lines

>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