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: cipher update process crash with input of max int32 length #45757

Closed
LongTengDao opened this issue Dec 6, 2022 · 6 comments · Fixed by #45769
Closed

crypto: cipher update process crash with input of max int32 length #45757

LongTengDao opened this issue Dec 6, 2022 · 6 comments · Fixed by #45769
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.

Comments

@LongTengDao
Copy link
Contributor

LongTengDao commented Dec 6, 2022

Version

v18.12.0

Platform

No response

Subsystem

crypto

What steps will reproduce the bug?

try {
    require('crypto')
    .createCipheriv('aes-128-gcm', Buffer.alloc(16), Buffer.alloc(12))
    .update(Buffer.allocUnsafeSlow(2**31-1));
}
catch (error) {
    console.error(error);
}

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior?

Throw a normal catchable error like size>=2**31 do, or work right like size<=2**31.

What do you see instead?

Only size===2**31-1 will cause an uncatchable process crash.

Additional information

No response

@panva panva added the crypto Issues and PRs related to the crypto subsystem. label Dec 6, 2022
@panva
Copy link
Member

panva commented Dec 6, 2022

cc @nodejs/crypto

@panva panva changed the title crypto cause process crash with magic data length crypto: cipher update process crash with input of max int32 length Dec 6, 2022
@panva panva added the confirmed-bug Issues with confirmed bugs. label Dec 6, 2022
@panva
Copy link
Member

panva commented Dec 6, 2022

#
# Fatal error in , line 0
# Check failed: byte_length <= i::JSArrayBuffer::kMaxByteLength.
#
#
#
#FailureMessage Object: 0x16d365b68
 1: 0x102baf630 node::NodePlatform::GetStackTracePrinter()::$_3::__invoke()
 2: 0x103ac31dc V8_Fatal(char const*, ...)
 3: 0x102cc068c v8::ArrayBuffer::NewBackingStore(v8::Isolate*, unsigned long)
 4: 0x102c3ff8c node::crypto::CipherBase::Update(char const*, unsigned long, std::__1::unique_ptr<v8::BackingStore, std::__1::default_delete<v8::BackingStore> >*)
 5: 0x102c406d0 node::crypto::CipherBase::Update(v8::FunctionCallbackInfo<v8::Value> const&)::$_0::__invoke(node::crypto::CipherBase*, v8::FunctionCallbackInfo<v8::Value> const&, char const*, unsigned long)
 6: 0x102c3864c void node::crypto::Decode<node::crypto::Verify>(v8::FunctionCallbackInfo<v8::Value> const&, void (*)(node::crypto::Verify*, v8::FunctionCallbackInfo<v8::Value> const&, char const*, unsigned long))
 7: 0x102d0920c v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo)
 8: 0x102d08d08 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)
 9: 0x102d08534 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*)
10: 0x1034f918c Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit
11: 0x103484198 Builtins_InterpreterEntryTrampoline
12: 0x103484198 Builtins_InterpreterEntryTrampoline
13: 0x103484198 Builtins_InterpreterEntryTrampoline
14: 0x103484198 Builtins_InterpreterEntryTrampoline
15: 0x103484198 Builtins_InterpreterEntryTrampoline
16: 0x103484198 Builtins_InterpreterEntryTrampoline
17: 0x103484198 Builtins_InterpreterEntryTrampoline
18: 0x103484198 Builtins_InterpreterEntryTrampoline
19: 0x1034824d0 Builtins_JSEntryTrampoline
20: 0x103482164 Builtins_JSEntry
21: 0x102dc4eac v8::internal::(anonymous namespace)::Invoke(v8::internal::Isolate*, v8::internal::(anonymous namespace)::InvokeParams const&)
22: 0x102dc43e0 v8::internal::Execution::Call(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Object>, v8::internal::Handle<v8::internal::Object>, int, v8::internal::Handle<v8::internal::Object>*)
23: 0x102cb4914 v8::Function::Call(v8::Local<v8::Context>, v8::Local<v8::Value>, int, v8::Local<v8::Value>*)
24: 0x102b1d204 node::ExecuteBootstrapper(node::Environment*, char const*, std::__1::vector<v8::Local<v8::Value>, std::__1::allocator<v8::Local<v8::Value> > >*)
25: 0x102b1e074 node::StartExecution(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>)
26: 0x102aa0420 node::LoadEnvironment(node::Environment*, std::__1::function<v8::MaybeLocal<v8::Value> (node::StartExecutionCallbackInfo const&)>)
27: 0x102b8d0f8 node::NodeMainInstance::Run()
28: 0x102b20ec8 node::LoadSnapshotDataAndRun(node::SnapshotData const**, node::InitializationResult const*)
29: 0x102b21190 node::Start(int, char**)
30: 0x18e6efe50 start [/usr/lib/dyld]
[1]    17989 trace trap  node some.cjs

@marco-ippolito
Copy link
Member

marco-ippolito commented Dec 7, 2022

I'll work on it. If need help can I bother you @panva ?

@bnoordhuis
Copy link
Member

It's a signed integer overflow, caused by openssl using ints for sizes and node mixing int and size_t (and not being diligent enough about overflow checking.)

This particular issue isn't that hard to fix but it's probably just one of many similar bugs lurking in src/crypto.

@bnoordhuis
Copy link
Member

Here is a quick fix:

diff --git a/src/crypto/crypto_cipher.cc b/src/crypto/crypto_cipher.cc
index b907e9e9cdc..2259e28bec8 100644
--- a/src/crypto/crypto_cipher.cc
+++ b/src/crypto/crypto_cipher.cc
@@ -803,7 +803,11 @@ CipherBase::UpdateResult CipherBase::Update(
   if (kind_ == kDecipher && IsAuthenticatedMode())
     CHECK(MaybePassAuthTagToOpenSSL());
 
-  int buf_len = len + EVP_CIPHER_CTX_block_size(ctx_.get());
+  const int block_size = EVP_CIPHER_CTX_block_size(ctx_.get());
+  CHECK_GT(block_size, 0);
+  if (len + block_size > INT_MAX) return kErrorState;
+
+  int buf_len = len + block_size;
   // For key wrapping algorithms, get output size by calling
   // EVP_CipherUpdate() with null output.
   if (kind_ == kCipher && mode == EVP_CIPH_WRAP_MODE &&

Note: len + block_size is not UB because block_size gets converted to size_t since len is a size_t.

@panva
Copy link
Member

panva commented Dec 7, 2022

@marco-ippolito absolutely, go for it.

nodejs-github-bot pushed a commit that referenced this issue Dec 9, 2022
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
ErickWendel pushed a commit to ErickWendel/node that referenced this issue Dec 12, 2022
PR-URL: nodejs#45769
Fixes: nodejs#45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos pushed a commit that referenced this issue Dec 12, 2022
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos pushed a commit that referenced this issue Dec 13, 2022
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this issue Jan 4, 2023
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this issue Jan 5, 2023
PR-URL: #45769
Fixes: #45757
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants