From 04b98a7d9dcdab90a75fbdd764ba443bb1cbded4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 18 May 2025 19:21:03 +0100 Subject: [PATCH] src: fix FIPS init error handling If `--enable-fips` or `--force-fips` fails to be applied during `ProcessFipsOptions()`, the node process might exit with `ExitCode::kNoFailure` because `ERR_GET_REASON(ERR_peek_error())` can return `0` since `ncrypto::testFipsEnabled()` does not populate the OpenSSL error queue. You can likely test this locally by running node --enable-fips && echo $? with the current `node` binary from the main branch if compiled without support for FIPS. As confirmed by the `XXX` comment here, which was added three years ago in commit b6971602564fc93c536ad469947536b487c968ea, even if the error queue was populated properly, the OpenSSL reason codes are not really related to the Node.js `ExitCode` enumeration. --- src/node.cc | 5 +---- test/parallel/test-crypto-fips.js | 29 ++++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/src/node.cc b/src/node.cc index f921303b639dbc..3f76fed280fb84 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1175,10 +1175,7 @@ InitializeOncePerProcessInternal(const std::vector& args, } #endif if (!crypto::ProcessFipsOptions()) { - // XXX: ERR_GET_REASON does not return something that is - // useful as an exit code at all. - result->exit_code_ = - static_cast(ERR_GET_REASON(ERR_peek_error())); + result->exit_code_ = ExitCode::kGenericUserError; result->early_return_ = true; result->errors_.emplace_back( "OpenSSL error when trying to enable FIPS:\n" + diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index de004b9a9e8f23..a5e25b8fd1073a 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -23,13 +23,16 @@ const FIPS_ENABLE_ERROR_STRING = 'OpenSSL error when trying to enable FIPS:'; const CNF_FIPS_ON = fixtures.path('openssl_fips_enabled.cnf'); const CNF_FIPS_OFF = fixtures.path('openssl_fips_disabled.cnf'); +const kNoFailure = 0; +const kGenericUserError = 1; + let num_children_ok = 0; function sharedOpenSSL() { return process.config.variables.node_shared_openssl; } -function testHelper(stream, args, expectedOutput, cmd, env) { +function testHelper(stream, args, expectedStatus, expectedOutput, cmd, env) { const fullArgs = args.concat(['-e', `console.log(${cmd})`]); const child = spawnSync(process.execPath, fullArgs, { cwd: path.dirname(process.execPath), @@ -56,6 +59,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) { // Normal path where we expect either FIPS enabled or disabled. assert.strictEqual(getFipsValue, expectedOutput); } + assert.strictEqual(child.status, expectedStatus); childOk(child); } @@ -66,6 +70,7 @@ function testHelper(stream, args, expectedOutput, cmd, env) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--enable-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING, 'process.versions', process.env); @@ -74,6 +79,7 @@ testHelper( testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--force-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_ENABLE_ERROR_STRING, 'process.versions', process.env); @@ -85,6 +91,7 @@ if (!sharedOpenSSL()) { testHelper( 'stdout', [], + kNoFailure, FIPS_DISABLED, 'require("crypto").getFips()', { ...process.env, 'OPENSSL_CONF': ' ' }); @@ -94,6 +101,7 @@ if (!sharedOpenSSL()) { testHelper( 'stderr', [], + kGenericUserError, 'Calling crypto.setFips() is not supported in workers', 'new worker_threads.Worker(\'require("crypto").setFips(true);\', { eval: true })', process.env); @@ -120,6 +128,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) { testHelper( 'stdout', [`--openssl-config=${CNF_FIPS_ON}`], + kNoFailure, testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED, 'require("crypto").getFips()', process.env); @@ -128,6 +137,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) { testHelper( 'stdout', [], + kNoFailure, testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED, 'require("crypto").getFips()', Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON })); @@ -136,6 +146,7 @@ if (!sharedOpenSSL() && !hasOpenSSL3) { testHelper( 'stdout', [`--openssl-config=${CNF_FIPS_ON}`], + kNoFailure, testFipsCrypto() ? FIPS_ENABLED : FIPS_DISABLED, 'require("crypto").getFips()', Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF })); @@ -149,6 +160,7 @@ if (!hasOpenSSL3) { testHelper( 'stdout', [`--openssl-config=${CNF_FIPS_OFF}`], + kNoFailure, FIPS_DISABLED, 'require("crypto").getFips()', Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_ON })); @@ -157,6 +169,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--enable-fips', `--openssl-config=${CNF_FIPS_OFF}`], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").getFips()', process.env); @@ -164,6 +177,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--force-fips', `--openssl-config=${CNF_FIPS_OFF}`], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").getFips()', process.env); @@ -171,6 +185,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--enable-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").getFips()', process.env); @@ -179,6 +194,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--force-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").getFips()', process.env); @@ -187,6 +203,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--enable-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").getFips()', Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF })); @@ -195,6 +212,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--force-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").getFips()', Object.assign({}, process.env, { 'OPENSSL_CONF': CNF_FIPS_OFF })); @@ -203,6 +221,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', [], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, '(require("crypto").setFips(true),' + 'require("crypto").getFips())', @@ -212,6 +231,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', [], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING, '(require("crypto").setFips(true),' + 'require("crypto").setFips(false),' + @@ -222,6 +242,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', [`--openssl-config=${CNF_FIPS_OFF}`], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, '(require("crypto").setFips(true),' + 'require("crypto").getFips())', @@ -231,6 +252,7 @@ if (!hasOpenSSL3) { testHelper( 'stdout', [`--openssl-config=${CNF_FIPS_ON}`], + kNoFailure, FIPS_DISABLED, '(require("crypto").setFips(false),' + 'require("crypto").getFips())', @@ -240,6 +262,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--enable-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_DISABLED : FIPS_UNSUPPORTED_ERROR_STRING, '(require("crypto").setFips(false),' + 'require("crypto").getFips())', @@ -249,6 +272,7 @@ if (!hasOpenSSL3) { testHelper( 'stderr', ['--force-fips'], + kGenericUserError, testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").setFips(false)', process.env); @@ -257,6 +281,7 @@ if (!hasOpenSSL3) { testHelper( testFipsCrypto() ? 'stdout' : 'stderr', ['--force-fips'], + testFipsCrypto() ? kNoFailure : kGenericUserError, testFipsCrypto() ? FIPS_ENABLED : FIPS_UNSUPPORTED_ERROR_STRING, '(require("crypto").setFips(true),' + 'require("crypto").getFips())', @@ -266,6 +291,7 @@ if (!hasOpenSSL3) { testHelper( 'stderr', ['--force-fips', '--enable-fips'], + kGenericUserError, testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").setFips(false)', process.env); @@ -274,6 +300,7 @@ if (!hasOpenSSL3) { testHelper( 'stderr', ['--enable-fips', '--force-fips'], + kGenericUserError, testFipsCrypto() ? FIPS_ERROR_STRING2 : FIPS_UNSUPPORTED_ERROR_STRING, 'require("crypto").setFips(false)', process.env);