Closed Bug 889116 Opened 7 years ago Closed 3 years ago

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

Categories

(NSS :: Libraries, defect, P2)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Target Milestone: 3.15.2 → 3.15.3
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
Summary: RNG_SystemRNG should not fall back on rng_systemFromNoise on platforms with /dev/urandom → RNG_SystemRNG and RNG_FileUpdate should not fall back on rng_systemFromNoise on platforms with /dev/urandom
Target Milestone: 3.15.4 → ---
See Also: → 1258669
See Also: → 1254334
See Also: → 1287231
Attached patch random.patch (obsolete) — Splinter Review
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.
Attachment #8797380 - Flags: review?(wtc)
Attached patch random.patch (obsolete) — Splinter Review
We should also remove unused functions.
Attachment #8797380 - Attachment is obsolete: true
Attachment #8797380 - Flags: review?(wtc)
Attachment #8797388 - Flags: review?(wtc)
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 :)
Oh, and if we can remote the OS/2 code, then we could then remove the noop functions entirely.
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.
Flags: needinfo?(rrelyea)
Flags: needinfo?(jjones)
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.
Flags: needinfo?(jjones)
>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
Flags: needinfo?(rrelyea)
> 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.
> 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.
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.
Attached patch prng-remove-fallback.patch (obsolete) — Splinter Review
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.
Attachment #8797388 - Attachment is obsolete: true
Attachment #8797388 - Flags: review?(wtc)
Attachment #8808526 - Flags: review?(wtc)
Attachment #8808526 - Flags: review?(rrelyea)
Attachment #8808526 - Flags: review?(martin.thomson)
Got me a phabricator link?
Flags: needinfo?(franziskuskiefer)
Flags: needinfo?(franziskuskiefer)
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 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).
Attachment #8808526 - Flags: review?(rrelyea) → review-
Ok, lets only remove the fallback and os2 for now.
Attachment #8808526 - Attachment is obsolete: true
Attachment #8808526 - Flags: review?(wtc)
Attachment #8808526 - Flags: review?(martin.thomson)
Attachment #8809755 - Flags: review?(rrelyea)
Attachment #8809755 - Flags: review?(martin.thomson)
Flags: needinfo?(rrelyea)
Attachment #8809755 - Flags: review?(martin.thomson) → review+
Comment on attachment 8809755 [details] [diff] [review]
prng-remove-fallback.patch

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

r+ rrelyea
Attachment #8809755 - Flags: review?(rrelyea) → review+
I think the r+ answers the need info:).

bob
Flags: needinfo?(rrelyea)
landed as https://hg.mozilla.org/projects/nss/rev/5ed1226cf4186738bca980d10d8c21af199ecfd9

I close this one and file follow ups for the other issues.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.29
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.
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?
(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.
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.
See Also: → 1346735
You need to log in before you can comment on or make changes to this bug.