CVE-2022-28355: Scala.js should not provide a cryptographically insecure `UUID.randomUUID()` implementation · Issue #4657 · scala-js/scala-js

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

armanbilge opened this issue

Mar 25, 2022

· 16 comments

Assignees

Comments

I’m specifically referencing this section of code:

private lazy val rng = new Random() // TODO Use java.security.SecureRandom

def randomUUID(): UUID = {

val i1 = rng.nextInt()

val i2 = (rng.nextInt() & ~0x0000f000) | 0x00004000

val i3 = (rng.nextInt() & ~0xc0000000) | 0x80000000

val i4 = rng.nextInt()

new UUID(i1, i2, i3, i4, null, null)

}

The Java 8 docs for UUID.randomUUID() state:

Static factory to retrieve a type 4 (pseudo randomly generated) UUID. The UUID is generated using a cryptographically strong pseudo random number generator.

Furthermore, https://github.com/tc39/proposal-uuid states that:

Developers who have not been exposed to RFC 4122 might naturally opt to invent their own approaches
to UUID generation, potentially using Math.random() (in TIFU by using Math.random()
there’s an in-depth discussion of why a Cryptographically-Secure-Pseudo-Random-Number-Generator
(CSPRNG) should be used when generating UUIDs).

It’s unclear to me how a developer cross-compiling their library or application for Scala.js should become aware that in fact they cannot rely on UUID.randomUUID() for cryptographically strong UUIDs.

This seems a lot like a CVE to me.

See also discussion in typelevel/cats-effect#2882 (comment).

PS would be good to set up a security policy at https://github.com/scala-js/scala-js/security.

Copy link

Member

sjrd commented Mar 26, 2022

That is very unfortunate, and concerning.

This code was introduced during the infancy of Scala.js, 8 years ago:
b1e239d
I don’t think it would have passed review in the later years.

Unfortunately, removing the method now is not an option, since that would break backward binary compatibility. We can only do so in Scala.js 2.0.0.

The best we can do is mitigate the issue by using the available secure random facilities of recent browsers and Node.js when they are there. Hopefully, that will be good enough, as it’s unlikely that such kind of security would be a concern in another obscure environment.

I’ve never had to submit a CVE. I’ll have to check what is involved there.

Copy link

Contributor

gzm0 commented Mar 26, 2022

We might do a similar thing we did with weakref for java.secure.Random or UUID. That would isolate the CVE into a single “fake” package.

Most users will likely be OK to depend on crypto, so they can take the real package.

Thanks for your attention to this.

I was also thinking along the lines of of @gzm0’s suggestion: isolate the current implementation into an aptly named “not-cryptographically-strong-uuid-use-at-your-own-risk” package, like the fake weakref. Then provide “webcrypto-uuid” and “node-uuid” packages, for example.

Hopefully, that will be good enough, as it’s unlikely that such kind of security would be a concern in another obscure environment.

IMO I think it’s best to let the users make this sort of judgement call.

Copy link

Member

sjrd commented Mar 26, 2022 •

Ah yes, we could use a trick similar to WeakReference.

The situation is a bit different, though, since for WeakReference, the entire classes were broken without WeakRef support. Here, most of UUID is just fine. The only problematic method is randomUUID. There could be a number of libraries that only need to manipulate UUIDs, but not generate them using randomUUID. These libraries would have to make a choice on what UUID support library they depend on, for no reason, but it would have consequences for their users.

We might need to be a bit more creative than for the weak references.

These libraries would have to make a choice on what UUID support library they depend on, for no reason

Well, not really. Libraries won’t need UUID support for compiling. They may need it to run their tests, but that’s only a test dependency.

I do agree with your point though, that this is confusing.

Copy link

Member

sjrd commented Mar 26, 2022

But then they would have to tell their users to add a dependency on some of the UUID libs? That’s not helping. There is 0 chance we can clearly explain to all library maintainers how they must or must not depend on UUID and what to tell their users to do about that.

Copy link

Contributor

gzm0 commented Mar 26, 2022

The alternative is that we implement java.security.SecureRandom in that package and simply depend on it from UUID. (similar to what we do with java.text). The downside is that we likely increase the vulnerability surface in the vulnerable package.

Copy link

Member

sjrd commented Mar 26, 2022

That’s tempting from a linking architecture point of view but:

The downside is that we likely increase the vulnerability surface in the vulnerable package.

