889116 - RNG_SystemRNG and RNG_FileUpdate should not fall back on rng_systemFromNoise on platforms with /dev/urandom

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

This bug continues the work of bug 882829.

RNG_SystemRNG should not fall back on rng_systemFromNoise on
platforms with a system entropy source. Most POSIX systems have
/dev/urandom or an equivalent.

By default, RNG_SystemRNG should fail (returning 0) rather than
falling back on rng_systemFromNoise if it cannot use the system
entropy source.

Comment 1

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 2

[Franziskus Kiefer [:franziskus]]

Attachment: random.patch

Do we still care about os2? If not, we can remove code here. But we should definitely remove the fallback here. I'm not sure if we still support systems where this is called and urandom is not present. If yes, we'll have to add a safeguard here instead checking for those and only fail if we're certain we have urandom.

Comment 3

[Franziskus Kiefer [:franziskus]]

Attachment: random.patch

We should also remove unused functions.

Comment 4

[Martin Thomson [:mt:]]

I think that I would prefer a much more severe cut here.

We have the following functions:
  RNG_RNGInit: could be a noop
  RNG_RandomUpdate: could be a noop
  RNG_GenerateGlobalRandomBytes: can draw on /dev/urandom
  RNG_SystemRNG: ditto
  RNG_RNGShutdown: could be a noop
  RNG_SystemInfoForRNG: could be a noop

The same is true for windows, where a call to CryptGenRandom should do the trick.

The OS/2 code can rot if we need to continue to support that platform.  Of course, we should remove it if we no longer need it; I would prefer that :)

Comment 5

[Martin Thomson [:mt:]]

Oh, and if we can remote the OS/2 code, then we could then remove the noop functions entirely.

Comment 6

[Franziskus Kiefer [:franziskus]]

That would be nice, but I think we can't do that. AFAIK fips requires two separate sources of randomness. We need input here from someone who knows fips before we can do bigger changes.

Comment 7

[J.C. Jones [:jcj] (he/him)]

To my knowledge, FIPS does not mandate two sources for the DRBG. Careful re-reading of the Implementation Guidelines, section 7.14
Entropy Caveats, doesn't mandate it either, though it's an option:

http://csrc.nist.gov/groups/STM/cmvp/documents/fips140-2/FIPS1402IG.pdf

That said, the implementation guidance, while nominally normative, is not always accurate with respect to what is actually under test. If there's doubt, the correct thing to do is to ask the validation lab for guidance.

For what it's worth though, my last validation did NOT include multiple entropy inputs, so I think it's safe to proceed as you've planned. But if there's doubt, we should contact a lab.

Comment 8

[Robert Relyea]

>I think that I would prefer a much more severe cut here.

I totally disagree here. We should not be sucking all the entropy out of /dev/urandom, nor should be be soly dependent on it for our cryptographically secure drbg. RNG is too important to leave completely outside ourselves.

Most of the mixing we do beside /dev/urandom is meant to deal with potentical issues like /dev/urandom fails catastrophically (which happens sometimes in things like containers).

I would prefer to have at least some fallback in that case.

I also don't care about the OS/2 code, but I don't think we can remove the drbg ourself (nor should we).

As for removing the seeding fallback, I'm of two minds. 1) I prefer that things work in the broadest environment, but 2) if we think the jitter code will also fail in that environment (particularly if it fails 'unsafe'). then it's better to fail altogether than to keep running with a vulnerable system.

bob

Comment 9

[Ryan Sleevi]

> We should not be sucking all the entropy out of /dev/urandom

If an application can "suck all the entropy out", that represents a fundamental design flaw of the OS that the userland should not (nor can it) work around.

> potentical issues like /dev/urandom fails catastrophically (which happens sometimes in things like containers)

Again, if the OS cannot provide a reasonable source of randomness, userland cannot account for this.


Stepping back, it seems like your threat model is "What happens if the OS cannot provide entropy? What should userland do?", and the answer, shown from the past two decades of trying to answer that, should and must be "nothing".

