174993 - NetBSD complains set{u,g}id pid # (netstat) was invoked by uid ... (mozilla-bin) with fd 0 closed

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[waitzman]

User-Agent:       Mozilla/5.0 (X11; U; NetBSD i386; en-US; rv:1.1) Gecko/20021017
Build Identifier: Mozilla/5.0 (X11; U; NetBSD i386; en-US; rv:1.1) Gecko/20021017

The NetBSD kernel has a paranoid check, with a syslog output, in
sys/kern/kern_descrip.c for standard fds being closed when invoking a setuid or
setgui program.  Mozilla's mozilla/security/nss/lib/freebl/unix_rand.c invokes
netstat with fd 0 closed occassionally when trying to get more randomness.

Here is a patch (hard to test, as the syslog message showing up is the bug and
it is hard to trigger it):

Index: unix_rand.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/unix_rand.c,v
retrieving revision 1.8.28.1
diff -c -r1.8.28.1 unix_rand.c
*** unix_rand.c 18 May 2002 00:57:05 -0000      1.8.28.1
--- unix_rand.c 17 Oct 2002 14:03:14 -0000
***************
*** 80,85 ****
--- 80,88 ----
  #if defined(SCO) || defined(UNIXWARE) || defined(BSDI) || defined(FREEBSD) \
      || defined(NETBSD) || defined(NTO) || defined(DARWIN)
  #include <sys/times.h>
+ #if defined(NETBSD)
+ #include <fcntl.h>
+ #endif /* NETBSD */
  
  #define getdtablesize() sysconf(_SC_OPEN_MAX)
  
***************
*** 692,697 ****
--- 695,713 ----
        if (p[1] != 1) dup2(p[1], 1);
        if (p[1] != 2) dup2(p[1], 2);
        close(0);
+ #if defined(NETBSD)
+         {
+         /* Stop kernel syslogs of the form:
+          * "set{u,g}id pid # (netstat) was invoked by uid ... 
+          * (mozilla-bin) with fd 0 closed"
+          */
+         int pstdin = open("/dev/null", O_RDONLY);
+         if (pstdin != 0) {
+           dup2(pstdin, 0);
+           close(pstdin);
+         }
+       }
+ #endif /* NETBSD */
        for (fd = getdtablesize(); --fd > 2; close(fd))
            ;
  


Reproducible: Sometimes

Steps to Reproduce:
1. guessing visiting ssl protected sites will trigger a need for randomness


Actual Results:  
nothing was logged -- which is the correct behavior

Expected Results:  
not trigger a kernel syslog

Comment 1

[Aaron Lehmann]

I just filed #182758 which is related to this bug because fixing it should fix
this bug as a side effect. My proposed solution is much more drastic (however,
IMHO, much more correct) than this patch.

Comment 2

[Mitchell Stoltz (not reading bugmail)]

->PSM. This appears to be a minor error caused by the entropy generator on
NetBSD. The reporter included a patch above.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

arg, let's try that again...

Comment 4

[John Unruh]

cc people for comments. Also see bug 182758. 

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

This nuisance warning sounds like an OS configuration problem, not a mozilla
problem.  But if the patch shown above suffices, I see no reason not to do it.

This bug is against code in NSS, so I'm changing it to be an NSS bug.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Hmm. That didn't change the bug owner.  Maybe this will.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Remove target milestone of 3.8, since these bugs didn't get into that release.

Comment 8

[Julien Pierre]

We shouldn't fork netstat anymore anyway.
Does the fix for 100447 mean that safe_popen won't be invoked anymore on NetBSD ?

Comment 9

[Kaspar]

The problem still occurs under NetBSD (I just tested with a checkout of trunk), I continue to see kernel messages like:

> set{u,g}id pid 3859 (netstat) was invoked by uid 1000 ppid 26876 (certutil)
> with fd 0 closed

The fix from bug 100447 only applies to BSD/OS, and AFAICT, netstat is still forked on most [UNIX] platforms (except Solaris and VMS), cf.

http://lxr.mozilla.org/mozilla/source/security/nss/lib/freebl/unix_rand.c#881:

881 /* Fork netstat to collect its output by default. Do not unset this unless
882  * another source of entropy is available
883  */
884 #define DO_NETSTAT 1

and

http://lxr.mozilla.org/mozilla/source/security/nss/lib/freebl/unix_rand.c#1016:

1016 #ifdef DO_NETSTAT
1017     fp = safe_popen(netstat_ni_cmd);
1018     if (fp != NULL) {
1019         while ((bytes = fread(buf, 1, sizeof(buf), fp)) > 0)
1020             RNG_RandomUpdate(buf, bytes);
1021         safe_pclose(fp);
1022     }
1023 #endif

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Has anyone considered that maybe NetBSD should be fixed to stop whining?

Comment 11

[Kaspar]

(In reply to comment #10)
> Has anyone considered that maybe NetBSD should be fixed to stop whining?

Why not fix the root cause (instead of the symptom)? FWIW, I believe this "whining" is actually considered a feature of NetBSD... note that other BSDs have this check as well (but in contrast to NetBSD, they simply don't complain and just silently fix things):

FreeBSD, fdcheckstd() in
http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/kern/kern_descrip.c?rev=1.312;f=h;ln=1

> /*
>  * It is unsafe for set[ug]id processes to be started with file
>  * descriptors 0..2 closed, as these descriptors are given implicit
>  * significance in the Standard C library.  fdcheckstd() will create a
>  * descriptor referencing /dev/null for each of stdin, stdout, and
>  * stderr that is not already open.
>  */

OpenBSD, sys_execve() in
http://www.openbsd.org/cgi-bin/cvsweb/src/sys/kern/kern_exec.c?rev=1.102&content-type=text/x-cvsweb-markup

> /*
>  * Ensure that stdin, stdout, and stderr are already
>  * allocated.  We do not want userland to accidentally
>  * allocate descriptors in this range which has implied
>  * meaning to libc.
>  */

Comment 12

[Kaspar]

Attachment: proposed patch

I think I've found a solution for this issue which is less platform specific than the one provided in comment #0 - see the attached patch. Instead of unconditionally closing file descriptor 0, it tries to reassociate it with /dev/null, first (only if this fails, it will close it anyway - this reflects the current behavior). Fixes the problem under NetBSD, of course.

This should be ok for all UNIX-like systems, I guess, or did I miss something...?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

It DOES seem like this should be OK on all Unix systems. 

Comment 14

[Kaspar]

Comment on attachment 272381 [details] [diff] [review]
proposed patch

Julien, would you mind reviewing this patch? As Nelson says, it should be ok on all UNIX systems - quoting from the manpage of freopen():

     The freopen() function opens the file whose name is the string pointed to
     by path and associates the stream pointed to by stream with it.  The
     original stream (if it exists) is closed.  The mode argument is used just
     as in the fopen() function.  The primary use of the freopen() function is
     to change the file associated with a standard text stream (stderr, stdin,
     or stdout).

freopen() should be available on all OSes where fopen() is available, since it's specified in ANSI C (which RNG_FileUpdate() and RNG_SystemRNG() in unix_rand.c already use).

Comment 15

[Julien Pierre]

Kaspar, I know the fix for bug 100447 doesn't apply to NetBSD . But could NetBSD use the same fix ? Is there a /dev/urandom on NetBSD ?

Comment 16

[Kaspar]

(In reply to comment #15)
> Kaspar, I know the fix for bug 100447 doesn't apply to NetBSD . But could
> NetBSD use the same fix ? Is there a /dev/urandom on NetBSD ?

Yes, there is (it was added with release 1.3 in 1998).

So, would you prefer to solve it in a way like this? (didn't care to update the comment yet ;-)

Index: unix_rand.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/unix_rand.c,v
retrieving revision 1.24
diff -u -8 -r1.24 unix_rand.c
--- unix_rand.c 6 Jan 2007 01:45:56 -0000       1.24
+++ unix_rand.c 18 Jul 2007 04:55:44 -0000
@@ -971,17 +972,17 @@

 /*
  * Bug 100447: On BSD/OS 4.2 and 4.3, we have problem calling safe_popen
  * in a pthreads environment.  Therefore, we call safe_popen last and on
  * BSD/OS we do not call safe_popen when we succeeded in getting data
  * from /dev/urandom.
  */

-#ifdef BSDI
+#if defined(BSDI) || defined(NETBSD)
     if (bytes)
         return;
 #endif

 #ifdef SOLARIS

 /*
  * On Solaris, NSS may be initialized automatically from libldap in


I thought it would be preferrable to have as few OS-specific ifdefs as possible, but am ok with simply skipping safe_popen() on NetBSD, of course (should we really only exit early for BSD/OS and NetBSD, btw?).

Comment 17

[Julien Pierre]

Kaspar,

Platform ifdefs are OK in this file, there is just no way around them. That fix in comment 16 looks fine. We can make it in addition to your other one. I think we want to make sure the platforms really support /dev/urandom proper before we make them exit early, so that code needs to be conditional.

Comment 18

[Julien Pierre]

Comment on attachment 272381 [details] [diff] [review]
proposed patch

Please also attach the other patch to exit early on NetBSD with /dev/urandom .

Comment 19

[Kaspar]

Attachment: Skip safe_popen() calls on platforms with /dev/urandom

(In reply to comment #18)
> Please also attach the other patch to exit early on NetBSD with /dev/urandom .

Here it is... I think it makes sense to add a couple of additional platforms, too (/dev/urandom first appeared in Linux 1.3.30, and then made its way into the BSDs).

I'll attach another patch which has just the change for NetBSD, though - if you prefer to limit the fix to this particular OS, for the time being.

Comment 20

[Kaspar]

Attachment: NetBSD only version of the previous patch

(I would prefer the previous one, so not setting the review flag here.)

Comment 21

[Julien Pierre]

Comment on attachment 272917 [details] [diff] [review]
Skip safe_popen() calls on platforms with /dev/urandom

I'm fine with this patch. Since it affects Linux too, I'm asking Bob from RedHat for a superreview.

Comment 22

[Robert Relyea]

Comment on attachment 272917 [details] [diff] [review]
Skip safe_popen() calls on platforms with /dev/urandom

safe_popen has generated fun on linux systems as well.

I think the tip has a patch that works around them, but I like the idea of continuing on if we got enough entropy from /dev/urandom.

Most linux distro's turn off the NETSTAT call already.

bob

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

When /dev/urandom first came out, the implementation on numerous platforms
was flawed so badly that we deliberately chose NOT to have it be the only
source of entropy on those systems.  We chose to use /dev/urandom ONLY on
those systems where it was believed to have been well implemented.  

That's why the code does not presently shortcut the function on all platforms
that have /dev/urandom.  It was deliberate.

But that was long ago.  One would hope that, by now, all platforms that 
support /dev/urandom have good implementations of it.  Still, it would be 
nice to know that that is the case, rather than to merely assume it.

Also, imagine a system with no real /dev/urandom, where someone (as root)
creates a file of (say) all zeros (or any constant contents) with the name 
/dev/urandom.  It would be unfortunate if we used that content as our only
source of randomness.  So, I think it remains a good idea to only allow
/dev/urandom to shortcut the function on platforms that are KNOWN to have
good implementations.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272381 [details] [diff] [review]
proposed patch

This patch is correct, but it produces code that surely looks wrong.  
I think that future readers of this code will assume that the line with the open curly brace was supposed to immediately follow the if line.  
So, I'd like to request that the declaration of the int ndesc be moved up to just after the line wiht the enclosing switch statement, and that the curly braces be removed here and the lines in them unindented.

Comment 25

[Kaspar]

Attachment: proposed patch v2, making the code less confusing

(In reply to comment #24)
> So, I'd like to request that the declaration of the int ndesc be moved up to
> just after the line wiht the enclosing switch statement,

This won't work (unless I'm completely misunderstanding you)... code between switch(pid) and "case: -1" will never get executed.

In any case, I think there's a better solution to this - IINM, we can get rid of ndesc altogether, as proposed by the attached patch (does this need another round of reviews? I didn't obsolete the other one for the time being).

Comment 26

[Kaspar]

Comment on attachment 272918 [details] [diff] [review]
NetBSD only version of the previous patch

This one is definitely obsolete, however.

Comment 27

[Kaspar]

(In reply to comment #25)
> (In reply to comment #24)
> > So, I'd like to request that the declaration of the int ndesc be moved up

Oh, now I'm understanding... you just meant to *declare* ndesc after the switch statement - this would work, of course (my bad).

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 273218 [details] [diff] [review]
proposed patch v2, making the code less confusing

PR_MIN is a macro that evaluates each of its arguments twice, so you don't want to put a function call as an argument to it.  
Yes, as I wrote before, move the declaration of ndesc to the beginning of the enclosing switch.
Just the declaration.  
Leave the assignment where it is.

Comment 29

[Kaspar]

Attachment: patch to safe_popen(), (checked in)

(In reply to comment #28)
> PR_MIN is a macro that evaluates each of its arguments twice, so you don't
> want to put a function call as an argument to it.

Right, I should have read bug 266206 before proposing such a change :-(

So, let's try again - hopefully I got it right this time:

- changed what Nelson requested in comment #24

- obsoleted previous versions of the patch (but only those changing
  safe_popen() itself, the patch in attachment 272917 [details] [diff] [review] continues to be valid)

- setting the same review flags as for the previous version (sorry for the
  additional bugspam ... thanks for bearing with me)

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 273297 [details] [diff] [review]
patch to safe_popen(), (checked in)

r=nelson

Comment 31

[Julien Pierre]

Comment on attachment 273297 [details] [diff] [review]
patch to safe_popen(), (checked in)

I wish we could use closefrom on Solaris, but that's an S9+ feature and we still support S8.

Comment 32

[Kaspar]

(In reply to comment #31)
> I wish we could use closefrom on Solaris, but that's an S9+ feature and we
> still support S8.

Oh, I just realized that as of release 3.0, even NetBSD would have closefrom()... too bad we're now returning early and skipping safe_popen() ;-)

Anyway, can somebody check in attachment 272917 [details] [diff] [review] and attachment 273297 [details] [diff] [review]? Both have two reviews by now, so should be ok for checkin to trunk and branch, I guess. Thanks!

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 273297 [details] [diff] [review]
patch to safe_popen(), (checked in)

Bug 174993 - freopen stdin to /dev/null in safe_popen.
Patch by Kaspar Brand <[e-mail]>, r=julien,nelson

On trunk:
Checking in unix_rand.c; new revision: 1.25; previous revision: 1.24

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 273297 [details] [diff] [review]
patch to safe_popen(), (checked in)

On branch:
unix_rand.c; new revision: 1.17.10.7; previous revision: 1.17.10.6

Comment 35

[Kai Engert (:KaiE:)]

Comment on attachment 272917 [details] [diff] [review]
Skip safe_popen() calls on platforms with /dev/urandom

Can this get checked in?

Comment 36

[Wan-Teh Chang]

Comment on attachment 272917 [details] [diff] [review]
Skip safe_popen() calls on platforms with /dev/urandom

Just check in the platforms you're responsible for.

Comment 37

[Kai Engert (:KaiE:)]

Wan-Teh, why is it a problem to check in attachment 272917 [details] [diff] [review] to trunk as is?
It got two reviews.

If you want me to land a subset of the patch, I'll need to a new patch, new comment, and request for review again. I can do that of course.

Comment 38

[Kai Engert (:KaiE:)]

Attachment: linux subset of attachment 272917 [checked in]

We should either check in attachment 272917 [details] [diff] [review], or this one.
Asking Wan-Teh for a decision and potentially review this one.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 291248 [details] [diff] [review]
linux subset of attachment 272917 [details] [diff] [review] [checked in]

r=nelson

Comment 40

[Kai Engert (:KaiE:)]

Comment on attachment 291248 [details] [diff] [review]
linux subset of attachment 272917 [details] [diff] [review] [checked in]

This patch checked in.

/cvsroot/mozilla/security/nss/lib/freebl/unix_rand.c,v  <--  unix_rand.c
new revision: 1.28; previous revision: 1.27

Comment 41

[Wan-Teh Chang]

Comment on attachment 291248 [details] [diff] [review]
linux subset of attachment 272917 [details] [diff] [review] [checked in]

r=wtc.

Comment 42

[Nelson Bolyard (seldom reads bugmail)]

Kai, can this bug be marked resolved/fixed now?

Comment 43

[Kai Engert (:KaiE:)]

Nelson, I don't know.
I checked in a fix for Linux, but this bug complain about NetBSD. I don't know what's the status on netBSD is.

Comment 44

[Nelson Bolyard (seldom reads bugmail)]

I'm marking this bug resolved/fixed.
If someone thinks that it is not fixed, they can reopen it and tell us why.