That is probably not acceptable. That would mean deliberately introduce in the ecosystem vulnerabilities that do not exist today, in order to mitigate one that does. Plus, while UUID.randomUUID is specified as cryptographically secure, it’s another level of expectation for SecureRandom whose name says that it’s secure.

Ok, here’s a new idea: what if we extract UUID into a separate artifact, that supports both Node.js and WebCrypto and throws a runtime error otherwise. This should cover the majority of use-cases, so libraries can depend on it without too much disruption for their users.

By keeping it a separate artifact, it still gives opportunity for applications to specifically exclude that dependency and replace it with something else suitable for their obscure environment. It’s not very convenient to do this, but it is possible.

I think it’s best to avoid providing the insecure implementation anywhere, if possible. It’s what started this mess to begin with.

Bonus points: add something to the sbt-scalajs plugin to make it easy to exclude the default UUID dependency.

Copy link

Contributor

gzm0 commented Mar 26, 2022

throws a runtime error otherwise

Well, if we are willing to go down that path, we can provide a SecureRandom that throws a runtime error. That would essentially have the same effect, without making the other parts of UUID unusable.

But at that point, I do not see the upside of this compared to making randomUUID not link anymore (except for honoring binary compatibility to the letter).

Copy link

Member

sjrd commented Mar 28, 2022 •

I have requested a CVE for this issue. We’ll see whether they actually give us one. From what I read about CVEs, this is a bit borderline when it comes to what “deserves” a CVE or not (it’s a library method, and an actual vulnerability can only be created in a context-dependent fashion, based on how it’s used).

I have given more thoughts about the possible fixes here, also talking to some other people here to get ideas. I am actually warming up to the idea of depending on SecureRandom, and implement SecureRandom in two different libraries:

• the fully secure one, requiring webcrypto or Node.’s crypto module (and otherwise throwing)
• the insecure one, in which we could actually implement something decent, while still advertising it as insecure.

For the latter, we could implement some CSPRNG algorithm. We would have to seed with a relatively bad source such as a combination of time + Math.random. But at least it would reduce the attack vector to having to guess that initial seed, as opposed to observing any publicly created UUID.

Thanks for looking into the CVE.

Yes, I like the SecureRandom idea too. That plan makes sense.

the insecure one, in which we could actually implement something decent, while still advertising it as insecure.

This also sounds like a good idea, and is certainly better than using an insecure PRNG :) FWIW I’m still not entirely sure if anybody actually needs this, but I guess it’s good to have just in case?

Copy link

Member

sjrd commented Mar 29, 2022

After discussion with the Scala Center, actually implementing the CSPRNG as fallback in the insecure version does not seem to be worth it. At least not as an urgent thing to do right now.

@gzm0 If that seems OK to you, I’ll proceed with creating two repos for SecureRandom. I guess the names could be scala-js-java-securerandom and scala-js-fake-insecure-java-securerandom. Does the latter reads "bad enough"?

sjrd added a commit to sjrd/scala-js that referenced this issue

Mar 29, 2022

…cureRandom.

Since Scala.js core does not implement `SecureRandom`, this means that `randomUUID()` will fail to link unless `SecureRandom` is otherwise provided.

We move the tests for `randomUUID()` in the test-suite-ex, and we implement a fake `SecureRandom` in the javalib-ext-dummies for testing purposes.

Copy link

Contributor

gzm0 commented Mar 29, 2022

Copy link

Member

sjrd commented Mar 30, 2022 •

OK, the 3 PRs are open.

As I see it, the next steps are:

1. Get approval on #4659, scala-js/scala-js-fake-insecure-java-securerandom#1 and scala-js/scala-js-java-securerandom#1
2. Merge #4659
3. Update the other two PRs to point to the resulting scala-js/scala-js head commit
4. Merge them when the CI passes
5. Launch the release process for Scala.js v1.10.0
6. Once artifacts are published, update the two securerandom libraries to use the released v1.10.0, and release them
7. Publish release notes for Scala.js v1.10.0
8. Issue a proper security advisory that tells people to upgrade to Scala.js v1.10.0

(I am still waiting for a reply about the CVE number.)

sjrd added a commit that referenced this issue

Apr 2, 2022

Fix #4657: Implement UUID.randomUUID() using java.security.SecureRandom.

Copy link

Member

sjrd commented Apr 2, 2022

We received the CVE number CVE-2022-28355 for this issue.