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

crypto function call unstable behaviour, node crushes #19655

Closed
eduardbme opened this issue Mar 28, 2018 · 9 comments
Closed

crypto function call unstable behaviour, node crushes #19655

eduardbme opened this issue Mar 28, 2018 · 9 comments
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@eduardbme
Copy link
Contributor

  • Version: v9.6.1+
  • Platform: unix
  • Subsystem: crypto

Node behavior is unstable during processing crypto function (listed at the bottom).
Have ran this code three times via lldb and each time I have different output.

MacBook-Pro:enigma admin$ lldb /Users/admin/Documents/opensource/node/out/Debug/node test
(lldb) target create "/Users/admin/Documents/opensource/node/out/Debug/node"
Current executable set to '/Users/admin/Documents/opensource/node/out/Debug/node' (x86_64).
(lldb) settings set -- target.run-args "test"
(lldb) r
Process 64817 launched: '/Users/admin/Documents/opensource/node/out/Debug/node' (x86_64)
Process 64817 stopped

  • thread deps: update openssl to 1.0.1j #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x0000000100f53937 node`v8::internal::LocalArrayBufferTracker::Add(v8::internal::JSArrayBuffer*, unsigned long) [inlined] std::__1::pair<std::__1::__hash_iterator<std::__1::__hash_node<v8::internal::JSArrayBuffer*, void*>>, bool> std::__1::__hash_table<v8::internal::JSArrayBuffer, v8::internal::LocalArrayBufferTracker::Hasher, std::__1::equal_tov8::internal::JSArrayBuffer*, std::__1::allocatorv8::internal::JSArrayBuffer* >::__emplace_unique_key_args<v8::internal::JSArrayBuffer*, v8::internal::JSArrayBuffer* const&>(this=0x0000000105806ab8, __k=0x00007fff5fbfce48, __args=0x00007fff5fbfce48) at __hash_table:1963
    1960 __nd = _bucket_list[__chash];
    1961 if (__nd != nullptr)
    1962 {
    -> 1963 for (__nd = __nd->_next; __nd != nullptr &&
    1964 (__nd->__hash() == __hash || __constrain_hash(__nd->__hash(), __bc) == __chash);
    1965 __nd = __nd->_next)
    1966 {
    Target 0: (node) stopped.
    (lldb) r
    There is a running process, kill it and restart?: [Y/n]
    Process 64817 exited with status = 9 (0x00000009)
    Process 64821 launched: '/Users/admin/Documents/opensource/node/out/Debug/node' (x86_64)
    internal/crypto/cipher.js:110
    const ret = this._handle.update(data, inputEncoding);
    ^

Error: Trying to add data in unsupported state
at Decipheriv.update (internal/crypto/cipher.js:110:28)
at testCipher5 (/Users/admin/Documents/work/enigma/test.js:139:24)
at Object. (/Users/admin/Documents/work/enigma/test.js:145:3)
at Module._compile (module.js:666:30)
at Object.Module._extensions..js (module.js:677:10)
at Module.load (module.js:577:32)
at tryModuleLoad (module.js:517:12)
at Function.Module._load (module.js:509:3)
at Function.Module.runMain (module.js:707:10)
at startup (bootstrap_node.js:196:16)
Process 64821 exited with status = 1 (0x00000001)
(lldb) r
Process 64825 launched: '/Users/admin/Documents/opensource/node/out/Debug/node' (x86_64)
node(64825,0x7fffa8d573c0) malloc: *** error for object 0x10581ea30: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
Process 64825 stopped

  • thread deps: update openssl to 1.0.1j #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
    frame #0: 0x00007fff9ff69d42 libsystem_kernel.dylib__pthread_kill + 10 libsystem_kernel.dylib__pthread_kill:
    -> 0x7fff9ff69d42 <+10>: jae 0x7fff9ff69d4c ; <+20>
    0x7fff9ff69d44 <+12>: movq %rax, %rdi
    0x7fff9ff69d47 <+15>: jmp 0x7fff9ff62caf ; cerror_nocancel
    0x7fff9ff69d4c <+20>: retq
    Target 0: (node) stopped.
function testCipher(key, iv) {
    // Test encryption and decryption with explicit key with aes128-wrap
    const plaintext =
        '32|RmVZZkFUVmpRRkp0TmJaUm56ZU9qcnJkaXNNWVNpTTU*|iXmckfRWZBGWWELw' +
        'eCBsThSsfUHLeRe0KCsK8ooHgxie0zOINpXxfZi/oNG7uq9JWFVCk70gfzQH8ZUJ' +
        'jAfaFg**';
    const cipher = crypto.createCipheriv('id-smime-alg-CMS3DESwrap', key, iv);
    let ciph = cipher.update(plaintext, 'utf8', 'buffer');
    ciph = Buffer.concat([ciph, cipher.final('buffer')]);

    const decipher = crypto.createDecipheriv('id-smime-alg-CMS3DESwrap', key, iv);
    let txt = decipher.update(ciph, 'buffer', 'utf8');
    txt += decipher.final('utf8');

    console.log(txt);
}

testCipher(crypto.randomBytes(24), crypto.randomBytes(0));
@mscdex
Copy link
Contributor

mscdex commented Mar 28, 2018

I can't reproduce this on Linux. I get the 'Trying to add data in unsupported state' error every time.

@bnoordhuis bnoordhuis added crypto Issues and PRs related to the crypto subsystem. v9.x labels Mar 28, 2018
@bnoordhuis
Copy link
Member

@eduardbcom Can you test with the latest v9.x release? I've fixed several bugs in the last months that might cause that.

@eduardbme
Copy link
Contributor Author

MacBook-Pro:enigma admin$ ~/Documents/opensource/node/out/Debug/node -v
v10.0.0-pre
MacBook-Pro:enigma admin$ ~/Documents/opensource/node/out/Debug/node test
node(18805,0x7fffa8d573c0) malloc: *** error for object 0x105b075a0: incorrect checksum for freed object - object was probably modified after being freed.
*** set a breakpoint in malloc_error_break to debug
Abort trap: 6
MacBook-Pro:enigma admin$ ~/Documents/opensource/node/out/Debug/node test
internal/crypto/cipher.js:119
const ret = this._handle.update(data, inputEncoding);
^

Error: Trying to add data in unsupported state
at Decipheriv.update (internal/crypto/cipher.js:119:28)
at testCipher5 (/Users/admin/Documents/work/enigma/test.js:139:24)
at Object. (/Users/admin/Documents/work/enigma/test.js:145:3)
at Module._compile (internal/modules/cjs/loader.js:678:30)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
at Module.load (internal/modules/cjs/loader.js:589:32)
at tryModuleLoad (internal/modules/cjs/loader.js:528:12)
at Function.Module._load (internal/modules/cjs/loader.js:520:3)
at Function.Module.runMain (internal/modules/cjs/loader.js:719:10)
at startup (internal/bootstrap/node.js:225:19)

Pulled latest master. Does it contain these changes ? @bnoordhuis

@eduardbme
Copy link
Contributor Author

eduardbme commented Mar 28, 2018

BTW, does node support this algorithm ?

No idea why I receive 'Error: Trying to add data in unsupported state.'

In case of createCipher/createDecipher (without IV) receives Error: error:0607B0AA:digital envelope routines:EVP_CipherInit_ex:wrap mode not allowed.

Thanks!

@TimothyGu TimothyGu removed the v9.x label Mar 28, 2018
@TimothyGu
Copy link
Member

It seems like on Linux no errors are emitted, but Valgrind shows some invalid reads/writes:

==4830== Invalid write of size 8
==4830==    at 0x11884EA: des_ede3_wrap_cipher (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x118B330: EVP_EncryptUpdate (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DCD89: node::crypto::CipherBase::Update(char const*, int, unsigned char**, int*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DC07C: node::crypto::CipherBase::Update(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x9C9A84: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA2490B: 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) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA24015: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x25101060427C: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==  Address 0x93509a0 is 0 bytes after a block of size 144 alloc'd
==4830==    at 0x4C2BADF: malloc (vg_replace_malloc.c:298)
==4830==    by 0x4C2DE5F: realloc (vg_replace_malloc.c:785)
==4830==    by 0x8DCD67: node::crypto::CipherBase::Update(char const*, int, unsigned char**, int*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DC07C: node::crypto::CipherBase::Update(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x9C9A84: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA2490B: 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) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA24015: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x25101060427C: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???

...
 
==4830== Invalid read of size 4
==4830==    at 0x1234470: DES_ede3_cbc_encrypt (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x1188791: des_ede3_wrap_cipher (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x118B330: EVP_EncryptUpdate (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DCD89: node::crypto::CipherBase::Update(char const*, int, unsigned char**, int*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DC07C: node::crypto::CipherBase::Update(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x9C9A84: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA2490B: 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) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA24015: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x25101060427C: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==  Address 0x93509a0 is 0 bytes after a block of size 144 alloc'd
==4830==    at 0x4C2BADF: malloc (vg_replace_malloc.c:298)
==4830==    by 0x4C2DE5F: realloc (vg_replace_malloc.c:785)
==4830==    by 0x8DCD67: node::crypto::CipherBase::Update(char const*, int, unsigned char**, int*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DC07C: node::crypto::CipherBase::Update(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x9C9A84: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA2490B: 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) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA24015: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x25101060427C: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???

...

==4830== Invalid write of size 1
==4830==    at 0x123449C: DES_ede3_cbc_encrypt (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x1188791: des_ede3_wrap_cipher (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x118B330: EVP_EncryptUpdate (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DCD89: node::crypto::CipherBase::Update(char const*, int, unsigned char**, int*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DC07C: node::crypto::CipherBase::Update(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x9C9A84: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA2490B: 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) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA24015: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x25101060427C: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==  Address 0x93509a0 is 0 bytes after a block of size 144 alloc'd
==4830==    at 0x4C2BADF: malloc (vg_replace_malloc.c:298)
==4830==    by 0x4C2DE5F: realloc (vg_replace_malloc.c:785)
==4830==    by 0x8DCD67: node::crypto::CipherBase::Update(char const*, int, unsigned char**, int*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x8DC07C: node::crypto::CipherBase::Update(v8::FunctionCallbackInfo<v8::Value> const&) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x9C9A84: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA2490B: 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) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0xA24015: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (in /home/timothy-gu/dev/node/node/out/Release/node)
==4830==    by 0x25101060427C: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???
==4830==    by 0x251010613616: ???

@yhwang
Copy link
Member

yhwang commented Apr 3, 2018

@eduardbcom

BTW, does node support this algorithm ?

Assuming that you build the node.js with the deps/openssl, then it should support id-smime-alg-CMS3DESwrap

I tried to run your script in my ubuntu 16.04 env, I found it would work if you skip cipher.final() and decipher.final(). I traced the openssl where it handles the id-smime-alg-CMS3DESwrap, the return value of the cipher.final() seems invalid. I know for cipher and decipher, supposedly, we should call final() as the last operation. However, I don't know why it's not true for id-smime-alg-CMS3DESwrap.

You can verify my finding in your env. If it works on your env, then we can discuss how to handle the decipher.final() and cipher.final() in this case.

         'jAfaFg**';
     const cipher = crypto.createCipheriv('id-smime-alg-CMS3DESwrap', key, iv);
     let ciph = cipher.update(plaintext, 'utf8', 'buffer');
-    ciph = Buffer.concat([ciph, cipher.final('buffer')]);
 
     const decipher = crypto.createDecipheriv('id-smime-alg-CMS3DESwrap', key, iv);
     let txt = decipher.update(ciph, 'buffer', 'utf8');
-    txt += decipher.final('utf8');
 
     console.log(txt);
 }

@TimothyGu
Copy link
Member

@yhwang I'm seeing invalid memory access even with the following reduced test case:

const crypto = require('crypto');

function testCipher(key, iv) {
    // Test encryption and decryption with explicit key with aes128-wrap
    const plaintext =
        '32|RmVZZkFUVmpRRkp0TmJaUm56ZU9qcnJkaXNNWVNpTTU*|iXmckfRWZBGWWELw' +
        'eCBsThSsfUHLeRe0KCsK8ooHgxie0zOINpXxfZi/oNG7uq9JWFVCk70gfzQH8ZUJ' +
        'jAfaFg**';
    const cipher = crypto.createCipheriv('id-smime-alg-CMS3DESwrap', key, iv);
    let ciph = cipher.update(plaintext, 'utf8', 'buffer');
    // ciph = Buffer.concat([ciph, cipher.final('buffer')]);

    // const decipher = crypto.createDecipheriv('id-smime-alg-CMS3DESwrap', key, iv);
    // let txt = decipher.update(ciph, 'buffer', 'utf8');
    // txt += decipher.final('utf8');
    //
    // console.log(txt);
}

testCipher(Buffer.from('0123'.repeat(6)), Buffer.alloc(0));

@yhwang
Copy link
Member

yhwang commented Apr 18, 2018

@TimothyGu sorry for the late response. I will try to investigate the invalid memory access.

@yhwang
Copy link
Member

yhwang commented Apr 21, 2018

@TimothyGu finally, I have some spared time to look at this issue today. I guess I found the root cause of the invalid read. In this case, the output buff that we allocated for EVP_CipherUpdate() is not enough. Let me try to find a proper number to add up when allocating the output buff.

MylesBorins pushed a commit that referenced this issue May 4, 2018
For key wrapping algorithms, calling EVP_CipherUpdate() with null output
could obtain the size for the ciphertext. Then use the returned size to
allocate output buffer. Also add a test case to verify des3-wrap.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>

PR-URL: #20370
Fixes: #19655
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 8, 2018
For key wrapping algorithms, calling EVP_CipherUpdate() with null output
could obtain the size for the ciphertext. Then use the returned size to
allocate output buffer. Also add a test case to verify des3-wrap.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>

PR-URL: #20370
Fixes: #19655
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
tniessen pushed a commit to tniessen/node that referenced this issue May 13, 2018
For key wrapping algorithms, calling EVP_CipherUpdate() with null output
could obtain the size for the ciphertext. Then use the returned size to
allocate output buffer. Also add a test case to verify des3-wrap.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>

PR-URL: nodejs#20370
Fixes: nodejs#19655
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue May 14, 2018
For key wrapping algorithms, calling EVP_CipherUpdate() with null output
could obtain the size for the ciphertext. Then use the returned size to
allocate output buffer. Also add a test case to verify des3-wrap.

Signed-off-by: Yihong Wang <yh.wang@ibm.com>
Backport-PR-URL: #20706
PR-URL: #20370
Fixes: #19655
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants