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

Expose important CSPRNG: crypto.getRandomValues #1003

Open
paulmillr opened this issue May 19, 2023 · 20 comments
Open

Expose important CSPRNG: crypto.getRandomValues #1003

paulmillr opened this issue May 19, 2023 · 20 comments
Labels
community-support-needed Facebook can't provide direct support for this issue, it is up to the community enhancement New feature or request

Comments

@paulmillr
Copy link

RN does not support globalThis.crypto. It's understandable, the module is huge.

There is no need in re-implementing the whole crypto module. Just one function: crypto.getRandomValues. The function exposes CSPRNG (cryptographically secure pseudorandom number generator). The function is critical to MANY apps which are not even related to deep tech.

It's important for one reason: without exposing the function, many people will go path of least resistance and simply switch to Math.random() which is PRNG, but not cryptographically secure. This has happened in the past and apps have been broken with catastrophic results: https://www.deepsource.com/blog/dont-use-math-random, https://owasp.org/www-community/vulnerabilities/Insecure_Randomness, https://blog.ledger.com/Funds-of-every-wallet-created-with-the-Trust-Wallet-browser-extension-could-have-been-stolen/

I write popular cryptographic libraries. Everything else is easily polyfillable. ed25519, sha3, pbkdf2, whatever. The CSPRNG is not easily polyfillable: it exposes low-level system interface. You cannot emulate or simulate this.

@elicwhite
Copy link
Member

It appears that there is this well used 3rd party package implementing getRandomValues, which when imported into your app polyfills globalThis. https://www.npmjs.com/package/react-native-get-random-values

The benefit of this approach is that it doesn't add to the size of React Native for all the apps that don't use this feature. It also addresses the tradeoffs I mention in this previous comment. facebook/react-native#20686 (comment)

What are your thoughts on this approach?

There is no need in re-implementing the whole crypto module. Just one function: crypto.getRandomValues

I'd also be nervous about such a partial implementation of the module officially in core. Perhaps that is a question for the Hermes team, Is there prior art of a partial implementation like this in Hermes?

@tmikov
Copy link
Contributor

tmikov commented May 19, 2023

Perhaps that is a question for the Hermes team, Is there prior art of a partial implementation like this in Hermes?

There have been quite a few issues like this, asking Hermes to implement various non-EcmaScript functionality. Unfortunately we can't maintain an ever expanding set of APIs in areas like crypto, where we lack relevant expertise. Plus, there is no technical reason why functionality like this needs to be part of Hermes - it doesn't rely on any internals, etc, and can be provided by integrators and/or polyfills.

v8 or jsc don't implement it either, it is provided by the browser, NodeJS, Bun, etc.

In previous similar issues I have mentioned that it might make sense to create a community supported central repository for additional functionality like this, but there doesn't seem to be enough interest or motivation for it.

Closing this, since it is not really Hermes related.

@tmikov tmikov closed this as completed May 19, 2023
@paulmillr
Copy link
Author

paulmillr commented May 19, 2023

It appears that there is this well used 3rd party package

So, you're saying React Native users who want to use anything that's related to CSPRNG (a lot of use cases and dependent modules) should defer to a 3rd party package maintained by someone random?

That seems terrible from a security standpoint.

JS lacks CSPRNG, and this is the only way of calling it. As far as I understand, the functionality is a simple redirect to /dev/random: https://github.com/LinusU/react-native-get-random-values/blob/master/ios/RNGetRandomValues.m - very simple to have around. No crypto expertise is necessary. Actually if you'll need a quick audit of such PR, I can provide one (my cryptographic libraries are popular).

Closing this, since it is not really Hermes related.

It's annoying to have issues closed, re-opened in different repositories, etc, without guidance where would a feature belong. Not a first time.

Where should the issue be opened, then? For the function to be present in React Native default configuration with Hermes enabled.

@tmikov
Copy link
Contributor

tmikov commented May 19, 2023

@paulmillr yes, I understand that this is annoying. But crypto is not provided by any other JS engine either, so I hope you also understand that this is not something that Hermes can solve, thus having this issue in our repository doesn't help.

