Drop #ifdef NODE_FIPS_MODE wherever possibleΒ #34903
Description
Activity
IOW, it would be nice if Node.js always relied solely on OpenSSL settings, no matter what build options or command line options are specified. The configuration options should only influence OpenSSL settings, not the Node.js code.
Pull request welcome, I think.
- added a commit that references this issue
on Aug 25, 2020 That seems reasonable to me and what you have in the branch which mentions this looks good so far.
Thx. I need to do some testing. I'll send PR once it is ready.
I did some testing around this on FIPS-enabled system. I built node with varying options and then ran
./node [--enable-fips] -p 'crypto.getFips()'.-
Node built with
--shared-openssl --openssl-is-fipsconfigure options, without this patch applied:./node --enable-fips -p 'crypto.getFips()'prints1β FIPS mode enabled../node -p 'crypto.getFips()'prints0β FIPS mode is not enabled.
-
Node built with
--shared-openssland with this patch applied:Same results as in point 1. Note that the configure option
--openssl-is-fipsseems to have no effect here, which is the whole point of this patch π. -
Node built with
--shared-opensslwithout this patch:./node --enable-fips -p 'crypto.getFips()'reportsbad option: --enable-fips../node -p 'crypto.getFips()'prints0β FIPS mode not enabled.
In this configuration, FIPS mode cannot be enabled at all.
-
Node built with
--openssl-is-fipsor this patch applied (same results), but without--shared-openssl(and thus with the bundled one):-
./node --enable-fips -p 'crypto.getFips()'throws an error:openssl fips failed: error:0F06D065:common libcrypto routines:FIPS_mode_set:fips mode not supported ./node[1627]: ../src/node_crypto.cc:7028:void node::crypto::InitCryptoOnce(): Assertion `"Unreachable code reached"' failed. 1: 0xad7950 node::Abort() [./node] 2: 0xad79e2 node::Assert(node::AssertionInfo const&) [./node] 3: 0xc02347 node::crypto::InitCryptoOnce() [./node] 4: 0x7fe39fc9e20b [/lib64/libpthread.so.0] 5: 0x1458469 uv_once [./node] 6: 0xc0b836 node::crypto::Initialize(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>, void*) [./node] 7: 0xaaa73b node::binding::GetInternalBinding(v8::FunctionCallbackInfo<v8::Value> const&) [./node] 8: 0xccd7a2 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [./node] 9: 0xccdbf3 [./node] 10: 0xcce496 [./node] 11: 0xcce726 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [./node] 12: 0x14cf119 [./node] -
./node -p crypto.getFips()prints0β FIPS mode not enabled.
As expected, here the FIPS mode is reported as unsupported by the bundled OpenSSL version,
although it is quite aggresive about it β the assertion seems quite strict. -
-
I did not really test the "vanilla" Node build, but from the above combinations
I assume that the--enable-fipsoption would not exist and without it,
thecrypto.getFips()would still return0, making this case equivalent to point 3.
@voxik Hope this helps, let me know if you need any other combination :)
-
@khardix Thx for testing. So if I understand correctly, you have tested on the system where FIPS was supported, but disabled by default. Do you think you could provide also test results on the system with OpenSSL configured in FIPS mode by default? I think that specifying
fips=1on kernel cmd line should be enough to enable the FIPS mode.And I am going to take a look into the 4th bullet.
And I am going to take a look into the 4th bullet.
So I think the problem lies at this part of
InitCryptoOnce():/* Override FIPS settings in cnf file, if needed. */ unsigned long err = 0; // NOLINT(runtime/int) if (per_process::cli_options->enable_fips_crypto || per_process::cli_options->force_fips_crypto) { if (0 == FIPS_mode() && !FIPS_mode_set(1)) { err = ERR_get_error(); } } if (0 != err) { fprintf(stderr, "openssl fips failed: %s\n", ERR_error_string(err, nullptr)); UNREACHABLE(); }where the
UNREACHABLE();aborts the Node.js and actually the abortion is probably correct, because when OpenSSL does not support FIPS, it can't be enabled. But it should abort more gracefully. Not sure how to do that. May be something likereturn ThrowCryptoError(env, err);- added a commit that references this issue
on Feb 9, 2021 - added a commit that references this issue
on Feb 28, 2021 - added 2 commits that reference this issue
on Nov 25, 2021 - added a commit that references this issue
on May 22, 2026
Drop
#ifdef NODE_FIPS_MODEwherever possible, because it does not make sense to guard FIPS code by#ifdef NODE_FIPS_MODE, when this ifdef is immediately followed byFIPS_mode(). It would make sense if theFIPS_mode()is not defined depending on OpenSSL settings, but that is not the case. I believe that removing the guards would help our users to get more precise information about FIPS settings from calls such asnode -p 'crypto.getFips()'. This would also help to resolve this ticket.