1500115 - Revisit MFBT RandomUint64 logic

Status: RESOLVED WONTFIX

(Core :: MFBT, enhancement)

Description

[Chris Martin [:cmartin]]

Logic was moved from js/src/JSMath.cpp to mfbt/RandomNum.cpp to generate a secure random number before NSS has been initialized.

There were some concerns that the current logic wasn't secure enough, but after discussion with :Alex_Gaynor and :tjr, they agreed that it *is secure*, but perhaps it's not ideal.

This ticket is just a reminder that perhaps this topic can be revisited at a later date, where we can try to decide what the best mechanisms available are for each operating system.

Some of the concerns that were brought up or found online and could be discussed are:

- arc4random() is a user-space generator, and some have concerns that those are broken-by-design

- /dev/urandom is good for CSPRNG (especially on BSDs, has a few minor issues on Linux), but it has the problem that it may not be available if locked in chroot jail, or if the number of system FDs are exhausted (which apparently has happened on Android)

- Linux has getrandom() and getentropy() syscalls that should be preferred if available. Perhaps BSD will have some equivalent?

- Or is all of this getting too complex and maybe we want to boil the function down to a couple of basic codepaths?

- :tjr had also floated the idea of *not* opening /dev/urandom every time we need it, but instead opening it once at initialization and re-using the handle throughout

Comment 1

[Gian-Carlo Pascutto [:gcp]]

IIRC the original code layout would also prefer arc4random even if getrandom is available.

Comment 2

[Chris Martin [:cmartin]]

Link to previous bug that spawned this one:

https://bugzilla.mozilla.org/show_bug.cgi?id=1402282

(In reply to Gian-Carlo Pascutto [:gcp] from comment #1)
> IIRC the original code layout would also prefer arc4random even if getrandom
> is available.

I believe that's the case for Android, Darwin, DragonFly, FreeBSD, NetBSD, and OpenBSD - They will use arc4random() even if getrandom() or getentropy() would have been available.


I didn't change that behavior, so the new code also does this.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Chris Martin [:rustafarian] from comment #0)
> - :tjr had also floated the idea of *not* opening /dev/urandom every time we
> need it, but instead opening it once at initialization and re-using the
> handle throughout

This would help on Android, where our tests would intermittently fail to open /dev/urandom because the system ran out of fds when running tests in parallel. So we switched to prefer arc4random() on Android in bug 1140806.

(In reply to Chris Martin [:rustafarian] from comment #2)
> I believe that's the case for Android, Darwin, DragonFly, FreeBSD, NetBSD,
> and OpenBSD - They will use arc4random() even if getrandom() or getentropy()
> would have been available.

We should probably prefer getrandom(GRND_NONBLOCK) over arc4random() when available. getrandom() without GRND_NONBLOCK and getentropy() can block if the kernel hasn't initialized its urandom entropy pool yet, same as when reading /dev/urandom. I don't know if this is a concern for us.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

The urandom entropy pool not having been initialized by the time you're running Firefox is inconceivable to me (modulo perhaps a FirefoxOS situation where Firefox is running at very early boot). Further, in practice this situation is basically limited to only the first boot, on most (all?) Linux distros on shutdown some entropy is read from /dev/urandom and stored in a file, and then /dev/urandom is re-seeded with that on boot, ensuring that the pool is ready to go very early.

Comment 5

[Chris Peterson [:cpeterson]]

Jan, the getentropy() and getrandom() functions were added in FreeBSD 12-current. If I want to call these functions in Firefox, which approach can I use?

1. #ifdef __FreeBSD__ // with no version check. This assumes Firefox >= 65 won't be compiled on FreeBSD < 12-current!
2. #if defined(__FreeBSD__) && __FreeBSD_version >= 1200000
3. Add an autoconf check like you did for pthread_get_name_np() in bug 1486281

https://www.freebsd.org/cgi/man.cgi?query=getentropy&manpath=FreeBSD+12-current

Comment 6

[Jan Beich]

(In reply to Chris Peterson [:cpeterson] from comment #5)
> 3. Add an autoconf check like you did for pthread_get_name_np() in bug 1486281

This one. FreeBSD 11.* is supported until 2021-09-30. __FreeBSD_version conditionals require <sys/param.h> or <osreldate.h> and maybe fragile if the code is moved. One can do defined(__FreeBSD__) && __FreeBSD__ < 12 if being precise doesn't matter such as when the support was added in X.0 release.

Landry, Petr, are you OK with autoconf check detecting getentropy() and getrandom() on Solaris or OpenBSD?
https://man.openbsd.org/getentropy.2
https://docs.oracle.com/cd/E88353_01/html/E37841/getentropy-2.html

François, Martin, do DragonFly or NetBSD plan to adopt getentropy() and getrandom()?

Comment 7

[Landry Breuil (:gaston)]

Well the manpage says:

     getentropy() is not intended for regular code; please use the
     arc4random(3) family of functions instead.

i dont know the details on why preferring one or the other but i'd rather follow what the manpage advises...

Comment 8

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #7)
>      getentropy() is not intended for regular code; please use the
>      arc4random(3) family of functions instead.

I wonder if it applies to FreeBSD as well.

Conrad, is it a good idea to replace arc4random() usage with getentropy() and getrandom() on FreeBSD?

Comment 9

[Martin Husemann]

NetBSD has no intention to introduce getentropy() nor getrandom().

We have a simple sysctl api to fetch entropy values (kern.urnd for single uint32_t values, kern.arnd for arbitrary byte arrays). For all other uses we recommend reading /dev/urandom.

Martin

Comment 10

[Conrad Meyer]

(In reply to Jan Beich from comment #8)
> (In reply to Landry Breuil (:gaston) from comment #7)
> >      getentropy() is not intended for regular code; please use the
> >      arc4random(3) family of functions instead.
> 
> I wonder if it applies to FreeBSD as well.
> 
> Conrad, is it a good idea to replace arc4random() usage with getentropy()
> and getrandom() on FreeBSD?

IMO, either is ok on FreeBSD 12+, although it's probably worth double-checking with secteam@.  But I would suggest using arc4random(3).  Some more detail below.

arc4random(3) in 12+ is implemented in terms of a Chacha20 keystream generated from a getentropy(3) seed.  It behaves correctly around fork, and it reseeds after some number of bytes.  It was taken more or less unmodified from OpenBSD.

Pros of getentropy/getrandom:
* Kernel implementation (re: "some have concerns that [user-space RNG] are broken-by-design" concern in description)
* Doesn't have a confusing name that suggests the implementation is the known-broken rc4 cipher

Pros of userspace arc4random(3):
* Does not require a syscall invocation on every request for random data
* arc4random(3)'s chacha20 keystream is almost certainly faster than the randomdev/getentropy/getrandom implementation (essentially, software AES-CTR mode), even ignoring syscall overhead
* Does not contend global randomdev lock with other processes as much

Similar to both:
* Threads in a single process attempting to get random bytes will contend on a mutex.  (arc4random(3) has a global pthread mutex, and the kernel has a global mutex for accessing randomdev/getentropy/getrandom)
* Fork-safe for fork-naive applications

Hope that helps.  Please let me know if I can answer any questions or provide more detail.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Conrad Meyer from comment #10)
> IMO, either is ok on FreeBSD 12+, although it's probably worth
> double-checking with secteam@.  But I would suggest using arc4random(3). 
> Some more detail below.
> 
> arc4random(3) in 12+ is implemented in terms of a Chacha20 keystream
> generated from a getentropy(3) seed.  It behaves correctly around fork, and
> it reseeds after some number of bytes.  It was taken more or less unmodified
> from OpenBSD.

Thanks, Conrad. Your analysis is very helpful!

For the record, here are my notes about the availability of getentropy() and getrandom() on different platforms:

Has getentropy():
  Android 4.1
  Darwin (since macOS 10.12, but not iOS yet?)
  Linux
  FreeBSD 12
  OpenBSD
  Solaris 11.3

Has getrandom():
  Android 4.1
  FreeBSD 12
  Linux
  Solaris 11.3

Has neither:
  DragonFly BSD
  NetBSD

https://github.com/crystal-lang/crystal/pull/6350 has some discussion about the Crystal programming language switching from /dev/urandom to arc4random().

https://bugs.python.org/issue25003 has discussion about Python switching from getentropy() to getrandom() to avoid performance problems (now fixed?) in Solaris 11.3.

Comment 12

[Conrad Meyer]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> Thanks, Conrad. Your analysis is very helpful!

Happy to help! 

> ...
>
> https://github.com/crystal-lang/crystal/pull/6350 has some discussion about
> the Crystal programming language switching from /dev/urandom to arc4random().
> 
> https://bugs.python.org/issue25003 has discussion about Python switching
> from getentropy() to getrandom() to avoid performance problems (now fixed?)
> in Solaris 11.3.

For what it's worth, I've taken a quick skim of both of these threads and am alarmed at the number of confused or flat out incorrect statements about random data, CSPRNGs, and terminology.  Please take them with a big grain of salt (or ignore them entirely).

Comment 13

[Chris Peterson [:cpeterson]]

So it sounds like arc4random() is the best option, where available. The current state of RandomUint64() is that it will use:

* arc4random() on Android, Darwin, DragonFly BSD, FreeBSD, NetBSD, and OpenBSD
* getrandom() on (non-Android) Linux
* /dev/urandom on other XP_UNIX platforms like Solaris
* RtlGenRandom() on Windows

Solaris supports arc4random(), so we could add Solaris (#ifdef XP_SOLARIS) to our arc4random() #ifdefs instead of removing arc4random(). I don't have a Solaris machine, so I'll let a Solaris maintainer decide what they would like to do. :)

Comment 14

[Landry Breuil (:gaston)]

I don't have much to add to what conrad said in comment #10 - but yes on OpenBSD arc4random() should definitely be used as it's in userspace and doesnt have the getentropy() syscall overhead (which is used by arc4random() under the hood to reseed when needed). the chacha stream (which is an implementation detail, in the end) is seeded by the kernel at early bootup, and you get high quality random data from arc4random(), which doesnt block.

Comment 15

[Petr Sumbera]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> https://bugs.python.org/issue25003 has discussion about Python switching
> from getentropy() to getrandom() to avoid performance problems (now fixed?)
> in Solaris 11.3.

I can confirm that the bug is now resolved in all relevant (for Firefox) Solaris releases:
https://github.com/oracle/solaris-userland/blob/master/components/python/python35/patches/26-getrandom.patch

Comment 16

[Chris Martin [:cmartin]]

Closing this bug, as it sounds like everyone is pretty happy with the current implementation, and there hasn't really been any movement in a couple of years now.