bcoin-org / bcrypto

JS crypto library
Other
99 stars 41 forks source link

Cannot read property 'randomBytes' of undefined #35

Open mpetrunic opened 5 years ago

mpetrunic commented 5 years ago

Bcrypto panics when rebuilded by electron. From my investigation it seems that electron replaces openssl with boressl and this causes some incompatibilities. It seems that requiring of native random passes and doesn't fallback to node version. I've mitigated it with (ELECTRON env is set by user , not electron itself):

  if(process && process.env.ELECTRON) {
    return require("bcrypto/lib/node/random").randomBytes(length);
  } else {
    return require("bcrypto/lib/random").randomBytes(length);
  }

bcrypto 4.2.8

mpetrunic commented 5 years ago

This is when pbkdf2 native is invoked on electron rebuilded bcrypto:

backend.load is not a function
      at derive (node_modules/@nodefactory/bls-keystore/node_modules/bcrypto/lib/native/pbkdf2.js:35:11)
tynes commented 5 years ago

@mpetrunic Have you seen this issue? https://github.com/bcoin-org/bcrypto/issues/27

At runtime, NODE_ENV can be set as js and that might fix the problem. The require of the module looks like this:

https://github.com/bcoin-org/bcrypto/blob/1d257bb1a505caf50181983f85a15a98d27de481/lib/pbkdf2.js#L9-L16

There is also pbkdf2-browser.js. https://github.com/bcoin-org/bcrypto/blob/master/lib/pbkdf2-browser.js

mpetrunic commented 5 years ago

Well code never reaches catch clause. It successfully requires native module and fails to execute function there. /node/pbkdf2 works perfectly so that means it would work if native throws exception,

mpetrunic commented 5 years ago

@tynes I debug this a lot, good thing it's not related to electron. It's jest that's causing this, apparently, moduleRegistry inside jest (they have custom loader because of mocking and stubbing) stores empty object({}) if module require throws. So in our tests in jest with electron rebuilded bcrypto, loading of sha256 throws with module did not self-register. and all other native modules load successfully because require('./bindings') returns "{}" instead of throwing.

Would you be willing to accept PR where all native modules have instead of

const assert = require('bsert');
const binding = require('./binding').random;

this

const assert = require('bsert');
const binding = require('./binding').random;

assert(binding);

? Which prevents during requiring binding to be undefined and it would throw causing bcrypto to fallback on js or node version.

alexbarnsley commented 4 years ago

Is there any update on this? I also get the same error during jest as @mpetrunic described. Thanks

mpetrunic commented 4 years ago

Bug is releated to jest and how it handles require calls. Aparently if require trows, jest will same empty object to modules cache and return it on all subsequent calls. Havent found solution yet

tynes commented 4 years ago

@alexbarnsley @mpetrunic Have you tried the latest version of bcrypto? A lot has changed including the removal of OpenSSL as a dependency.

braydonf commented 4 years ago

A lot has changed including the removal of OpenSSL as a dependency.

OpenSSL is still used, however is via the existing Node.js crypto API for example aes and random. Also sha{256,384,512}, hash160 and hash256 use OpenSSL via libtorsion, except on electron (which has a fallback).