Warn if fake secure random is on the classpath

[armanbilge]

In light of scalatest/scalatest#2116, I think it would be a very good thing if there was more hand-holding regarding the fake secure random artifact. Since downstreams can receive this dependency transitively, it is really not a nice situation for them if an upstream accidentally forgets to add % Test or worse.

This is a similar situation to #4610 or the warning for the global EC. Warn, because it's probably not what the user wants, and give actionable instructions how to fix it.

If the sbt-plugin could make a best effort to check the classpath for the fake secure random and log a warning that alerts users to its presence. Or honestly even raise a fatal error, unless you explicitly opt-in a setting clearly indicating that this is what you want to do. Although the dependency can be inherited transitively, this opt-in cannot be, which is the point.

Or maybe this could be done on the linker level, IDK.

Thanks.

[sjrd]

It says here:

Usage from a library

Never, ever depend on scalajs-fake-insecure-securerandom from library code!

If you do, you will expose your users to insecure code.

Always use scalajs-java-securerandom instead.

This is the only page that shows the dependency that you need to write to get the fake insecure library. We made sure of that, so that library maintainers basically have to go there and see that message.

If library maintainers ignore this big fat warning, no amount of hand-holding will be enough.

---

Using % Test is not an excuse. Either you need SecureRandom for your library and you depend on scalajs-java-securerandom, or you don't. There's no reason to depend on this library only in the Test configuration, unless for some reason your tests themselves need SecureRandom although your main library code doesn't.

---

The linker wouldn't be able to tell the difference. It doesn't know where things come from.

The sbt plugin of course technically could. The situation is not the same as #4610 however. In that case, Scala.js core itself provides something which is worth warning about. You get it even if you're not trying. Here, Scala.js core doesn't know that this insecure library exists. It also wouldn't know if any third-party ever developed an insecure library for Scala.js. It wouldn't know if any library contained similar, custom insecure code (by design, or unintentionally). In this case I don't think adding a warning in the core is appropriate.

[gzm0]

I second this. Such a thing would essentially be a poor-man's security scanner.

If a maintainer is concerned about the security of transitive dependencies they should have a proper scanner.

What we can (and probably should) do is to publish a security advisory on scalajs-fake-insecure-securerandom to make it as likely as possible that scanners pick it up.

[sjrd]

What we can (and probably should) do is to publish a security advisory on scalajs-fake-insecure-securerandom to make it as likely as possible that scanners pick it up.

I've been thinking about this, and I can't convince myself either way. The problems I see with doing this are:

• it feels like an abuse of the system, and abusing the system might reduce the trust from security people (including GitHub people reviewing our advisories) in us,
• what would people with a genuine need of the fake-insecure library do? They would have to live forever with a spurious warning that they cannot address.

[armanbilge]

I've bit my tongue. Your technical arguments are solid, but IMHO I feel there's a bigger picture.

Here, Scala.js core doesn't know that this insecure library exists. It also wouldn't know if any third-party ever developed an insecure library for Scala.js.

I offer the alternative non-technical interpretation that you are Scala.js core, you know about this insecure library, and in fact you developed it and published it (not a third party).

There's no reason to depend on this library only in the Test configuration

Except, people seem to be doing it anyway :) I hope we've all seen the Discord questions. I think the popularity of the JSDOM env for testing, which doesn't support WebCrypto, throws a wrench in this. Maybe that's the technical problem that needs to be solved.

[sjrd]

I think the popularity of the JSDOM env for testing, which doesn't support WebCrypto, throws a wrench in this. Maybe that's the technical problem that needs to be solved.

Trying to address that at the source: jsdom/jsdom#3352

[gzm0]

it feels like an abuse of the system, and abusing the system might reduce the trust from security people (including GitHub people reviewing our advisories) in us,

I haven't thought about it that way. Maybe indeed :-/ @paullepoulpe, maybe you can give us your perspective on this (summary of what happened at the bottom of this comment)?

what would people with a genuine need of the fake-insecure library do? They would have to live forever with a spurious warning that they cannot address.

IMHO this is not a use-case we should support. A genuine need for this library only arises due to backwards compatibility scenarios. Any user of randomUUID that wants a non-cryptographically secure random UUID is using the wrong method. They should use a different way to generate the UUID.

** Summary for @paullepoulpe **

1. Vulnerability in UUID.randomUUID() is discovered (generated IDs are not cryptographically random).
   [CVE-2022-28355] Scala.js should not provide a cryptographically insecure UUID.randomUUID() implementation #4657
2. Vulnerable code is removed from Scala.js core
   Fix #4657: Implement UUID.randomUUID() using java.security.SecureRandom. #4659
3. scala-js-fake-insecure-java-securerandom is created, isolating the vulnerability (to maintain backwards compatibility). A proper implementation is also provided, but it doesn't work on all JS platforms.

Our question: Is posting a security advisory (without patched versions) on the fake implementation an abuse of security advisories?

For reference: Scala.js core advisory

[armanbilge]

So I've been working on improving scoverage support for Scala.js, and I stumbled across another use of the fake insecure random in its runtime.

    // While not exactlu ideal, this is only used in the invoker to assign a
    // unique id to ensure measurements have unique ids. It's never exposed to
    // the user and doesn't touch anything sensitve, so we should have no
    // issues here. Still, I don't like having this, so we should try to
    // replace it.
    libraryDependencies += ("org.scala-js" %%% "scalajs-fake-insecure-java-securerandom" % "1.0.0")
      .cross(CrossVersion.for3Use2_13)

