Commit be6f8f800ae for nodejs
commit be6f8f800aece9527bf296d0fb015b2b08179f11
Author: Matteo Collina <hello@matteocollina.com>
Date: Fri Nov 6 08:28:07 2026 +0100
crypto: prevent stale OpenSSL errors
Clear errors raised by key comparison, SPKAC parsing, and JWK
import. Ensure TLS and private-key parsing only inspect errors
from their own OpenSSL calls.
Assisted-by: Codex
Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: https://github.com/nodejs/node/pull/67225
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
diff --git a/deps/ncrypto/ncrypto.cc b/deps/ncrypto/ncrypto.cc
index f0940bb38f5..7279e893194 100644
--- a/deps/ncrypto/ncrypto.cc
+++ b/deps/ncrypto/ncrypto.cc
@@ -933,6 +933,7 @@ int64_t PortableTimeGM(struct tm* t) {
// SPKAC
bool VerifySpkac(const char* input, size_t length) {
+ ClearErrorOnReturn clear_error_on_return;
#ifdef OPENSSL_IS_BORINGSSL
// OpenSSL uses EVP_DecodeBlock, which explicitly removes trailing characters,
// while BoringSSL uses EVP_DecodedLength and EVP_DecodeBase64, which do not.
@@ -951,6 +952,7 @@ bool VerifySpkac(const char* input, size_t length) {
}
BIOPointer ExportPublicKey(const char* input, size_t length) {
+ ClearErrorOnReturn clear_error_on_return;
BIOPointer bio(BIO_new(BIO_s_mem()));
if (!bio) return {};
@@ -972,6 +974,7 @@ BIOPointer ExportPublicKey(const char* input, size_t length) {
}
Buffer<char> ExportChallenge(const char* input, size_t length) {
+ ClearErrorOnReturn clear_error_on_return;
#ifdef OPENSSL_IS_BORINGSSL
// OpenSSL uses EVP_DecodeBlock, which explicitly removes trailing characters,
// while BoringSSL uses EVP_DecodedLength and EVP_DecodeBase64, which do not.
@@ -4542,6 +4545,7 @@ bool ECKeyHasMissingOid(const EVPKeyPointer& key) {
EVPKeyPointer::ParseKeyResult EVPKeyPointer::TryParsePrivateKey(
const PrivateKeyEncodingConfig& config,
const Buffer<const unsigned char>& buffer) {
+ ERR_clear_error();
static constexpr auto keyOrError = [](EVPKeyPointer pkey,
bool had_passphrase = false) {
if (unsigned long err = ERR_peek_error()) { // NOLINT(runtime/int)
diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc
index 88381b901cd..35c01a5e9e6 100644
--- a/src/crypto/crypto_keys.cc
+++ b/src/crypto/crypto_keys.cc
@@ -21,6 +21,7 @@ namespace node {
using ncrypto::BignumPointer;
using ncrypto::BIOPointer;
+using ncrypto::ClearErrorOnReturn;
using ncrypto::ECKeyPointer;
using ncrypto::ECPointPointer;
using ncrypto::EVPKeyCtxPointer;
@@ -680,6 +681,7 @@ static KeyObjectData ImportRawKey(Environment* env,
// Shared helper for importing a JWK asymmetric key. Extracts kty from the
// JWK object and dispatches to the appropriate importer.
static KeyObjectData ImportJWKFromArgs(Environment* env, Local<Object> jwk) {
+ ClearErrorOnReturn clear_error_on_return;
Local<Value> kty;
if (!jwk->Get(env->context(), env->jwk_kty_string()).ToLocal(&kty) ||
!kty->IsString()) {
@@ -1311,6 +1313,7 @@ void KeyObjectHandle::Equals(const FunctionCallbackInfo<Value>& args) {
case kKeyTypePublic:
// Fall through
case kKeyTypePrivate: {
+ ClearErrorOnReturn clear_error_on_return;
EVP_PKEY* pkey = key.GetAsymmetricKey().get();
EVP_PKEY* pkey2 = key2.GetAsymmetricKey().get();
#ifndef OPENSSL_IS_BORINGSSL
diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc
index 075ab9241b9..7f4d7047337 100644
--- a/src/crypto/crypto_tls.cc
+++ b/src/crypto/crypto_tls.cc
@@ -73,6 +73,20 @@ namespace crypto {
namespace {
+void InjectOpenSSLErrorForTesting(const FunctionCallbackInfo<Value>&) {
+#if NCRYPTO_USE_OPENSSL_PROVIDER
+ ERR_raise(ERR_LIB_USER, ERR_R_INTERNAL_ERROR);
+#elif NCRYPTO_USE_BORINGSSL
+ ERR_put_error(ERR_LIB_USER, 0, ERR_R_INTERNAL_ERROR, __FILE__, __LINE__);
+#else
+#error "Unsupported crypto backend"
+#endif
+}
+
+void HasOpenSSLErrorForTesting(const FunctionCallbackInfo<Value>& args) {
+ args.GetReturnValue().Set(ERR_peek_error() != 0);
+}
+
// Our custom implementation of the certificate verify callback
// used when establishing a TLS handshake. Because we cannot perform
// I/O quickly enough with X509_STORE_CTX_ APIs in this callback,
@@ -883,6 +897,7 @@ void TLSWrap::ClearOut() {
for (;;) {
{
SSLLibraryCallScope ssl_library_call_scope(this);
+ if (ERR_set_mark() != 0) ERR_clear_error();
read = SSL_read(ssl_.get(), out + length, kClearOutChunkSize - length);
}
Debug(this, "Read %d bytes of cleartext output", read);
@@ -1016,6 +1031,7 @@ void TLSWrap::ClearIn() {
int written;
{
SSLLibraryCallScope ssl_library_call_scope(this);
+ if (ERR_set_mark() != 0) ERR_clear_error();
written = SSL_write(ssl_.get(), bs->Data(), bs->ByteLength());
}
Debug(this, "Writing %zu bytes, written = %d", bs->ByteLength(), written);
@@ -1214,6 +1230,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(length);
{
SSLLibraryCallScope ssl_library_call_scope(this);
+ if (ERR_set_mark() != 0) ERR_clear_error();
written = SSL_write(ssl_.get(), bs->Data(), length);
}
} else {
@@ -1222,6 +1239,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
NodeBIO::FromBIO(enc_out_)->set_allocate_tls_hint(buf->len);
{
SSLLibraryCallScope ssl_library_call_scope(this);
+ if (ERR_set_mark() != 0) ERR_clear_error();
written = SSL_write(ssl_.get(), buf->base, buf->len);
}
@@ -2351,6 +2369,12 @@ void TLSWrap::Initialize(
Isolate* isolate = env->isolate();
SetMethod(context, target, "wrap", TLSWrap::Wrap);
+ SetMethod(context,
+ target,
+ "injectOpenSSLErrorForTesting",
+ InjectOpenSSLErrorForTesting);
+ SetMethodNoSideEffect(
+ context, target, "hasOpenSSLErrorForTesting", HasOpenSSLErrorForTesting);
NODE_DEFINE_CONSTANT(target, HAVE_SSL_TRACE);
@@ -2445,6 +2469,8 @@ void TLSWrap::Initialize(
void TLSWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
registry->Register(TLSWrap::Wrap);
registry->Register(GetWriteQueueSize);
+ registry->Register(InjectOpenSSLErrorForTesting);
+ registry->Register(HasOpenSSLErrorForTesting);
registry->Register(CertCbDone);
registry->Register(ClientHelloDone);
diff --git a/test/cctest/test_node_crypto.cc b/test/cctest/test_node_crypto.cc
index bbf06673431..e23a3b82df0 100644
--- a/test/cctest/test_node_crypto.cc
+++ b/test/cctest/test_node_crypto.cc
@@ -71,6 +71,16 @@ std::string BioContents(const ncrypto::BIOPointer& bio) {
return std::string(mem->data, mem->length);
}
+void RaiseTestError() {
+#if NCRYPTO_USE_OPENSSL_PROVIDER
+ ERR_raise(ERR_LIB_USER, ERR_R_INTERNAL_ERROR);
+#elif NCRYPTO_USE_BORINGSSL
+ ERR_put_error(ERR_LIB_USER, 0, ERR_R_INTERNAL_ERROR, __FILE__, __LINE__);
+#else
+#error "Unsupported crypto backend"
+#endif
+}
+
} // namespace
/*
@@ -102,6 +112,50 @@ TEST(NodeCrypto, TryGetIntCipherOutputLength) {
0, static_cast<size_t>(INT_MAX) + 1, &output_len));
}
+TEST(NodeCrypto, SpkacOperationsClearErrorQueue) {
+ ncrypto::ClearErrorOnReturn clear_errors;
+ constexpr std::string_view invalid = "garbage";
+
+ EXPECT_FALSE(ncrypto::VerifySpkac(invalid.data(), invalid.size()));
+ EXPECT_EQ(ERR_peek_error(), 0UL);
+
+ EXPECT_FALSE(ncrypto::ExportPublicKey(invalid.data(), invalid.size()));
+ EXPECT_EQ(ERR_peek_error(), 0UL);
+
+ const auto challenge =
+ ncrypto::ExportChallenge(invalid.data(), invalid.size());
+ EXPECT_EQ(challenge.data, nullptr);
+ EXPECT_EQ(challenge.len, 0UL);
+ EXPECT_EQ(ERR_peek_error(), 0UL);
+}
+
+TEST(NodeCrypto, PrivateKeyParsingIgnoresUnrelatedErrors) {
+ ncrypto::ClearErrorOnReturn clear_errors;
+ auto ctx = EVPKeyCtxPointer::NewFromAlgorithm(KeyAlgorithm::RSA);
+ ASSERT_TRUE(ctx);
+ ASSERT_TRUE(ctx.initForKeygen());
+ ASSERT_TRUE(ctx.setRsaKeygenBits(2048));
+ EVP_PKEY* raw = nullptr;
+ ASSERT_EQ(EVP_PKEY_keygen(ctx.get(), &raw), 1);
+ EVPKeyPointer key(raw);
+
+ const EVPKeyPointer::PrivateKeyEncodingConfig config(
+ false,
+ EVPKeyPointer::PKFormatType::PEM,
+ EVPKeyPointer::PKEncodingType::PKCS8);
+ auto encoded = key.writePrivateKey(config);
+ ASSERT_TRUE(encoded);
+ const BUF_MEM* mem = encoded.value;
+ ASSERT_NE(mem, nullptr);
+
+ RaiseTestError();
+ ASSERT_NE(ERR_peek_error(), 0UL);
+ auto parsed = EVPKeyPointer::TryParsePrivateKey(
+ config, {reinterpret_cast<const unsigned char*>(mem->data), mem->length});
+ EXPECT_TRUE(parsed);
+ EXPECT_EQ(ERR_peek_error(), 0UL);
+}
+
TEST(NodeCrypto, KeyAlgorithmNames) {
EXPECT_EQ(KeyAlgorithm::FromName("rsa-pss"), &KeyAlgorithm::RSA_PSS);
EXPECT_EQ(KeyAlgorithm::FromName("ML-DSA-44"), &KeyAlgorithm::ML_DSA_44);
diff --git a/test/cctest/test_node_crypto_env.cc b/test/cctest/test_node_crypto_env.cc
index 4c349ac1a0a..d83118d969d 100644
--- a/test/cctest/test_node_crypto_env.cc
+++ b/test/cctest/test_node_crypto_env.cc
@@ -35,6 +35,56 @@ TEST_F(NodeCryptoEnv, LoadBIO) {
"any errors on the OpenSSL error stack\n";
}
+TEST_F(NodeCryptoEnv, KeyObjectEqualsClearsErrorQueue) {
+ v8::HandleScope handle_scope(isolate_);
+ Argv argv;
+ Env env{handle_scope, argv};
+
+ ERR_clear_error();
+ auto result = node::LoadEnvironment(*env, R"JS(
+ const { generateKeyPairSync } = require('node:crypto');
+ const rsa = generateKeyPairSync('rsa', { modulusLength: 2048 });
+ const ec = generateKeyPairSync('ec', { namedCurve: 'P-256' });
+ return rsa.publicKey.equals(ec.publicKey);
+ )JS")
+ .ToLocalChecked();
+ EXPECT_TRUE(result->IsFalse());
+ EXPECT_EQ(ERR_peek_error(), 0UL);
+}
+
+TEST_F(NodeCryptoEnv, InvalidEcJwkClearsErrorQueue) {
+ v8::HandleScope handle_scope(isolate_);
+ Argv argv;
+ Env env{handle_scope, argv};
+
+ ERR_clear_error();
+ auto result = node::LoadEnvironment(*env, R"JS(
+ const { diffieHellman, generateKeyPairSync } = require('node:crypto');
+ const { privateKey } = generateKeyPairSync(
+ 'ec', { namedCurve: 'P-256' });
+ try {
+ diffieHellman({
+ privateKey,
+ publicKey: {
+ key: {
+ kty: 'EC',
+ crv: 'P-256',
+ x: 'AAAA',
+ y: 'AAAA',
+ },
+ format: 'jwk',
+ },
+ });
+ return false;
+ } catch (error) {
+ return error.code === 'ERR_CRYPTO_INVALID_JWK';
+ }
+ )JS")
+ .ToLocalChecked();
+ EXPECT_TRUE(result->IsTrue());
+ EXPECT_EQ(ERR_peek_error(), 0UL);
+}
+
#if NCRYPTO_USE_OPENSSL_PROVIDER
TEST_F(NodeCryptoEnv, ExportIncompleteRsaPrivateKeyAsJwk) {
v8::HandleScope handle_scope(isolate_);
diff --git a/test/parallel/test-crypto-encap-decap.js b/test/parallel/test-crypto-encap-decap.js
index 50a44f91051..6e98c40cbac 100644
--- a/test/parallel/test-crypto-encap-decap.js
+++ b/test/parallel/test-crypto-encap-decap.js
@@ -111,7 +111,7 @@ for (const [name, {
}] of Object.entries(keys)) {
if (!supported) {
assert.throws(() => crypto.encapsulate(publicKey),
- { code: /ERR_OSSL_EVP_DECODE_ERROR|ERR_OSSL_EVP_UNSUPPORTED_ALGORITHM|ERR_CRYPTO_OPERATION_FAILED/ });
+ { code: /ERR_OSSL_UNSUPPORTED|ERR_OSSL_PEM_NO_START_LINE|ERR_CRYPTO_OPERATION_FAILED/ });
continue;
}
diff --git a/test/parallel/test-crypto-openssl-error-queue.js b/test/parallel/test-crypto-openssl-error-queue.js
new file mode 100644
index 00000000000..3c3329a819d
--- /dev/null
+++ b/test/parallel/test-crypto-openssl-error-queue.js
@@ -0,0 +1,46 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+ common.skip('missing crypto');
+
+const assert = require('node:assert');
+const {
+ Certificate,
+ createPrivateKey,
+ createPublicKey,
+ diffieHellman,
+ generateKeyPairSync,
+} = require('node:crypto');
+const fixtures = require('../common/fixtures');
+
+const privateKeyPem = fixtures.readKey('rsa_private.pem');
+const rsaPublicKey = createPublicKey(fixtures.readKey('rsa_public.pem'));
+const ec = generateKeyPairSync('ec', { namedCurve: 'P-256' });
+
+function parseValidPrivateKey() {
+ // This must not pick up an error from the preceding operation.
+ createPrivateKey(privateKeyPem);
+}
+
+assert.strictEqual(rsaPublicKey.equals(ec.publicKey), false);
+parseValidPrivateKey();
+
+for (const method of ['verifySpkac', 'exportPublicKey', 'exportChallenge']) {
+ Certificate[method](Buffer.from('garbage'));
+ parseValidPrivateKey();
+}
+
+assert.throws(() => diffieHellman({
+ privateKey: ec.privateKey,
+ publicKey: {
+ key: {
+ kty: 'EC',
+ crv: 'P-256',
+ x: 'AAAA',
+ y: 'AAAA',
+ },
+ format: 'jwk',
+ },
+}), { code: 'ERR_CRYPTO_INVALID_JWK' });
+parseValidPrivateKey();
diff --git a/test/parallel/test-crypto-pqc-key-objects-ml-dsa.js b/test/parallel/test-crypto-pqc-key-objects-ml-dsa.js
index 1fb0fa19f08..7c96a38b215 100644
--- a/test/parallel/test-crypto-pqc-key-objects-ml-dsa.js
+++ b/test/parallel/test-crypto-pqc-key-objects-ml-dsa.js
@@ -102,7 +102,7 @@ for (const [asymmetricKeyType, pubLen] of [
if (!hasOpenSSL(3, 5) && !isBoringSSL) {
assert.throws(() => createPublicKey(keys.public), {
- code: 'ERR_OSSL_EVP_DECODE_ERROR',
+ code: 'ERR_OSSL_UNSUPPORTED',
});
for (const pem of [keys.private, keys.private_seed_only, keys.private_priv_only]) {
diff --git a/test/parallel/test-crypto-pqc-key-objects-ml-kem.js b/test/parallel/test-crypto-pqc-key-objects-ml-kem.js
index 7ae706af400..10fd1625669 100644
--- a/test/parallel/test-crypto-pqc-key-objects-ml-kem.js
+++ b/test/parallel/test-crypto-pqc-key-objects-ml-kem.js
@@ -102,7 +102,7 @@ for (const [asymmetricKeyType, pubLen] of [
if (!hasOpenSSL(3, 5) && !isBoringSSL) {
assert.throws(() => createPublicKey(keys.public), {
- code: 'ERR_OSSL_EVP_DECODE_ERROR',
+ code: 'ERR_OSSL_UNSUPPORTED',
});
for (const pem of [keys.private, keys.private_seed_only, keys.private_priv_only]) {
@@ -113,7 +113,7 @@ for (const [asymmetricKeyType, pubLen] of [
} else if (isBoringSSL && asymmetricKeyType === 'ml-kem-512') {
// BoringSSL does not support ML-KEM-512.
assert.throws(() => createPublicKey(keys.public),
- { code: 'ERR_OSSL_EVP_UNSUPPORTED_ALGORITHM' });
+ { code: 'ERR_OSSL_PEM_NO_START_LINE' });
for (const pem of [keys.private, keys.private_seed_only, keys.private_priv_only]) {
assert.throws(() => createPrivateKey(pem),
{ code: 'ERR_OSSL_EVP_UNSUPPORTED_ALGORITHM' });
diff --git a/test/parallel/test-crypto-pqc-key-objects-slh-dsa.js b/test/parallel/test-crypto-pqc-key-objects-slh-dsa.js
index 090aa65f9dc..3f0dbc73ba5 100644
--- a/test/parallel/test-crypto-pqc-key-objects-slh-dsa.js
+++ b/test/parallel/test-crypto-pqc-key-objects-slh-dsa.js
@@ -101,7 +101,7 @@ for (const asymmetricKeyType of [
if (!hasOpenSSL(3, 5)) {
assert.throws(() => createPublicKey(keys.public), {
- code: isBoringSSL ? 'ERR_OSSL_EVP_UNSUPPORTED_ALGORITHM' : 'ERR_OSSL_EVP_DECODE_ERROR',
+ code: isBoringSSL ? 'ERR_OSSL_PEM_NO_START_LINE' : 'ERR_OSSL_UNSUPPORTED',
});
assert.throws(() => createPrivateKey(keys.private), {
diff --git a/test/parallel/test-tls-openssl-error-queue.js b/test/parallel/test-tls-openssl-error-queue.js
new file mode 100644
index 00000000000..944f5d7a2cf
--- /dev/null
+++ b/test/parallel/test-tls-openssl-error-queue.js
@@ -0,0 +1,95 @@
+// Flags: --expose-internals
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+ common.skip('missing crypto');
+
+const assert = require('node:assert');
+const { fork } = require('node:child_process');
+const https = require('node:https');
+const fixtures = require('../common/fixtures');
+
+if (process.argv[2] === 'server') {
+ const {
+ hasOpenSSLErrorForTesting,
+ injectOpenSSLErrorForTesting,
+ } = require('internal/test/binding').internalBinding('tls_wrap');
+
+ let requests = 0;
+ const server = https.createServer({
+ key: fixtures.readKey('agent1-key.pem'),
+ cert: fixtures.readKey('agent1-cert.pem'),
+ }, common.mustCall((req, res) => {
+ requests++;
+ if (requests === 1) {
+ res.on('finish', common.mustCall(() => {
+ injectOpenSSLErrorForTesting();
+ process.send({ injected: true });
+ }));
+ res.end('first');
+ return;
+ }
+
+ const queueIsClear = !hasOpenSSLErrorForTesting();
+ res.on('finish', common.mustCall(() => {
+ server.close(common.mustCall(() => process.disconnect()));
+ }));
+ res.end(queueIsClear ? 'clear' : 'dirty');
+ }, 2));
+
+ server.listen(0, '127.0.0.1', common.mustCall(() => {
+ process.send({ port: server.address().port });
+ }));
+ return;
+}
+
+function nextMessage(child) {
+ return new Promise((resolve) => child.once('message', resolve));
+}
+
+function request(port, agent) {
+ return new Promise((resolve, reject) => {
+ https.get({
+ agent,
+ host: '127.0.0.1',
+ port,
+ rejectUnauthorized: false,
+ }, (res) => {
+ const socket = res.socket;
+ let body = '';
+ res.setEncoding('utf8');
+ res.on('data', (chunk) => body += chunk);
+ res.on('end', () => resolve({ body, socket }));
+ }).on('error', reject);
+ });
+}
+
+async function main() {
+ const child = fork(__filename, ['server']);
+ const exited = new Promise((resolve, reject) => {
+ child.once('error', reject);
+ child.once('exit', (code, signal) => {
+ if (code === 0) {
+ resolve();
+ } else {
+ reject(new Error(`server exited with code ${code}, signal ${signal}`));
+ }
+ });
+ });
+
+ const { port } = await nextMessage(child);
+ const injected = nextMessage(child);
+ const agent = new https.Agent({ keepAlive: true, maxSockets: 1 });
+ const first = await request(port, agent);
+ assert.strictEqual(first.body, 'first');
+ assert.deepStrictEqual(await injected, { injected: true });
+
+ const second = await request(port, agent);
+ assert.strictEqual(second.socket, first.socket);
+ assert.strictEqual(second.body, 'clear');
+ agent.destroy();
+ await exited;
+}
+
+main().then(common.mustCall());