Closed Bug 1723674 (crypto-randomUUID) Opened 3 years ago Closed 3 years ago

Implement crypto.randomUUID()

Categories

(Core :: DOM: Web Crypto, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox-esr91 --- wontfix
firefox90 --- wontfix
firefox91 --- wontfix
firefox92 --- wontfix
firefox93 --- wontfix
firefox94 --- wontfix
firefox95 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files)

WICG draft specification for crypto.randomUUID():
https://wicg.github.io/uuid/

I have a patch to implement crypto.randomUUID() and fix the WPT failures.

Alias: crypto-randomUUID
Component: DOM: Core & HTML → DOM: Web Crypto

I know of at least two ways to generate UUIDs in Gecko:

It might be nice to unify them. The easiest would probably be to pipe nsUUIDGenerator into the Rust crate, assuming we think it's of sufficient quality.

So I don't want to make a big deal of this, but I am not happy with either of those implementations. nsUUIDGenerator isn't guaranteed to produce cryptographic random numbers (it invokes random() in some cases). It's probably not at the level of a security problem, as we mostly use arc4random(), but it's not something I would trust.

I traced the rust version back to the uuid crate, which just uses thread_rng() from the rand crate. (BTW, that uuid crate is overkill for our needs, but it's probably deeply entrenched already, so no point trying to fix that, especially given how small it is.) I don't know if we override the default thread PRNG. It looks like we just use the default from the rand crate, which looks to use os-specific functions. We should be using the same CSPRNG we have for crypto.getRandomBytes. That is, the one in NSS that we know is good.

(In reply to Martin Thomson [:mt:] from comment #3)

I traced the rust version back to the uuid crate, which just uses thread_rng() from the rand crate. (BTW, that uuid crate is overkill for our needs, but it's probably deeply entrenched already, so no point trying to fix that, especially given how small it is.) I don't know if we override the default thread PRNG. It looks like we just use the default from the rand crate, which looks to use os-specific functions. We should be using the same CSPRNG we have for crypto.getRandomBytes. That is, the one in NSS that we know is good.

In that case, I will look into implementing randomUUID using the same CSPRNG we use for getRandomBytes (nsRandomGenerator::GetRandomBytes). I will probably update nsUUIDGenerator itself to use nsRandomGenerator and then implement randomUUID using nsUUIDGenerator.

(In reply to Chris Peterson [:cpeterson] from comment #4)

(In reply to Martin Thomson [:mt:] from comment #3)

I traced the rust version back to the uuid crate, which just uses thread_rng() from the rand crate. (BTW, that uuid crate is overkill for our needs, but it's probably deeply entrenched already, so no point trying to fix that, especially given how small it is.) I don't know if we override the default thread PRNG. It looks like we just use the default from the rand crate, which looks to use os-specific functions. We should be using the same CSPRNG we have for crypto.getRandomBytes. That is, the one in NSS that we know is good.

In that case, I will look into implementing randomUUID using the same CSPRNG we use for getRandomBytes (nsRandomGenerator::GetRandomBytes). I will probably update nsUUIDGenerator itself to use nsRandomGenerator and then implement randomUUID using nsUUIDGenerator.

And once we do that, probably worth also eliminating GkRustUtils_GenerateUUID and rerouting its one caller to the new path.

Blocks: 801950

(In reply to Bobby Holley (:bholley) from comment #5)

And once we do that, probably worth also eliminating GkRustUtils_GenerateUUID and rerouting its one caller to the new path.

I filed bug 1724152 to replace GkRustUtils_GenerateUUID.

(In reply to Chris Peterson [:cpeterson] from comment #4)

In that case, I will look into implementing randomUUID using the same CSPRNG we use for getRandomBytes (nsRandomGenerator::GetRandomBytes).

This new patch is taking longer than expected. Firefox generates UUIDs before NSS is initialized. Instead of returning an error, NSS will quietly self-initialize in "nocertdb" mode because the user's profile directory path is not known yet so early in Firefox startup. In "nocertdb" mode, tests can't insert test certs into the certdb necessary to establish HTTPS connections to local test servers.

I need to find a workaround, such as finding the user's profile directory path sooner during startup, using NSS without the user's profile directory path, or special casing UUID generation during early startup to fall back to a different RNG. These approaches will surely have zero unanticipated side effects. :)

I think this issue might be a duplicate of bug 1700675, which has patches for migrating nsUUIDGenerator to use RandomUint64OrDie() (a CSPRNG which doesn't require NSS), and removing the use of GkRustUtils for generating UUIDs.

Hi Dana, in this bug I am implementing a new crypto.randomUUID() Web API that Chrome has shipped. In comment 3, Martin recommended that UUID generation (nsUUIDGenerator) use NSS's CSRNG (via nsRandomGenerator like crypto.getRandomValues() does). nsUUIDGenerator currently uses the OS's UUID APIs on Windows and macOS, but falls back to the standard C library's weak random() on Linux.

Unfortunately, changing nsUUIDGenerator to use nsRandomGenerator is not straightforward because nsUUIDGenerator is called very early during Firefox startup, before NSS is initialized. Calling nsRandomGenerator during early startup does return random data, but it causes NSS to quietly auto-initialize in "nocertdb" mode which causes hard to debug test failures.

Do you like any of these alternatives?

  1. Change nsUUIDGenerator to use the OS's strong RNG (e.g. /dev/urandom, arc4random(), etc via MFBT's RandomNum code). This is similar to the patches in bug 1700675 (r+'d but never landed) to work around a different problem.
  2. Change nsUUIDGenerator to use the OS's RNG during early startup and nsRandomGenerator after NSS has been initialized.
  3. Change nsUUIDGenerator to use nsRandomGenerator, but change nsRandomGenerator to use the OS's RNG during early startup and NSS after it has been initialized.
  4. None of the above: use some other option or don't implement crypt.randomUUID() at all.

I like #3 because it would make nsRandomGenerator available during early startup, but still use NSS when it's available. You might not like #3 if you intend nsRandomGenerator to strictly be a wrapper around NSS's CSRNG.

Flags: needinfo?(dkeeler)

I would actually prefer #2 here - nsRandomGenerator should really be guaranteed to be using NSS's CSPRNG. My understanding of a quick read of the randomUUID spec is that using /dev/urandom would be acceptable, so it should be okay to have that fallback.

Flags: needinfo?(dkeeler)

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #10)

I would actually prefer #2 here - nsRandomGenerator should really be guaranteed to be using NSS's CSPRNG. My understanding of a quick read of the randomUUID spec is that using /dev/urandom would be acceptable, so it should be okay to have that fallback.

Thanks. I'm relieved you preferred option #2 because I found some cases where nsRandomGenerator's main thread initialization requirement would add new complications to nsUUIDGenerator. Option #2 avoids that problem.

I have patches that I will clean up and post soon.

(In reply to Dana, Chris)

My understanding of a quick read of the randomUUID spec is that using /dev/urandom would be acceptable, so it should be okay to have that fallback.

My expectation in the UUID spec, was that a source of randomness similar to crypto.getRandomBytes would be used. The Web Crypto spec indicates for crypto.getRandomBytes:

Implementations should generate cryptographically random values using well-established cryptographic pseudo-random number generators seeded with high-quality entropy, such as from an operating-system entropy source (e.g., "/dev/urandom"). This specification provides no lower-bound on the information theoretic entropy present in cryptographically random values, but implementations should make a best effort to provide as much entropy as practicable.

So /dev/urandom should be good 👍

I'm excited to see this work in motion 🎉

Generalize RandomUint64() into a new GenerateRandomBytes() function, which will be used in the next changeset to replace nsUUIDGenerator's various platform-specific implementations with one simple implementation.

This changeset is an adaptation of the accepted changes (but not landed) for bug 1700675:

https://phabricator.services.mozilla.com/D118714

Also, clean up some old (1998!) JavaDoc comments that just make the code more difficult to read.

Depends on D124311

Google shipped crypto.randomUUID() in Chrome 92.

WICG draft specification for crypto.randomUUID():
https://wicg.github.io/uuid/

Depends on D124312

Attachment #9239104 - Attachment description: Bug 1723674 - mfbt: Add new GenerateRandomBytes() function. r?glandium,cmartin → Bug 1723674 - mfbt: Add new GenerateRandomBytes() function. r?cmartin
Attachment #9239105 - Attachment description: Bug 1723674 - Move nsUUIDGenerator logic to new nsID::GenerateUUID() factory functions that use nsRandomGenerator. r?ckerschb,nika,tjr,ngogge → Bug 1723674 - Move nsUUIDGenerator logic to new nsID::GenerateUUID() factory functions that use nsRandomGenerator. r?nika,keeler!
Attachment #9239106 - Attachment description: Bug 1723674 - Implement crypto.randomUUID(). r?keeler → Bug 1723674 - Implement crypto.randomUUID(). r?keeler,nika!
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eaa49cc4154b
mfbt: Add new GenerateRandomBytes() function. r=cmartin
https://hg.mozilla.org/integration/autoland/rev/a1b5d8bd40fc
Move nsUUIDGenerator logic to new nsID::GenerateUUID() factory functions that use nsRandomGenerator. r=keeler
https://hg.mozilla.org/integration/autoland/rev/ad6b007fd7af
Implement crypto.randomUUID(). r=keeler,nika
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

FYI, docs tracking for FF95 work on this method can be found here: https://github.com/mdn/content/issues/10143#issuecomment-955945183

There isn't very much. This was already documented so the main change is update of the browser compatibility information.

Blocks: 1739563
Regressions: 1744425
Blocks: 1744425
No longer regressions: 1744425
Regressions: 1745613
Blocks: 1603144
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: