flipper icon indicating copy to clipboard operation
flipper copied to clipboard

Network Plugin HTTP Response body always (Empty)

Open davidfuzju opened this issue 3 months ago • 1 comments

🐛 Bug Report

Network Plugin HTTP Response body always (Empty) image

To Reproduce

just use Network Plugin and check HTTP response body

Environment

iOS Version: 17.4.1 Flipper Desktop version: 0.251.0

davidfuzju avatar Mar 29 '24 07:03 davidfuzju

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. https://github.com/facebook/react-native/pull/20686#issuecomment-612503367

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?

TheSavior avatar May 19 '23 14:05 TheSavior

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 avatar May 19 '23 16:05 tmikov

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.

paulmillr avatar May 19 '23 16:05 paulmillr

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

tmikov avatar May 19 '23 17:05 tmikov

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.

TheSavior avatar May 19 '23 18:05 TheSavior

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

paulmillr avatar May 20 '23 02:05 paulmillr

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

tmikov avatar May 20 '23 04:05 tmikov

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

LinusU avatar May 22 '23 10:05 LinusU

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

paulmillr avatar May 22 '23 10:05 paulmillr

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.

LinusU avatar May 22 '23 11:05 LinusU

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

tmikov avatar May 23 '23 20:05 tmikov

@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?

paulmillr avatar May 24 '23 10:05 paulmillr

Reopened the issue.

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

tmikov avatar May 24 '23 16:05 tmikov

@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?

TheSavior avatar May 25 '23 04:05 TheSavior

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.

tmikov avatar May 25 '23 05:05 tmikov

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

paulmillr avatar May 26 '23 11:05 paulmillr

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?

shamilovtim avatar Jul 16 '23 15:07 shamilovtim

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

paulmillr avatar Jul 16 '23 21:07 paulmillr

High level discussion of out of the box non-ECMAScript APIs here: https://github.com/facebook/hermes/discussions/1072

tmikov avatar Jul 28 '23 16:07 tmikov

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.

paulmillr avatar Jan 17 '24 21:01 paulmillr