React Native would theoretically be a more appropriate place to file this, but frankly I doubt that they would be willing to implement and maintain parts of crypto, judging by what @TheSavior said above and other similar discussions.

I am trying to gauge interest in creating a separate community project to serve as a repository for APIs like this one, but so far unfortunately there has been zero interest. I posted about it here https://twitter.com/tmikov/status/1659601939708137475?s=20, but I have little hope.

Ultimately someone has to do the work, which is non-trivial, and be responsible for maintenance, handling security bugs, etc.

@elicwhite
Copy link
Member

elicwhite commented May 19, 2023

It's annoying to have issues closed, re-opened in different repositories, etc, without guidance where would a feature belong. Not a first time.

I agree and recognize that this is frustrating. The previous issue you came from tried to provide clear guidance of where this feature belongs:

Both of the reasons you listed are exactly why I think this belongs outside of core. Ideally written and maintained by reputable people and companies with a reliance on its accuracy.

That led to the creation of the library I linked, where all the code is open source and auditable with 1M downloads a day. https://www.npmjs.com/package/react-native-get-random-values

That seems terrible from a security standpoint.

I think this was also addressed in the previous post, about why it would actually be worse off from a security and reliability perspective if we own it.

FWIW, I agree that the community needs this functionality, but there is a well supported and maintained solution to this problem already today.

What would help the most is to help us understand why the existing solutions to these problems aren't sufficient for the community, beyond just the arguments already discussed.

@paulmillr
Copy link
Author

paulmillr commented May 20, 2023

@TheSavior the library may be open-source and auditable, but it introduces a new attack vector on RN apps. RN app users need to trust library maintainer and his security practices to not add a malware into a new release. The maintainer of the other library can introduce silent backdoor that would make the CSPRNG output biased and you wouldn't even know about that until much later. I have the expertise to audit the library, other users don't.

Ultimately someone has to do the work, which is non-trivial, and be responsible for maintenance, handling security bugs, etc.

The work is really trivial, as you can see from the polyfill. It's one system call, to /dev/random. Takes 10 lines of code. Now, I agree re-implementing webcrypto, and all its functionality is really complicated. But, the other webcrypto methods can be emulated in JS - and CSPRNG cannot.

But crypto is not provided by any other JS engine either

Semantics doesn't matter to users. Is there a standard? Sure, W3 one: https://www.w3.org/TR/WebCryptoAPI/. Is the feature available in browsers, bun, deno, node? Sure thing. Is the feature NOT available in RN? Yep, it lacks it. So, to me, as an abstract user, code I write runs properly everywhere and doesn't run in RN.

Semantics may matter to you because you maintain Hermes and you are not DRO for this particular thing. But then, who is responsible, if it's all funded by FB? Hard to tell from the outside.

To summarize:

  1. CSPRNG is much more important than all other parts of webcrypto, combined.
  2. CSPRNG takes 10 lines of code and basically no time to maintain.
  3. CSPRNG cannot be emulated in JS, other functionality can.
  4. Making users depend on 3rd party library for this massively increases attack surface for RN apps.
  5. It's possible by the polyfill author to introduce silent backdoors that bias CSPRNG which would not be spotted by non-experts.
  6. The feature is already present in every other runtime.

@tmikov
Copy link
Contributor

tmikov commented May 20, 2023

@paulmillr I disagree that "CSPRNG takes 10 lines of code and basically no time to maintain". It needs an implementation for Linux, Windows, MacOS+iOS and Android. On Android it has to call back into Java. It needs tests. It needs someone to research what the most secure CSPRNG is on each of these platforms. Even if we did that, we would be providing a single method in the crypto namespace, which will invite more questions, bug reports, etc.

However I agree with the rest of your reasoning. I would go even further and say that RN users would probably benefit from a complete, trusted and systematically developed SDK, containing more than crypto, instead of hunting for individual NPM packages for things already provided by browsers and NodeJS.

However something like that would be a major and expensive undertaking, and so far unfortunately I am not seeing real interest in it in the broader community.

FWIW, one of the next major versions of Hermes will make it much easier to invoke native APIs without needing a separate C++ module. So, in that sense it will become possible to implement things like this directly in JS.

@LinusU
Copy link

LinusU commented May 22, 2023

@paulmillr I invite you and anyone else to audit react-native-get-random-values, that would be great!

I'm not sure exactly how it would work, but I imagine that you would publish the results of your audit together with the integrity field that npm uses in the package-lock.json, e.g. sha512-H/zghhun0T+UIJLmig3+ZuBCvF66rdbiWUfRSNS6kv5oDSpa1ZiVyvRWtuPesQpT8dXj+Bv7WJRQOUP+5TB1sA== for version 1.8.0.

@paulmillr
Copy link
Author

paulmillr commented May 22, 2023

@LinusU your current version is probably more or less secure. FYI for the time being i'm recommending your module through my cryptographic libs.

My point is that, however secure it may be, every future release must be audited as well. Ideally, releases would be rare, because of that. Also there are concerns with regards to your opsec - how plausible is it for a malware to infect your computer and upload bad version to npm? I would think Meta's opsec is better. Having additional entities to trust in something as important as CSPRNG is not optimal.

@LinusU
Copy link

LinusU commented May 22, 2023

Opsec is a very valid concern, whilst I of course use 2fa and follow best practices, a lone open source developer is going to have a hard time to stand up against a target attack that has resources behind it...

Hopefully all users would follow security best practices and audit the package whenever their lock file changes, but many projects/companies probably doesn't...


We're going a bit of topic here but it would be really nice if Npm, or a third party, could provide a tool for auditing. Where you would be able to see who has audited which version, etc.

@tmikov
Copy link
Contributor

tmikov commented May 23, 2023

@paulmillr @LinusU I agree that this is a problem that needs to be addressed. To be constructive, I am proposing the following compromise solution. We will review and if it s good quality accept a Hermes PR for crypto.getRandomValues(). It needs to support al of our OSes - Android, iOS, MacOS, Linux, Windows, and pass CI tests.

@paulmillr
Copy link
Author

@tmikov sounds great.

I can provide audit from my end. Will defer coding to someone else who is familar with React Native. Could this issue be re-opened for visibility?

@tmikov tmikov reopened this May 24, 2023
@tmikov
Copy link
Contributor

tmikov commented May 24, 2023

Reopened the issue.

@paulmillr the code can't depend on React Native, it has to use only facilities provided by Hermes.

@elicwhite
Copy link
Member

@tmikov, do you happen to have a good code pointer for @paulmillr of another feature in Hermes implemented like this to go from? Perhaps something from Intl?

@tmikov
Copy link
Contributor

tmikov commented May 25, 2023

Intl is probably not a great starting point, since it is rather complex. Unfortunately we don't really have examples or documentation for how to add library functionality to Hermes, but the source is pretty well commented.

@paulmillr
Copy link
Author

@LinusU since you already have this great polyfill, maybe you could help us with this, whenever you'll have a time?

@shamilovtim
Copy link

Curious what is the distinction of security for CSPRNG vs. all of the other various unaudited node modules in a Javascript crypto project. Since they aren't sandboxed from each other doesn't that render JS crypto projects inherently insecure since they will always have thousands of dependencies?

@paulmillr
Copy link
Author

@shamilovtim

they will always have thousands of dependencies

No, they won't, if that's your goal. For example, my goal was to massively reduce dependency count for important projects. One case was reducing ethereum-cryptography deps from 38 to 5 by writing 4 new minimal dependencies and having a third-party auditor to oversee it, the other one was rewriting 10M downloads/week module from the ground to save 32 terabytes of traffic per week for NPM users.

Just a matter of priorities.

@tmikov
Copy link
Contributor

tmikov commented Jul 28, 2023

High level discussion of out of the box non-ECMAScript APIs here: #1072

@tmikov tmikov added the community-support-needed Facebook can't provide direct support for this issue, it is up to the community label Oct 20, 2023
@paulmillr
Copy link
Author

I'm sure the community does not (and should not) understand what's cryptographically secure random and what's not. Maybe consult with your in-company security guys instead if that's important or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-support-needed Facebook can't provide direct support for this issue, it is up to the community enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants