Skip to content
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

[CVE-2022-28355] Scala.js should not provide a cryptographically insecure UUID.randomUUID() implementation #4657

Closed
armanbilge opened this issue Mar 25, 2022 · 20 comments
Assignees
Labels
bug Confirmed bug. Needs to be fixed.
Milestone

Comments

@armanbilge
Copy link
Member

armanbilge commented Mar 25, 2022

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.

@sjrd
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.

@gzm0
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.

@armanbilge
Copy link
Member Author

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.

@gzm0 gzm0 added the bug Confirmed bug. Needs to be fixed. label Mar 26, 2022
@sjrd
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.

@armanbilge
Copy link
Member Author

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.

@sjrd
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.

@gzm0
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.

@sjrd
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.

@armanbilge
Copy link
Member Author

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.

@gzm0
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).

@sjrd
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.

@armanbilge
Copy link
Member Author

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?

@sjrd
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.
@gzm0
Copy link
Contributor

gzm0 commented Mar 29, 2022

SGTM.

@sjrd
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 Fix #4657: Implement UUID.randomUUID() using java.security.SecureRandom. #4659, Initial implementation. scala-js-fake-insecure-java-securerandom#1 and Initial implementation. scala-js-java-securerandom#1
  2. Merge Fix #4657: Implement UUID.randomUUID() using java.security.SecureRandom. #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 sjrd self-assigned this Mar 30, 2022
@sjrd sjrd added this to the v1.10.0 milestone Mar 30, 2022
@sjrd sjrd closed this as completed in 9def628 Apr 2, 2022
sjrd added a commit that referenced this issue Apr 2, 2022
Fix #4657: Implement UUID.randomUUID() using java.security.SecureRandom.
@sjrd
Copy link
Member

sjrd commented Apr 2, 2022

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

@sjrd sjrd changed the title Scala.js should not provide a cryptographically insecure UUID.randomUUID() implementation [CVE-2022-28355] Scala.js should not provide a cryptographically insecure UUID.randomUUID() implementation Apr 4, 2022
@Atry
Copy link

Atry commented Apr 6, 2022

It seems there are some failures when upgrading Scala.js to 1.10
https://github.com/ThoughtWorksInc/Dsl.scala/runs/5826473345?check_suite_focus=true

@armanbilge
Copy link
Member Author

@Atry the Scala.js 1.10 release notes explain how to fix it :)
https://www.scala-js.org/news/2022/04/04/announcing-scalajs-1.10.0/

@gzm0
Copy link
Contributor

gzm0 commented Apr 6, 2022

Yes, that is expected. Please read the release notes: http://www.scala-js.org/news/2022/04/04/announcing-scalajs-1.10.0/

@Atry
Copy link

Atry commented Apr 6, 2022

Thank you! I found the related issue in ScalaTest scalatest/scalatest#2116

sjrd added a commit to sjrd/scalatest that referenced this issue Apr 6, 2022
…tingReporter.

The UUIDs are only used as an in-memory identifier for `Slot`s.
It is more efficient and more reliable to use an object identity
instead, for that purpose.

This removes the dependency on `UUID.randomUUID()`, which unblocks
using ScalaTest with Scala.js >= 1.10.0. See
GHSA-j2f9-w8wh-9ww4
and
scala-js/scala-js#4657
for the reason why `randomUUID()` fails to link by default in
recent Scala.js.
cheeseng pushed a commit to cheeseng/scalatest that referenced this issue Apr 7, 2022
…tingReporter.

The UUIDs are only used as an in-memory identifier for `Slot`s.
It is more efficient and more reliable to use an object identity
instead, for that purpose.

This removes the dependency on `UUID.randomUUID()`, which unblocks
using ScalaTest with Scala.js >= 1.10.0. See
GHSA-j2f9-w8wh-9ww4
and
scala-js/scala-js#4657
for the reason why `randomUUID()` fails to link by default in
recent Scala.js.
xerial added a commit to wvlet/airframe that referenced this issue Apr 11, 2022
* Remove dependency to UUID.randomUUID() as a workaround for scala-js/scala-js#4657

* [ci skip] fix comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug. Needs to be fixed.
Projects
None yet
Development

No branches or pull requests

4 participants