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

workers, crypto: passing KeyObject as workerData crashes #35263

Closed
jasnell opened this issue Sep 18, 2020 · 11 comments
Closed

workers, crypto: passing KeyObject as workerData crashes #35263

jasnell opened this issue Sep 18, 2020 · 11 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. good first issue Issues that are suitable for first-time contributors. worker Issues and PRs related to Worker support.

Comments

@jasnell
Copy link
Member

jasnell commented Sep 18, 2020

@addaleax @nodejs/workers ... The following segfaults on master and 14.x ...

I'll be investigating shortly...

'use strict';

const { createSecretKey } = require('crypto');

const { Worker, isMainThread, workerData } = require('worker_threads');

if (isMainThread) {
  const key = createSecretKey(Buffer.from('hello'));
  new Worker(__filename, { workerData: key });
} else {
  console.log(workerData);
}

lldb yields...

james@ubuntu:~/node/node$ lldb -- node ../tmp/test
(lldb) target create "node"
Current executable set to 'node' (x86_64).
(lldb) settings set -- target.run-args  "../tmp/test"
(lldb) r
Process 20852 launched: '/home/james/node/node/node' (x86_64)
Process 20852 stopped
* thread #2, name = 'node', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x00005555560fb837 node`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) + 103
node`v8::internal::(anonymous namespace)::Invoke:
->  0x5555560fb837 <+103>: movq   (%r13), %rax
    0x5555560fb83b <+107>: testb  $0x1, %al
    0x5555560fb83d <+109>: jne    0x5555560fba10            ; <+576>
    0x5555560fb843 <+115>: movl   0x3320(%rbx), %r13d
(lldb) bt
* thread #2, name = 'node', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x00005555560fb837 node`v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&) + 103
    frame #1: 0x00005555560fcb7d node`v8::internal::Execution::New(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*) + 109
    frame #2: 0x0000555555f8f01f node`v8::Function::NewInstanceWithSideEffectType(v8::Local<v8::Context>, int, v8::Local<v8::Value>*, v8::SideEffectType) const + 415
    frame #3: 0x0000555555ee51d8 node`node::crypto::KeyObjectHandle::Create(node::Environment*, std::shared_ptr<node::crypto::KeyObjectData>) + 40
    frame #4: 0x0000555555ee6f91 node`node::crypto::NativeKeyObject::KeyObjectTransferData::Deserialize(node::Environment*, v8::Local<v8::Context>, std::unique_ptr<node::worker::TransferData, std::default_delete<node::worker::TransferData> >) + 385
    frame #5: 0x0000555555d73130 node`node::worker::Message::Deserialize(node::Environment*, v8::Local<v8::Context>) + 464
    frame #6: 0x0000555555d78335 node`node::worker::MessagePort::ReceiveMessage(v8::Local<v8::Context>, bool) + 1605
    frame #7: 0x0000555555d788b8 node`node::worker::MessagePort::OnMessage() + 312
    frame #8: 0x000055555686c8c6 node`uv__async_io(loop=0x00007ffff447ba48, w=<unavailable>, events=<unavailable>) at async.c:163
    frame #9: 0x0000555556880f84 node`uv__io_poll(loop=0x00007ffff447ba48, timeout=<unavailable>) at linux-core.c:461
    frame #10: 0x000055555686d1fa node`uv_run(loop=0x00007ffff447ba48, mode=UV_RUN_DEFAULT) at core.c:385
    frame #11: 0x0000555555e00336 node`node::worker::Worker::Run() + 5766
    frame #12: 0x0000555555e00707 node`node::worker::Worker::StartThread(v8::FunctionCallbackInfo<v8::Value> const&)::'lambda'(void*)::_FUN(void*) + 71
    frame #13: 0x00007ffff707a6db libpthread.so.0`start_thread + 219
    frame #14: 0x00007ffff6da3a3f libc.so.6`__GI___clone at clone.S:95
(lldb)
@jasnell
Copy link
Member Author

jasnell commented Sep 18, 2020

Ok... yeah, it's what I suspected... KeyObjectHandle::Create() depends on env->crypto_key_object_handle_constructor() being set, which it is not until internalBinding('crypto') is called... so when the attempt is made to deserialize the KeyObject in the Worker, we get a crash.

I added a CHECK to verify in my local branch:

