paulmillr / noble-hashes

Audited & minimal JS implementation of hash functions, MACs and KDFs.
https://paulmillr.com/noble
MIT License
551 stars 46 forks source link

BigInt syntax errors in unsupported environments #12

Closed jacogr closed 2 years ago

jacogr commented 2 years ago

... you are not going to like this one, bear with me :)

Ok, I started pulling this in. Basically in my case I still need to support some ancient environments as well, which is not that much of an issue, basically on my hashing wrappers I do the following -

import { blake2b as blake2bJs } from '@noble/hashes/lib/blake2b';

// wasm
import { blake2b as blake2bWasm } from '@polkadot/wasm-crypto'; 

export function blake2 (u8a: Uint8Array): Uint8Array {
  return typeof BigInt !== 'undefined'
    ? blake2bJs(u8a)
    : blake2bWasm(u8a);
}

So effectively there is some fallback in there. (The above is typed from memory as illustration, but shows the point. Normally when wasm is initialized, it actually is the preferred route in my case. The same applies to eg fallback to blake2-js when BigInt is not available)

Anyway, all good. Now however the issue is the following - on some older environments, such as for instance React Native, it even throws up when trying to parse the modules, e.g. this is problematic (although not used at runtime) https://github.com/paulmillr/noble-hashes/blob/main/src/_u64.ts#L1

I did a quick search for the 32n e.g. /\dn/ and there are actually not that many. So for instance, we could get around it with the following ...

// _64.ts 
// suggested adjustments as an example, some files will need adjusting 
// yes, it is, well ... horrid :(
export const _BigInt: (value: string | number | bigint | boolean) => bigint =
  typeof BigInt !== 'undefined'
    ? BigInt
    : () => undefined as unknown as bigint;

const U32_MASK64 = _BigInt(2 ** 32 - 1);

const _32n = _BigInt(32);

export function fromBig(n: bigint, le = false) {
  if (le) return { h: Number(n & U32_MASK64), l: Number((n >> _32n) & U32_MASK64) };
  return { h: Number((n >> _32n) & U32_MASK64) | 0, l: Number(n & U32_MASK64) | 0 };
}

The above is problematic, obviously since it will give undefined results in non-BigInt environments. In BigInt environments there is no extra overhead, well, maybe slightly at library load. However for non-BigInt-envs inside functions it does use BigInt which would throw up at runtime.

At this point the TL;DR is -

  1. I'm making sure that at runtime, the BigInt libs are not used (if not available)
  2. I still have support issue where library users now complain that it doesn't compile in their environments

So my options are -

  1. Having to debug all user environments to figure out why
  2. Documenting all of it
  3. Making a PR here with "some" hacks like above

None of the options are fun atm :) Other suggestions welcome.

jacogr commented 2 years ago

EDIT: I actually have this issue as it stands in the current version of my libs, i.e. had this (it is not used anywhere, only declared in a file that has a function exported) -

const SQRT_MAX_SAFE_INTEGER = 94906265n;

Took a similar "hack" approach now as laid out above (to not spend all day on issues) to get around it with a _BigInt(94906265) definition where it is defined. At runtime, if called and BigInt is not available, the function will fail since BigInt(<some-number>) is used internally to it.

jacogr commented 2 years ago

Made a PR here since I had a small gap and always prefer seeing things in code - https://github.com/paulmillr/noble-hashes/pull/13

However, feel free to close this and that after some thought it is absolutely horrid in your eyes, it is just an approach.

paulmillr commented 2 years ago

If there are no bigints, we should absolutely NOT fall back to something else, and instead an error should be thrown.

It's like falling back to "math random" when crypto.secureRandom is not available. Very bad idea.

jacogr commented 2 years ago

Agreed. So in this case, there is detection -

return typeof BigInt !== 'undefined'
  ? somethingWithBi(...)
  : somethingWithNoBi(...)

The issue is the build steps - it pulls in all code (even unused libs) and then fails on things like 32n.

So there are def. 2 parts -

paulmillr commented 2 years ago

when wasm is initialized, it actually is the preferred route in my case

Can I ask, why? WASM:

  1. Cannot be verified. You cannot be sure you're not running a malware. The binaries are unsigned. You're downloading them from NPM which was allowing anyone to upload a new version of any package without auth for a few years.
  2. Eats more storage / network than JS
  3. Is in some cases faster, but it's very rare that you need huge hashing speed in JS apps. Huge meaning 1M ops/sec instead of 200k ops/sec. Not instead of 2k ops/sec.

ensuring that old env can actually import the code

If the old env is able to import the code, but then fails immediately, what's the point here? I think the environments must update to be compliant with modern ECMAScript.

Leaving bigint code as-is would increase the pressure on build tools to be compliant. E.g. if we merge the pull request, less people would know about the problem, and less people would report to react-native.

Anyway i'll leave the PR open for now until we get more environment examples. If enough people report this, we'll prob go ahead. Note that it's not just noble-hashes that needs to be adjusted, it's also all other noble libraries.

paulmillr commented 2 years ago

Another question. How would your env handle an error that would be thrown if the bigints do not exist, during the runtime Can the error be catched?

paulmillr commented 2 years ago

Also, it seems like the most recent react-native supports bigints. The problem only resides in devices with ios 13.7 or earlier. (current is v15)

jacogr commented 2 years ago

