safely handle openssl realloc failures during resize

[prabhu]

NOTE: qwen3-coder model was used to develop this patch.

While troubleshooting a recent out-of-memory issue, I arrived at the ncrypto resize logic. I realised that the method looked too simplistic, so I began looking for the correct and safe use of OPENSSL_realloc api.

qwen3 was used to confirm the suspicion and generate this patch. I am not familiar with writing unit tests or testing this flow, so some guidance will be much appreciated.

Testing:

• Tests on my mac were failing due to some error, so this patch wasn't tested locally.

Path: parallel/test-cluster-dgram-1
Mismatched <anonymous> function calls. Expected exactly 10, actual 0.
Mismatched <anonymous> function calls. Expected exactly 10, actual 0.
    at Proxy.mustCall (/Users/prabhu/ent_work/node/test/common/index.js:395:10)
    at worker (/Users/prabhu/ent_work/node/test/parallel/test-cluster-dgram-1.js:100:31)
    at Object.<anonymous> (/Users/prabhu/ent_work/node/test/parallel/test-cluster-dgram-1.js:37:3)
    at Module._compile (node:internal/modules/cjs/loader:1722:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1854:10)
    at Module.load (node:internal/modules/cjs/loader:1454:32)
    at Module._load (node:internal/modules/cjs/loader:1274:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:236:24)
Mismatched <anonymous> function calls. Expected exactly 10, actual 0.
Mismatched <anonymous> function calls. Expected exactly 10, actual 0.
    at Proxy.mustCall (/Users/prabhu/ent_work/node/test/common/index.js:395:10)
    at worker (/Users/prabhu/ent_work/node/test/parallel/test-cluster-dgram-1.js:100:31)
    at Object.<anonymous> (/Users/prabhu/ent_work/node/test/parallel/test-cluster-dgram-1.js:37:3)
    at Module._compile (node:internal/modules/cjs/loader:1722:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1854:10)
    at Module.load (node:internal/modules/cjs/loader:1454:32)
    at Module._load (node:internal/modules/cjs/loader:1274:12)
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:236:24)
Command: out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/prabhu/ent_work/node/test/parallel/test-cluster-dgram-1.js
--- TIMEOUT ---


[06:04|% 100|+ 4541|-   1]: Done

Failed tests:
out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/prabhu/ent_work/node/test/parallel/test-cluster-dgram-1.js
make[1]: *** [jstest] Error 1
make: *** [test] Error 2

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/security-wg

[bnoordhuis]

This should not land as-is but it did highlight a real (albeit hopefully academic) bug in the code.

[bnoordhuis]

This reallocation cannot realistically fail because of line 213: actual_len is min(len, len_), i.e., it always shrinks, never grows the buffer.

I'd have suggested adding a CHECK_NOT_NULL(buf.data) sanity check but because this is code that's been moved from src/ to deps/ncrypto, you can't use that macro 🤦

[bnoordhuis]

IMO this secure_ flag is problematic with resizes because secure memory comes from a different heap and should not be mixed with OPENSSL_realloc.

I believe it's a theoretical problem right now because secure buffers are never resized but it should either be a runtime error or a fatal error.

[prabhu]

Thank you so much for the review. I agree that the heaps should not be mixed. However, the current implementation isn't explicit enough about secure_ leading to uncertainty and theoretical bugs (Avoiding some specific wordings). I have tried to improve the method to make things a bit explicit (false and explicit check).

Question: I might have overlooked compatibility with BoringSSL. I noticed that OPENSSL_secure_clear_free is really a wrapper for OPENSSL_clear_free, so the previous patch might have worked too. I would appreciate your help with testing this across crypto implementations as well, since my mac is not really setup for node development and testing.

DataPointer DataPointer::resize(size_t len) {
  NCRYPTO_ASSERT_TRUE(!secure_);
  size_t actual_len = std::min(len_, len);
  auto buf = release();
  // Handle the unexpected null pointer
  if (buf.data == nullptr) {
    return DataPointer(nullptr, 0, false);
  }
  // This scenario is unlikely
  if (actual_len == len_) {
    return DataPointer(buf.data, actual_len, false);
  }
  // This should correctly handle secure memory based on input pointer
  void* new_data = OPENSSL_realloc(buf.data, actual_len);
  // Handle reallocation failure. NULL will be returned on error.
  // https://www.manpagez.com/man/3/OPENSSL_realloc/
  if (new_data == nullptr && actual_len > 0) {
    // OPENSSL_realloc doesn't free automatically. Below is a code snippet showing correct use
    // https://github.com/openssl/openssl/blob/e7d5398aa1349cc575a5b80e0d6eb28e61cb4bfa/apps/engine.c#L72
    OPENSSL_clear_free(buf.data, len_);
    return DataPointer(nullptr, 0, false);
  }
  return DataPointer(new_data, actual_len, false);
}

Kindly let me know if I can update and push my branch with this.

[bnoordhuis]

Perhaps the simplest thing to do is to add this as the first line of resize:

if (secure_) return DataPointer();

[prabhu]

@bnoordhuis updated and pushed the branch based on this review. Kindly review the new commit. I personally prefer the assert NCRYPTO_ASSERT_TRUE(!secure_); over silently failing with return DataPointer(). NCRYPTO_ASSERT_TRUE is used in other places in the code.

[prabhu]

@bnoordhuis any thoughts?