https://github.com/scoverage/scalac-scoverage-plugin/blob/1d404ca3f12d00c75820da2783158f9eee82e97c/build.sbt#L124-L130

There seem to be some serious misunderstandings about how this dependency works...

If library maintainers ignore this big fat warning, no amount of hand-holding will be enough.

😕

[ckipp01]

At the risk of looking foolish, I'll jump in here and say that the inclusion in scoverage was my fault. Looking more closely now, I definitely understand how foolish of a decision it was to include it, but I'll give a bit of insight into why it was done.

• I never use ScalaJS, so my desire to quickly bump and move on as quickly as possible caused me to not read nearly as closely as I should have
• Even with the warning, I glossed over them. Again, this is 100% my fault, but I looked at insecure in a similar way as I read the def insecureRandomUUID that @armanbilge replaced it with. We have a terrible habit on the JVM at times of using the word insecure with a ton of caveats that sort of just teach people to not look as closely as they should. In this example my brain didn't register the actual issue, and it was "not an issue" and I moved on.

Again, this was totally my fault, and due to me not nearly being as diligent as I should have been here (I'll for sure remember this), but I just wanted to include this here as an example that I do think there are instances out there where this will happen again, and people like myself that aren't super invested in a project, and doing as little as possible to keep a project afloat, will take the easiest route, which in this case was a terrible decision.

I honestly don't even think the insecure-securerandom should exist, as this will probably happen again. @armanbilge, thanks for catching this and quickly sending in a PR to fix it. I've cut a new release (thankfully the one that included it wasn't used in any of the scoverage plugins, was an RC, and was only a day old), and I've also included a large warning in the release notes about the affected version.

[sjrd]

Perhaps the best question to ask is: what directed you to the fake-insecure version instead of the actual secure version https://github.com/scala-js/scala-js-java-securerandom ?

[ckipp01]

Perhaps the best question to ask is: what directed you to the fake-insecure version instead of the actual secure version https://github.com/scala-js/scala-js-java-securerandom ?

I looked at the secure version first and it wasn't clear to me if it would work as a drop replacement in every environment that was needed, since it mentions a note about unsupported environments.

[armanbilge]

In this case, it wouldn't have worked in the JSDOM environment I added scoverage support for actually.

[gzm0]

I honestly don't even think the insecure-securerandom should exist, as this will probably happen again.

@ckipp01 so just to clarify this: You would have preferred if we explicitly broke backwards compatibility by not providing fake-insecure?

[ckipp01]

I honestly don't even think the insecure-securerandom should exist, as this will probably happen again.

@ckipp01 so just to clarify this: You would have preferred if we explicitly broke backwards compatibility by not providing fake-insecure?

I don't have a straightforward answer for this, because I don't have enough context to know the ramifications of this in the scala-js ecosystem. My initial gut reaction to this was, yes, of course. If you've identified a serious security issue and needed to break compatibility due to to it, then I'd prefer this, especially if it included specific instructions on how to address it for library maintainers, or it was versioned accordingly to reflect it. Providing a by design insecure package as a drop in replacement gives the false assumption that you shouldn't do this, but it's maybe not that big of a deal since we're providing it to you. You're handing people a way to shoot themselves in the foot.

Again, in this scenario I was at fault, and didn't look at this with the due diligence that was needed. But you'll have projects that are maintaining scala-js even though they have no interest in scala-js, which causes them to do the most minimal work necessary to bump when needed. So if the most frictionless option is to just drop in a package that the project created, even if labeled insecure, people will do it. I'm not saying it's wise, right, or anything, it's just a reality that people will do it. So after typing all this, I guess yea, I can confidently say for myself, I'd rather you break compatibility than easily provide a package that is insecure by design.

Again, I say this without fully understanding the larger ramifications that this might have on the ecosystem.

[japgolly]

I'm very glad the insecure replacement exists. Let's keep in mind what we actually mean by "security" here: we mean cryptographically secure wrt entropy IIRC. For me personally so far, such a concern would be extremely pedantic in cases where I'm using this from ScalaJs. I'm glad it's been identified and considered but now that it has, I usually just need something to poop out a few uuids in unit tests and using the secure package didn't work for me. I'm thankful I could just switch to the insecure package and not have to deal with further headache.

[armanbilge]

using the secure package didn't work for me

@japgolly why was this? Is it because you are using JSDOM?

[japgolly]

Yes I was using JSDOM, are they incompatible?

…

On Thu, 2 June 2022, 11:36 pm Arman Bilge, ***@***.***> wrote: using the secure package didn't work for me @japgolly <https://github.com/japgolly> why was this? Is it because you are using JSDOM? — Reply to this email directly, view it on GitHub <#4663 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABRRN7ZHVWXTHPHM2L4YTTVNC2GHANCNFSM5SVYQQ5A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[armanbilge]

I think that JSDOM is only modern JSEnv that doesn't currently support cryptographically secure random numbers. But Seb has already fixed that in jsdom/jsdom#3352 :)

[armanbilge]

https://github.com/jsdom/jsdom/releases/tag/20.0.0 🎉

Now I think some projects should be encouraged to remove this dependency.
https://github.com/search?q=scalajs-fake-insecure-java-securerandom+-user%3Ascala-js+language%3AScala&type=Code&ref=advsearch&l=Scala&l=