If we accept that threat model into the scope of NSS, a userland application, then it means we must take all appropriate steps to prevent things like forking (which has bitten OpenSSL), the need to persist the entropy value in the case of spontaneous power outage (which means file writing, which doesn't work in many sandboxes), etc.

A better solution than "Try to fall back with bad random" is to fail fast, and loudly, so that we never continue on with bad entropy. If the environment cannot provide good random, then it should not be an environment NSS can or should run on.

I have real trouble with the suggestion of exhaustion or spontaneous failure being threat models that NSS can accept.

Comment 10

[Wan-Teh Chang]

> We should not be sucking all the entropy out of /dev/urandom

I think Bob meant /dev/urandom should not be the only source of
the entropy used to see the NSS PRNG.

Comment 11

[Martin Thomson [:mt:]]

I'm not convinced by the containers argument.  If /dev/urandom isn't present, I would prefer to shutdown and do nothing.  On systems where /dev/urandom is present, but not good enough, I doubt that we'll find any way to scrape together enough entropy from within the operating system (especially a container).

If you have evidence that there are operating systems on which we have to support and but they don't have /dev/urandom (or equivalent), I'd be very concerned for the people running those machines.

I'm just going to throw these two links in (I find the latter convincing on this point):
http://www.2uo.de/myths-about-urandom/
http://blog.cr.yp.to/20140205-entropy.html

> I also don't care about the OS/2 code, but I don't think we can remove the drbg ourself (nor should we).

I didn't propose removing the drbg code, just to simplify the code that feeds it.

Comment 12

[Franziskus Kiefer [:franziskus]]

Attachment: prng-remove-fallback.patch

As discussed we should remove the fallback in RNG_SystemRNG. The patch also removes calls to RNG_GetNoise in RNG_SystemInfoForRNG as it doesn't provide any entropy.

Comment 13

[Martin Thomson [:mt:]]

Got me a phabricator link?

Comment 14

[Robert Relyea]

Franziskus, let's just go with the fallback that we agreed about. I'm pretty sure I don't want to get rid of the GetNoise yet.

bob

Comment 15

[Robert Relyea]

Comment on attachment 8808526 [details] [diff] [review]
prng-remove-fallback.patch

Review of attachment 8808526 [details] [diff] [review]:
-----------------------------------------------------------------

So the r- is only for the total removal of the Get_Noise(). I'm ok with most of the high res clocks going away (just the intel hw clock is all we really need). I'm OK with keeping the OS/2 code if we have to.

::: lib/freebl/sysrand.c
@@ +21,5 @@
>  #ifdef XP_OS2
>  #include "os2_rand.c"
>  #endif
>  
> +#ifdef XP_OS2

OK, I guess I was expecting this to go away altogether. Do we have a big call to compile for OS/2?


So I think this whole file could go away unless someone objects to dropping OS/2 support. (I just want to make it clear that *I* don't object to dropping OS/2 support, especially since we don't have much confidence in the seeding).

::: lib/freebl/unix_rand.c
@@ -660,5 @@
>      if (rv > 0) {
>          RNG_RandomUpdate(buf, rv);
>      }
>  }
>  #endif /* nec_ews */

I'm OK with getting rid of the high resolution clock for most platforms, but I'd still like to keep the intel clock around.

@@ -680,5 @@
> -    c = CopyLowBits((char *)buf + n, maxbytes, &tv.tv_sec, sizeof(tv.tv_sec));
> -    n += c;
> -    return n;
> -}
> -

I'd also like to keep this. Yes, it's only adding a couple of bits. We just need to make sure we don't depend on it.

@@ -1034,5 @@
> -            ;
> -        fclose(file);
> -    }
> -}
> -

yes, go ahead and get rid of this one.

@@ -1136,5 @@
> -    } else {
> -        fileToRead++;
> -    }
> -}
> -

Yes, these as well. (I initially thought there were windows versions of these as well, but they are already gone).

Comment 16

[Franziskus Kiefer [:franziskus]]

Attachment: prng-remove-fallback.patch

Ok, lets only remove the fallback and os2 for now.

Comment 17

[Robert Relyea]

Comment on attachment 8809755 [details] [diff] [review]
prng-remove-fallback.patch

Review of attachment 8809755 [details] [diff] [review]:
-----------------------------------------------------------------

r+ rrelyea

Comment 18

[Robert Relyea]

I think the r+ answers the need info:).

bob

Comment 19

[Franziskus Kiefer [:franziskus]]

landed as https://hg.mozilla.org/projects/nss/rev/5ed1226cf4186738bca980d10d8c21af199ecfd9

I close this one and file follow ups for the other issues.

Comment 20

[Kai Engert (:KaiE:)]

FYI, on Linux it's possible to create "chroot" environments. This can be used to simulate environments with different operating system files, and those environments might not necessarily have /dev/urandom available. The change from this bug to require /dev/urandom breaks those environments.

Comment 21

[Kai Engert (:KaiE:)]

It can be difficult to identify that a missing /dev/urandom is the cause for a failing NSS init.

Could NSS be changed to print a message about missing /dev/urandom, if NSS refuses to init without it?

Comment 22

[Ryan Sleevi]

(In reply to Kai Engert (:kaie) from comment #20)
> FYI, on Linux it's possible to create "chroot" environments. This can be
> used to simulate environments with different operating system files, and
> those environments might not necessarily have /dev/urandom available. The
> change from this bug to require /dev/urandom breaks those environments.

That's correct and intentional. A container that doesn't allow quality entropy through is a priori broken, and NSS failing to initializae is ensuring things fail safe and securely.

Comment 23

[Martin Thomson [:mt:]]

I would be OK with writing to stderr in this case, but for the reasons Ryan notes, not OK with just soldiering on through what is a catastrophic failure (insert Admiral Ackbar image).  Though I note that we could probably just make sure that the documentation for SEC_ERROR_NEED_RANDOM is sufficiently clear on this point.