From 10b21e5685acc9f50df646c2ff47971eb6cd320e Mon Sep 17 00:00:00 2001 From: GauriSpears Date: Thu, 18 May 2023 21:28:44 +0500 Subject: [PATCH] crypto: use openssl's own memory BIOs in crypto_context.cc NodeBIO's memory buffer structure does not support BIO_C_FILE_SEEK and B IO_C_FILE_TELL. This prevents OpenSSL PEM_read_bio_PrivateKey from readi ng some private keys. So I switched to OpenSSL'w own protected memory bu ffers. Fixes: https://github.com/nodejs/node/issues/47008 PR-URL: https://github.com/nodejs/node/pull/47160 Reviewed-By: James M Snell Reviewed-By: Ben Noordhuis --- node.gyp | 1 + src/crypto/crypto_context.cc | 20 +++++++++---------- test/cctest/test_node_crypto_env.cc | 31 +++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+), 11 deletions(-) create mode 100644 test/cctest/test_node_crypto_env.cc diff --git a/node.gyp b/node.gyp index f9621fc1e15470..4562d23c728dc6 100644 --- a/node.gyp +++ b/node.gyp @@ -1054,6 +1054,7 @@ 'sources': [ 'test/cctest/test_crypto_clienthello.cc', 'test/cctest/test_node_crypto.cc', + 'test/cctest/test_node_crypto_env.cc', 'test/cctest/test_quic_cid.cc', 'test/cctest/test_quic_tokens.cc', ] diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 626a1e2c9a6389..36b59eb4d5dab3 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -62,18 +62,16 @@ inline X509_STORE* GetOrCreateRootCertStore() { // Takes a string or buffer and loads it into a BIO. // Caller responsible for BIO_free_all-ing the returned object. BIOPointer LoadBIO(Environment* env, Local v) { - HandleScope scope(env->isolate()); - - if (v->IsString()) { - Utf8Value s(env->isolate(), v); - return NodeBIO::NewFixed(*s, s.length()); - } - - if (v->IsArrayBufferView()) { - ArrayBufferViewContents buf(v.As()); - return NodeBIO::NewFixed(buf.data(), buf.length()); + if (v->IsString() || v->IsArrayBufferView()) { + BIOPointer bio(BIO_new(BIO_s_secmem())); + if (!bio) return nullptr; + ByteSource bsrc = ByteSource::FromStringOrBuffer(env, v); + if (bsrc.size() > INT_MAX) return nullptr; + int written = BIO_write(bio.get(), bsrc.data(), bsrc.size()); + if (written < 0) return nullptr; + if (static_cast(written) != bsrc.size()) return nullptr; + return bio; } - return nullptr; } diff --git a/test/cctest/test_node_crypto_env.cc b/test/cctest/test_node_crypto_env.cc new file mode 100644 index 00000000000000..b42cdc107e8a94 --- /dev/null +++ b/test/cctest/test_node_crypto_env.cc @@ -0,0 +1,31 @@ +#include "crypto/crypto_bio.h" +#include "gtest/gtest.h" +#include "node_options.h" +#include "node_test_fixture.h" +#include "openssl/err.h" + +using v8::Local; +using v8::String; + +/* + * This test verifies that an object created by LoadBIO supports BIO_tell + * and BIO_seek, otherwise PEM_read_bio_PrivateKey fails on some keys + * (if OpenSSL needs to rewind pointer between pem_read_bio_key() + * and pem_read_bio_key_legacy() inside PEM_read_bio_PrivateKey). + */ +class NodeCryptoEnv : public EnvironmentTestFixture {}; + +TEST_F(NodeCryptoEnv, LoadBIO) { + v8::HandleScope handle_scope(isolate_); + Argv argv; + Env env{handle_scope, argv}; + // just put a random string into BIO + Local key = String::NewFromUtf8(isolate_, "abcdef").ToLocalChecked(); + node::crypto::BIOPointer bio(node::crypto::LoadBIO(*env, key)); +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + BIO_seek(bio.get(), 2); + ASSERT_EQ(BIO_tell(bio.get()), 2); +#endif + ASSERT_EQ(ERR_peek_error(), 0UL) << "There should not have left " + "any errors on the OpenSSL error stack\n"; +}