crypto: pass empty passphrases to OpenSSL properly

Fixes: #35898

PR-URL: #35914
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>

Author: tniessen

src/crypto/crypto_context.cc

@@ -522,13 +522,19 @@ void SecureContext::SetKey(const FunctionCallbackInfo<Value>& args) {
   if (!bio)
     return;
 
-  node::Utf8Value passphrase(env->isolate(), args[1]);
+  ByteSource passphrase;
+  if (args[1]->IsString())
+    passphrase = ByteSource::FromString(env, args[1].As<String>());
+  // This redirection is necessary because the PasswordCallback expects a
+  // pointer to a pointer to the passphrase ByteSource to allow passing in
+  // const ByteSources.
+  const ByteSource* pass_ptr = &passphrase;
 
   EVPKeyPointer key(
       PEM_read_bio_PrivateKey(bio.get(),
                               nullptr,
                               PasswordCallback,
-                              *passphrase));
+                              &pass_ptr));
 
   if (!key) {
     unsigned long err = ERR_get_error();  // NOLINT(runtime/int)

src/crypto/crypto_keygen.h

@@ -251,8 +251,10 @@ struct KeyPairGenConfig final : public MemoryRetainer {
 
   void MemoryInfo(MemoryTracker* tracker) const override {
     tracker->TrackField("key", key);
-    tracker->TrackFieldWithSize("private_key_encoding.passphrase",
-                        private_key_encoding.passphrase_.size());
+    if (!private_key_encoding.passphrase_.IsEmpty()) {
+      tracker->TrackFieldWithSize("private_key_encoding.passphrase",
+                                  private_key_encoding.passphrase_->size());
+    }
     tracker->TrackField("params", params);
   }
 

src/crypto/crypto_keys.cc

@@ -210,8 +210,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
                                const PrivateKeyEncodingConfig& config,
                                const char* key,
                                size_t key_len) {
-  // OpenSSL needs a non-const pointer, that's why the const_cast is required.
-  char* const passphrase = const_cast<char*>(config.passphrase_.get());
+  const ByteSource* passphrase = config.passphrase_.get();
 
   if (config.format_ == kKeyFormatPEM) {
     BIOPointer bio(BIO_new_mem_buf(key, key_len));
@@ -221,7 +220,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
     pkey->reset(PEM_read_bio_PrivateKey(bio.get(),
                                         nullptr,
                                         PasswordCallback,
-                                        passphrase));
+                                        &passphrase));
   } else {
     CHECK_EQ(config.format_, kKeyFormatDER);
 
@@ -238,7 +237,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
         pkey->reset(d2i_PKCS8PrivateKey_bio(bio.get(),
                                             nullptr,
                                             PasswordCallback,
-                                            passphrase));
+                                            &passphrase));
       } else {
         PKCS8Pointer p8inf(d2i_PKCS8_PRIV_KEY_INFO_bio(bio.get(), nullptr));
         if (p8inf)
@@ -260,7 +259,7 @@ ParseKeyResult ParsePrivateKey(EVPKeyPointer* pkey,
     return ParseKeyResult::kParseKeyOk;
   if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
       ERR_GET_REASON(err) == PEM_R_BAD_PASSWORD_READ) {
-    if (config.passphrase_.get() == nullptr)
+    if (config.passphrase_.IsEmpty())
       return ParseKeyResult::kParseKeyNeedPassphrase;
   }
   return ParseKeyResult::kParseKeyFailed;
@@ -293,6 +292,28 @@ MaybeLocal<Value> WritePrivateKey(
   BIOPointer bio(BIO_new(BIO_s_mem()));
   CHECK(bio);
 
+  // If an empty string was passed as the passphrase, the ByteSource might
+  // contain a null pointer, which OpenSSL will ignore, causing it to invoke its
+  // default passphrase callback, which would block the thread until the user
+  // manually enters a passphrase. We could supply our own passphrase callback
+  // to handle this special case, but it is easier to avoid passing a null
+  // pointer to OpenSSL.
+  char* pass = nullptr;
+  size_t pass_len = 0;
+  if (!config.passphrase_.IsEmpty()) {
+    pass = const_cast<char*>(config.passphrase_->get());
+    pass_len = config.passphrase_->size();
+    if (pass == nullptr) {
+      // OpenSSL will not actually dereference this pointer, so it can be any
+      // non-null pointer. We cannot assert that directly, which is why we
+      // intentionally use a pointer that will likely cause a segmentation fault
+      // when dereferenced.
+      CHECK_EQ(pass_len, 0);
+      pass = reinterpret_cast<char*>(-1);
+      CHECK_NE(pass, nullptr);
+    }
+  }
+
   bool err;
 
   PKEncodingType encoding_type = config.type_.ToChecked();
@@ -303,12 +324,11 @@ MaybeLocal<Value> WritePrivateKey(
     RSAPointer rsa(EVP_PKEY_get1_RSA(pkey));
     if (config.format_ == kKeyFormatPEM) {
       // Encode PKCS#1 as PEM.
-      const char* pass = config.passphrase_.get();
       err = PEM_write_bio_RSAPrivateKey(
                 bio.get(), rsa.get(),
                 config.cipher_,
-                reinterpret_cast<unsigned char*>(const_cast<char*>(pass)),
-                config.passphrase_.size(),
+                reinterpret_cast<unsigned char*>(pass),
+                pass_len,
                 nullptr, nullptr) != 1;
     } else {
       // Encode PKCS#1 as DER. This does not permit encryption.
@@ -322,17 +342,17 @@ MaybeLocal<Value> WritePrivateKey(
       err = PEM_write_bio_PKCS8PrivateKey(
                 bio.get(), pkey,
                 config.cipher_,
-                const_cast<char*>(config.passphrase_.get()),
-                config.passphrase_.size(),
+                pass,
+                pass_len,
                 nullptr, nullptr) != 1;
     } else {
       // Encode PKCS#8 as DER.
       CHECK_EQ(config.format_, kKeyFormatDER);
       err = i2d_PKCS8PrivateKey_bio(
                 bio.get(), pkey,
                 config.cipher_,
-                const_cast<char*>(config.passphrase_.get()),
-                config.passphrase_.size(),
+                pass,
+                pass_len,
                 nullptr, nullptr) != 1;
     }
   } else {
@@ -344,12 +364,11 @@ MaybeLocal<Value> WritePrivateKey(
     ECKeyPointer ec_key(EVP_PKEY_get1_EC_KEY(pkey));
     if (config.format_ == kKeyFormatPEM) {
       // Encode SEC1 as PEM.
-      const char* pass = config.passphrase_.get();
       err = PEM_write_bio_ECPrivateKey(
                 bio.get(), ec_key.get(),
                 config.cipher_,
-                reinterpret_cast<unsigned char*>(const_cast<char*>(pass)),
-                config.passphrase_.size(),
+                reinterpret_cast<unsigned char*>(pass),
+                pass_len,
                 nullptr, nullptr) != 1;
     } else {
       // Encode SEC1 as DER. This does not permit encryption.
@@ -640,7 +659,8 @@ ManagedEVPPKey::GetPrivateKeyEncodingFromJs(
         THROW_ERR_OUT_OF_RANGE(env, "passphrase is too big");
         return NonCopyableMaybe<PrivateKeyEncodingConfig>();
       }
-      result.passphrase_ = passphrase.ToNullTerminatedCopy();
+      result.passphrase_ = NonCopyableMaybe<ByteSource>(
+          passphrase.ToNullTerminatedCopy());
     } else {
       CHECK(args[*offset]->IsNullOrUndefined() && !needs_passphrase);
     }

src/crypto/crypto_keys.h

@@ -63,7 +63,10 @@ using PublicKeyEncodingConfig = AsymmetricKeyEncodingConfig;
 
 struct PrivateKeyEncodingConfig : public AsymmetricKeyEncodingConfig {
   const EVP_CIPHER* cipher_;
-  ByteSource passphrase_;
+  // The ByteSource alone is not enough to distinguish between "no passphrase"
+  // and a zero-length passphrase (which can be a null pointer), therefore, we
+  // use a NonCopyableMaybe.
+  NonCopyableMaybe<ByteSource> passphrase_;
 };
 
 // This uses the built-in reference counter of OpenSSL to manage an EVP_PKEY

src/crypto/crypto_util.cc

@@ -74,13 +74,13 @@ bool EntropySource(unsigned char* buffer, size_t length) {
 }
 
 int PasswordCallback(char* buf, int size, int rwflag, void* u) {
-  const char* passphrase = static_cast<char*>(u);
+  const ByteSource* passphrase = *static_cast<const ByteSource**>(u);
   if (passphrase != nullptr) {
     size_t buflen = static_cast<size_t>(size);
-    size_t len = strlen(passphrase);
+    size_t len = passphrase->size();
     if (buflen < len)
       return -1;
-    memcpy(buf, passphrase, len);
+    memcpy(buf, passphrase->get(), len);
     return len;
   }
 

src/util.h

@@ -602,6 +602,15 @@ class NonCopyableMaybe {
     return empty_;
   }
 
+  const T* get() const {
+    return empty_ ? nullptr : &value_;
+  }
+
+  const T* operator->() const {
+    CHECK(!empty_);
+    return &value_;
+  }
+
   T&& Release() {
     CHECK_EQ(empty_, false);
     empty_ = true;

test/parallel/test-crypto-keygen.js

@@ -7,6 +7,7 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const {
   constants,
+  createPrivateKey,
   createSign,
   createVerify,
   generateKeyPair,
@@ -1171,3 +1172,41 @@ const sec1EncExp = (cipher) => getRegExpForPEM('EC PRIVATE KEY', cipher);
     );
   }
 }
+
+{
+  // Passing an empty passphrase string should not cause OpenSSL's default
+  // passphrase prompt in the terminal.
+  // See https://github.com/nodejs/node/issues/35898.
+
+  for (const type of ['pkcs1', 'pkcs8']) {
+    generateKeyPair('rsa', {
+      modulusLength: 1024,
+      privateKeyEncoding: {
+        type,
+        format: 'pem',
+        cipher: 'aes-256-cbc',
+        passphrase: ''
+      }
+    }, common.mustSucceed((publicKey, privateKey) => {
+      assert.strictEqual(publicKey.type, 'public');
+
+      for (const passphrase of ['', Buffer.alloc(0)]) {
+        const privateKeyObject = createPrivateKey({
+          passphrase,
+          key: privateKey
+        });
+        assert.strictEqual(privateKeyObject.asymmetricKeyType, 'rsa');
+      }
+
+      // Encrypting with an empty passphrase is not the same as not encrypting
+      // the key, and not specifying a passphrase should fail when decoding it.
+      assert.throws(() => {
+        return testSignVerify(publicKey, privateKey);
+      }, {
+        name: 'TypeError',
+        code: 'ERR_MISSING_PASSPHRASE',
+        message: 'Passphrase required for encrypted key'
+      });
+    }));
+  }
+}