./node[21545]: ../src/crypto/crypto_keys.cc:863:static v8::MaybeLocal<v8::Object> node::crypto::KeyObjectHandle::Create(node::Environment*, std::shared_ptr<node::crypto::KeyObjectData>): Assertion `!env->crypto_key_object_handle_constructor().IsEmpty()' failed.
 1: 0x55a2986f0c80 node::Abort() [./node]
 2: 0x55a2986f0d14  [./node]
 3: 0x55a2988acc3c node::crypto::KeyObjectHandle::Create(node::Environment*, std::shared_ptr<node::crypto::KeyObjectData>) [./node]
 4: 0x55a2988aee01 node::crypto::NativeKeyObject::KeyObjectTransferData::Deserialize(node::Environment*, v8::Local<v8::Context>, std::unique_ptr<node::worker::TransferData, std::default_delete<node::worker::TransferData> >) [./node]
 5: 0x55a29873b130 node::worker::Message::Deserialize(node::Environment*, v8::Local<v8::Context>) [./node]
 6: 0x55a298740335 node::worker::MessagePort::ReceiveMessage(v8::Local<v8::Context>, bool) [./node]
 7: 0x55a2987408b8 node::worker::MessagePort::OnMessage() [./node]
 8: 0x55a299234736  [./node]
 9: 0x55a299248df4  [./node]
10: 0x55a29923506a uv_run [./node]
11: 0x55a2987c8336 node::worker::Worker::Run() [./node]
12: 0x55a2987c8707  [./node]
13: 0x7fba8b6af6db  [/lib/x86_64-linux-gnu/libpthread.so.0]
14: 0x7fba8b3d1a3f clone [/lib/x86_64-linux-gnu/libc.so.6]
Aborted (core dumped)

@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. worker Issues and PRs related to Worker support. labels Sep 18, 2020
@addaleax
Copy link
Member

We could probably make KeyObjectHandle::Initialize() in node_crypto.cc do something like we do in GetMessagePortConstructorTemplate() in node_messaging.cc, where we create the FunctionTemplate only once, and then afterwards return the cached value.

@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. good first issue Issues that are suitable for first-time contributors. mentor-available labels Sep 20, 2020
@addaleax
Copy link
Member

I’ve marked this good first issue, but if you want to pick this up, you’ll probably want to know some C++ and maybe also be a bit familiar with the V8 API (if you are not, https://github.com/nodejs/node/blob/master/src/README.md has some overview over the concepts involved here).

@jasnell
Copy link
Member Author

jasnell commented Sep 20, 2020

I was going to work up the fix this next week as part of #35093 but if someone wants to pick it up, feel free!

@ThakurKarthik
Copy link
Contributor

ThakurKarthik commented Sep 20, 2020

@addaleax @jasnell I want to pick it up. As a starter how much c++ should i know be knowing to tackle this issue?

Edit: I am going through the relevant details from https://github.com/nodejs/node/blob/master/src/README.md and i will try to work on it.

Thanks

@addaleax
Copy link
Member

@ThakurKarthik I don’t think it requires a deep knowledge of the language, but some basic understanding would be helpful.

Fwiw, the relevant parts of the code base are:

node/src/node_crypto.cc

Lines 3268 to 3288 in a8971f8

Local<Function> KeyObjectHandle::Initialize(Environment* env,
Local<Object> target) {
Local<FunctionTemplate> t = env->NewFunctionTemplate(New);
t->InstanceTemplate()->SetInternalFieldCount(
KeyObjectHandle::kInternalFieldCount);
t->Inherit(BaseObject::GetConstructorTemplate(env));
env->SetProtoMethod(t, "init", Init);
env->SetProtoMethodNoSideEffect(t, "getSymmetricKeySize",
GetSymmetricKeySize);
env->SetProtoMethodNoSideEffect(t, "getAsymmetricKeyType",
GetAsymmetricKeyType);
env->SetProtoMethod(t, "export", Export);
auto function = t->GetFunction(env->context()).ToLocalChecked();
target->Set(env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "KeyObjectHandle"),
function).Check();
return function;
}

node/src/node_crypto.cc

Lines 6953 to 6954 in a8971f8

env->set_crypto_key_object_handle_constructor(
KeyObjectHandle::Initialize(env, target));

And this is how we’ve solved this for the MessagePort case, another situation where the same problem occurred:

node/src/node_messaging.cc

Lines 1076 to 1094 in a8971f8

Local<FunctionTemplate> GetMessagePortConstructorTemplate(Environment* env) {
// Factor generating the MessagePort JS constructor into its own piece
// of code, because it is needed early on in the child environment setup.
Local<FunctionTemplate> templ = env->message_port_constructor_template();
if (!templ.IsEmpty())
return templ;
{
Local<FunctionTemplate> m = env->NewFunctionTemplate(MessagePort::New);
m->SetClassName(env->message_port_constructor_string());
m->InstanceTemplate()->SetInternalFieldCount(
MessagePort::kInternalFieldCount);
m->Inherit(HandleWrap::GetConstructorTemplate(env));
env->SetProtoMethod(m, "postMessage", MessagePort::PostMessage);
env->SetProtoMethod(m, "start", MessagePort::Start);
env->set_message_port_constructor_template(m);
}

@ThakurKarthik
Copy link
Contributor

Thanks @addaleax for the pointers

@ThakurKarthik
Copy link
Contributor

Hi @addaleax with your reference I made these changes and ran ./configure && make -j4 test it was successful .
But running this code still showing error.

./out/Release/node test.js
Hello world 123 true
Segmentation fault (core dumped)

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 05da3af09c..8ef2adebeb 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -3265,8 +3265,11 @@ size_t KeyObjectData::GetSymmetricKeySize() const {
   return symmetric_key_len_;
 }
 
-Local<Function> KeyObjectHandle::Initialize(Environment* env,
-                                            Local<Object> target) {
+Local<Function> KeyObjectHandle::Initialize(Environment* env) {
+  Local<Function> templ = env->crypto_key_object_handle_constructor();
+  if (!templ.IsEmpty()) {
+    return templ;
+  }
   Local<FunctionTemplate> t = env->NewFunctionTemplate(New);
   t->InstanceTemplate()->SetInternalFieldCount(
       KeyObjectHandle::kInternalFieldCount);
@@ -3280,20 +3283,16 @@ Local<Function> KeyObjectHandle::Initialize(Environment* env,
   env->SetProtoMethod(t, "export", Export);
 
   auto function = t->GetFunction(env->context()).ToLocalChecked();
-  target->Set(env->context(),
-              FIXED_ONE_BYTE_STRING(env->isolate(), "KeyObjectHandle"),
-              function).Check();
-
-  return function;
+  env->set_crypto_key_object_handle_constructor(function);
+  return KeyObjectHandle::Initialize(env);
 }
 
 MaybeLocal<Object> KeyObjectHandle::Create(
     Environment* env,
     std::shared_ptr<KeyObjectData> data) {
   Local<Object> obj;
-  if (!env->crypto_key_object_handle_constructor()
-           ->NewInstance(env->context(), 0, nullptr)
-           .ToLocal(&obj)) {
+  Local<Function> fctun = KeyObjectHandle::Initialize(env);
+  if (!fctun->NewInstance(env->context(), 0, nullptr).ToLocal(&obj)) {
     return MaybeLocal<Object>();
   }
 
@@ -6950,8 +6949,9 @@ void Initialize(Local<Object> target,
 
   Environment* env = Environment::GetCurrent(context);
   SecureContext::Initialize(env, target);
-  env->set_crypto_key_object_handle_constructor(
-      KeyObjectHandle::Initialize(env, target));
+  target->Set(env->context(),
+            FIXED_ONE_BYTE_STRING(env->isolate(), "KeyObjectHandle"),
+            KeyObjectHandle::Initialize(env)).Check();
   env->SetMethod(target, "createNativeKeyObjectClass",
                  CreateNativeKeyObjectClass);
   CipherBase::Initialize(env, target);
diff --git a/src/node_crypto.h b/src/node_crypto.h
index 63468e4f18..8f78103560 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -447,8 +447,7 @@ class KeyObjectData {
 
 class KeyObjectHandle : public BaseObject {
  public:
-  static v8::Local<v8::Function> Initialize(Environment* env,
-                                            v8::Local<v8::Object> target);
+  static v8::Local<v8::Function> Initialize(Environment* env);
 
   static v8::MaybeLocal<v8::Object> Create(Environment* env,
                                            std::shared_ptr<KeyObjectData> data);

For some reason i am not able to generate the debug build.

./configure --debug
make -j4

Can you guide me how to proceed further ?
Thanks

@addaleax
Copy link
Member

@ThakurKarthik

For some reason i am not able to generate the debug build.

It’s hard to tell without further information, but the only reason that I know of why this might not work is that debug builds take up a lot more memory and disk space. In this case, using ./configure --debug-node instead of ./configure --debug might be more helpful anyway, because you only want to debug the Node.js part, not dependencies like V8 (I assume).

@ThakurKarthik
Copy link
Contributor

Hi @addaleax I was able to generate the debug build with --debug-node and used gdb, the ouput is similar to what @jasnell has posted earlier. I tried debugging step by step
I can see env->crypto_key_object_handle_constructor() is defined and giving some value when printed but i think this is
key_ctor = env->crypto_key_object_secret_constructor(); undefined. It is set inside this function CreateNativeKeyObjectClass. So maybe i need to call this inside Deserialize method.
You can see the different variables i printed in this screenshot
Screenshot from 2020-09-27 20-27-30

@addaleax
Copy link
Member

@ThakurKarthik Oh, right, that’s … unfortunate, sorry

It is set inside this function CreateNativeKeyObjectClass. So maybe i need to call this inside Deserialize method.

That’s probably the best approach here – the function is called from lib/internal/crypto/keys.js. I think you should be able to load that via env->native_module_require()->Call(context, Null(isolate), 1, &arg)) where arg is FIXED_ONE_BYTE_STRING(isolate, "internal/crypto/keys"), maybe you could try that? (It’s basically the C++ equivalent of writing require('internal/crypto/keys'))

ThakurKarthik added a commit to ThakurKarthik/node that referenced this issue Sep 30, 2020
danielleadams pushed a commit that referenced this issue Oct 6, 2020
Fixes: #35263

PR-URL: #35416
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 3, 2020
Fixes: #35263

PR-URL: #35416
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 16, 2020
Fixes: #35263

PR-URL: #35416
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this issue Jan 8, 2021
Fixes: nodejs#35263

PR-URL: nodejs#35416
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. good first issue Issues that are suitable for first-time contributors. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants