Crash while doing setPrivateKey on ECDH

[NicholasBertazzon]

• Output of node_modules/.bin/electron --version: v4.0.1
• Operating System (Platform and Version): Windows 10 1809 (64 bit)
• Output of node_modules/.bin/electron --version on last known working Electron version (if applicable): v3.1.1

Expected Behavior
Shouldn't crash :)

Actual behavior
Electron crashes (half of it, I mean the process is still going on at almost 100% CPU, in background)

To Reproduce

const dh = crypto.createECDH('prime256v1');
console.log("step1");
dh.setPrivateKey(privateKey, 'base64');
console.log("step2");

Additional Information
It works with electron 3.1.1, but not 4.0.1
Maybe an update of Node could fix the issue

[jayarjo]

@NicholasBertazzon have you tried it on 4.0.4 (was released recently) or v5.0.0-beta.2?

[nornagon]

Here's the stack trace:

Received signal 11 SEGV_MAPERR 000000000000
3   ???                                 0x00007fe5f983aa08 0x0 + 140625710393864
4   Electron Framework                  0x000000011cc3efc1 node::crypto::ECDH::SetPrivateKey(v8::FunctionCallbackInfo<v8::Value> const&) + 385
5   Electron Framework                  0x0000000116a0e949 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo*) + 937
6   Electron Framework                  0x00000001169acbce v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) + 1182
7   Electron Framework                  0x00000001169aae21 v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) + 673
8   Electron Framework                  0x00000001169aa93c v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) + 124
9   Electron Framework                  0x00000001178a6715 v8_Default_embedded_blob_ + 2571349
[end of stack trace]

[nornagon]

This line in ECDH::SetPrivateKey looks mighty suspicious:

  BignumPointer priv(BN_bin2bn(
      reinterpret_cast<unsigned char*>(Buffer::Data(args[0].As<Object>())),
      Buffer::Length(args[0].As<Object>()),
      nullptr));

[chevonc]

Any traction on this? We're experiencing an app crash as well trying to upgrade to Electron 4.0. Particularly affecting - MatthieuLemoine/push-receiver#23

[jayarjo]

There's a pull request, that should address it, but so far there was no progress beyond that: #16822

[chevonc]

Thanks for the info @jayarjo. Glad to see there's a PR out 🎉

[nornagon]

I don't believe #16822 is related to this crash.

[jamesb3ll]

Yeah #16822 doesn't seem to be related. @nornagon could you look into this? Why would this break in an Electron environment? What seems suspicious in ECDH::SetPrivateKey?

[nornagon]

We patch some of node/v8's Buffer code for compatibility (e.g.), and that function in node seems to make some assumptions about the memory layout of buffers. cc @codebytere whom i think knows more about the Buffer patches.

[chevonc]

@jamesb3ll not sure if this is applicable here, but you posted a workaround on another repo's thread. Adding here for context. cc @codebytere

..calling `crypto.createECDH(...).setPrivateKey` is making Electron crash. I replaced `cryto.createECDH` with [`create-ecdh`](https://github.com/crypto-browserify/createECDH/blob/master/browser.js) module in [`decrypt.js`](https://github.com/MatthieuLemoine/push-receiver/blob/master/src/utils/decrypt/index.js#L12) and it works as expected without crashes. Let's hope Electron fixes this soon..

[ItsEcholot]

@jamesb3ll not sure if this is applicable here, but you posted a workaround on another repo's thread. Adding here for context. cc @codebytere

..calling `crypto.createECDH(...).setPrivateKey` is making Electron crash. I replaced `cryto.createECDH` with [`create-ecdh`](https://github.com/crypto-browserify/createECDH/blob/master/browser.js) module in [`decrypt.js`](https://github.com/MatthieuLemoine/push-receiver/blob/master/src/utils/decrypt/index.js#L12) and it works as expected without crashes. Let's hope Electron fixes this soon..

Sadly this workaround doesn't seem to work for me @v5.0.0-beta.3... Crashes with an malloc error. Any other ideas? This bug is currently keeping me back quite a bit @chevonc @codebytere.

The last working release I could find on npm was v4.0.0-nightly.20180821

[nornagon]

I did a little more digging and it looks like the culprit is actually this in node_crypto.cc:

  // To avoid inconsistency, clear the current public key in-case computing
  // the new one fails for some reason.
  EC_KEY_set_public_key(ecdh->key_.get(), nullptr);

The BoringSSL implementation of that function expects the key to be non-null.

[chevonc]

@nornagon Thanks for taking a stab. Any ETA on when this PR will hit release trains - #17219

[nornagon]

@chevonc should go out in the next releases of 4.x and 5.x-beta.

[jamesb3ll]

Thank you so much @nornagon 😃 I wish I'd knew how to fix it myself, I learnt a little bit about electron uses patches. Thanks!