100% - however, I'm already this week flooded with support and all I did was adding unused bi utils :) The plan is to swap to noble in the next major release (which I want to do soon), however I cannot force everybody to upgrade their environments, but a lot of times I do have to force them to upgrade my libs and if the two don't quite work as expected, great unhappiness.

(In the case of RN, according to the Ledger Live developers, it may be solved by an upgrade from Expo to Hermes, but it is not a small undertaking. Current version of my libs, even before @noble/hashes integration is an issue for this team as it stands, so have applied the same kind of to-be-released fixes for the bigint utils included there... it is what I could come up with...)

So I will point you to actual (in master branch) code using @noble/hashes -

https://github.com/polkadot-js/common/blob/master/packages/util-crypto/src/blake2/asU8a.ts#L33-L35

The Rust libs used for WASM is part of the audit for the Polkadot runtime, i.e. use the same versions as there. So yes, I agree, it is not readable, e.g. https://github.com/polkadot-js/wasm/tree/master/packages/wasm-crypto/src/rs and it brings a host of other issues with it (i.e. for React Native it also needs asm.js since it doesn't do Webassembly... but at least in this case it now provides a fallback for that env)

So, in my case, I guess for the time being I'll revert @noble/hashes (or include it with these changes as a fork), since all hell will break loose in my life... it already has :)

Sadly I deal with a lot of different environments using the libs and, well, I would love to tell them to just not use it, but the ecosystem has no other options apart from what is provided by my libs - so that needs to cater for a wide spectrum. I am trying my best to drive forward bit by bit (and not get held back).

As for other libs, I am aware of it, i.e. the @noble/secp256k1 I have a PR for, but it is very problematic - since I don't have a WASM fallback, so it really is all-or-nothing. Luckily it is only a small part, i.e. most keys are sr25519 (https://github.com/polkadot-js/wasm/blob/master/packages/wasm-crypto/src/rs/sr25519.rs) or ed25519 (tweetnacl, I have not attempted @noble/ed25519 as of yet)

paulmillr commented 2 years ago

So, if we merge the BigInt changes to secp and hashes, would that help your users with old React Natives? Does RN support BigInts, just not syntax?

jacogr commented 2 years ago

Yes, it would allow me to swap over completely. On some environments, e.g. iOS, will have 100% support - on some older RN the fallbacks would be used. However, it does mean it can be available for 99% of end-users with no issues (i.e. have JS hashing available, the remainder is on fallback) and available for 100% of all library users with no issues in their tooling.

In the end it needs to make 100% sense here, if not it should not go in.

Sometimes I wish it was 2025 already and 2020-era new is everywhere... :)

paulmillr commented 2 years ago

The question is still in the error thrown on import. Would you be able to handle it?

jacogr commented 2 years ago

On use of a non-compliant function, indeed. An example of this is the sr25519 pair functions, they are only available in WASM (or asm.js under RN - only included in this environment, too big otherwise). If there is no WASM support, they do fail and it is up to the caller to handle.

So on my side (others may do it differently), each hashing function has a check for typeof BigInt !== 'undefined' to determine the path to follow. If it then calls into the BigInt-using function and that does fail for whatever reason, it won't use another fallback attempt, rather it is up to the library user to sort out. So basically need to ensure I do the BigInt-available check before calling into anything.

(secp, when I get to it, will be different, with no BigInt it will just fail outright without even trying to call into @noble/secp256k1. In my case it is a tradeoff than can be made since 99.9% of keys are all ed25519 or sr25519)

On import itself, I don't have lazy imports, so if I do -

import { blake2b } from '@noble/hashes/lib/blake2b';

and that fails, there is an issue. However when calling into blake2b above, it is fine since I better make sure the env can handle it.

Does that make sense?

paulmillr commented 2 years ago

I'm thinking about throwing an error on import. What users of yours would be affected by it?

jacogr commented 2 years ago

Really not 100%. Will need to see what it looks like code-wise at the end to determine impact.

It is probably mostly my RN users. It is more tricky to determine capability paths (for me downstream) at import. Obviously at runtime things like this (random example) would work without issues -

class BLAKE2b extends blake2.BLAKE2<BLAKE2b> {
  // Same as SHA-512, but LE
  // ... snipped ...
  constructor(opts: blake2.BlakeOpts = {}) {
    if (typeof BigInt ==='undefined') throw new Error('BigInt is not available');

But it does have a slight runtime impact.

At file level it would be trickier (for me), but I'm guessing it is the 100% clear solution here for this.

paulmillr commented 2 years ago

yeah, I don't want runtime impacts for stuff that's used by 1% of users. I don't see big disadvantages in changing 1n to BigInt(1) tho.

aulneau commented 2 years ago

Just as a side note, this is something i have also ran into trying to support react-native usage of my library micro-stacks. the n helper (while not the nicest to look at), would make a huge impact in being able to support more environments. I spent some time trying to build out a babel plugin that would transform native bigint usage to something like jsbi, but with a change like this in both the hashes and secp256k1 it would be much more realistic.

paulmillr commented 2 years ago

Thanks @jacogr, i've added this, but without _n util. You'll need to replace BigInt on your own in your app.

jacogr commented 2 years ago

Thank you. I think that is a sane approach.

jacogr commented 2 years ago

Edit to the above: I have something similar for secp256k1 (in this case also BigInt(…) wrappers), will contribute… once again if somebody doesn’t get there before me…