Skip to content

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

Closed
#45769
Closed
crypto: cipher update process crash with input of max int32 length#45757
#45769
Labels
confirmed-bugIssues and PRs for confirmed bugs.cryptoIssues and PRs related to the crypto subsystem.

Description

@LongTengDao
LongTengDao
opened on 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

Activity

  1. panva
    added
    cryptoIssues and PRs related to the crypto subsystem.
    on Dec 6, 2022

  2. panva commented on Dec 6, 2022

    @panva
    panva
    on Dec 6, 2022
    Member

    cc @nodejs/crypto

  3. panva
    changed the title [-]crypto cause process crash with magic data length[/-] [+]crypto: cipher update process crash with input of max int32 length[/+] on Dec 6, 2022
  4. panva
    added
    confirmed-bugIssues and PRs for confirmed bugs.
    on Dec 6, 2022

  5. panva commented on Dec 6, 2022

    @panva
    panva
    on Dec 6, 2022
    Member
    #
# 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

  6. marco-ippolito commented on Dec 7, 2022

    @marco-ippolito
    marco-ippolito
    on Dec 7, 2022
    Member

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

  7. bnoordhuis commented on Dec 7, 2022

    @bnoordhuis
    bnoordhuis
    on Dec 7, 2022
    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.

  8. bnoordhuis commented on Dec 7, 2022

    @bnoordhuis
    bnoordhuis
    on Dec 7, 2022
    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.

  9. panva commented on Dec 7, 2022

    @panva
    panva
    on Dec 7, 2022
    Member

    @marco-ippolito absolutely, go for it.

  10. nodejs-github-bot
    closed this as completedin #45769on Dec 9, 2022
  11. nodejs-github-bot
    added a commit that references this issue on Dec 9, 2022

    crypto: fix CipherBase Update int32 overflow

    5fe0795
  12. targos
    added 2 commits that reference this issue on Dec 12, 2022

    crypto: fix CipherBase Update int32 overflow

    556b270

    crypto: fix CipherBase Update int32 overflow

    2e4d37e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.cryptoIssues and PRs related to the crypto subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions