Draft: Keep FIPS provider

[khardix]

With shared OpenSSL >= 3, I'm encountering a core dump/assertion failure when trying to use --force-fips/--enable-fips:

$ python configure.py --shared-openssl --shared-zlib --shared-brotli --shared-libuv --shared-nghttp2 --without-corepack --openssl-use-def-ca-store --openssl-default-cipher-list=PROFILE=SYSTEM
INFO: configure completed successfully
$ make -j4
...
$ ./node --version
v21.0.0-pre
$ ./node --force-fips -p 'crypto.getFips()'
./node[495774]: ../src/node.cc:1070:std::unique_ptr<node::InitializationResultImpl> node::InitializeOncePerProcessInternal(const std::vector<std::__cxx11::basic_string<char> >&, ProcessInitializationFlags::Flags): Assertion `crypto::CSPRNG(nullptr, 0).is_ok()' failed.
 1: 0xc0e140 node::Abort() [./node]
 2: 0xc0e1be  [./node]
 3: 0xbc7af0  [./node]
 4: 0xbc7ffc node::Start(int, char**) [./node]
 5: 0x7f7f7ba49b4a  [/lib64/libc.so.6]
 6: 0x7f7f7ba49c0b __libc_start_main [/lib64/libc.so.6]
 7: 0xb22925 _start [./node]
[1]    496569 IOT instruction (core dumped)  ./node --force-fips -p 'crypto.getFips()'

I'm on Fedora 38, openssl-libs-3.0.9-1.fc38.x86_64.

---

I was toying with fixing this a bit, and this PR is my WIP approach at fixing this. The first commit just turns that assertion into an error message in order to better debug what is wrong with the CSPRNG; the other makes the error go away, but I'm not sure what other impact it can have.

On the other hand, can anyone tell me why the unload is there? I would thought that when the FIPS flags are encountered, we would want to actually leave the provider in place in order to be utilized by the application, not immediately yanking it back once confirmed it can be loaded.

I also noticed that in TestFipsCrypto() loads the provider again, then leaves it loaded… 😕

I'm making it as draft for now, because I want to know more about the points above before trying to actually push the change.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/startup

[khardix]

Please note that I will be on holidays/off the grid for next week, so have patience when I'm not responding.😉 I just wanted to kick this off before leaving.

[mhdawson]

@danbev may remember why the load/unload. From the PR my read is that it was to be able to throw an error during options processing, but not have the provider loaded unless it was used.

Looking at the other branch I'm wondering if this would be consistent with what we have in the non >= section

#if OPENSSL_VERSION_MAJOR >= 3
  if (!EVP_default_properties_is_fips_enabled(nullptr)) {
    OSSL_PROVIDER* fips_provider = OSSL_PROVIDER_load(nullptr, "fips");
    if (fips_provider == nullptr)
      return false;
    OSSL_PROVIDER_unload(fips_provider);

    return EVP_default_properties_enable_fips(nullptr, 1) &&
    EVP_default_properties_is_fips_enabled(nullptr);
  }

I think you mentioned to me that doing that still fails. If so, then I wonder why if FIPs is already enabled on the base system, then why the appropriate provider is not already loaded. It would be good to confirm if its expected that you still need to load the provider even if the system has enabled FIPs to make sure the correct provider is being used.

@richardlau I know you have also look a bit at how FIPs and providers work so I'd be interested in any insights you have as well.

[khardix]

Yes, according to my testing just having the guard there does not help on non-FIPS system! I did not test it yet on FIPS system, as in my opinion coredump should not happen in any system configuration.

---

The issue might be in getting confused on what the EVP_… functions actually do – according to manual page, they do not check the provider status at all:

EVP_default_properties_enable_fips() sets the 'fips=yes' to be a default property if enable is non zero, otherwise it clears 'fips' from the default property query for the given libctx. It merges the fips default property query with any existing query strings that have been set via EVP_set_default_properties().

EVP_default_properties_is_fips_enabled() indicates if 'fips=yes' is a default property for the given libctx.

I'm very much guessing in the dark here, but I suspect setting the EVP property fips=yes does not load the provider, nor does it indicate it was ever loaded – it just indicate you want to use it by default.

[richardlau]

@richardlau I know you have also look a bit at how FIPs and providers work so I'd be interested in any insights you have as well.

FIPS support in OpenSSL 3 is completely different from what came before and works with the (new for OpenSSL 3) Provider model. This model is a lot more generic -- you may have any number of providers loaded (aka available) and can change the loaded providers at runtime (through OSSL_PROVIDER_load/OSSL_PROVIDER_unload). Since it's generic the providers themselves can be named anything, i.e. a FIPS provider does not need to be named "fips" (which means the code in Node.js being queried is overly specific and probably does need to change).

So what does "fips mode" mean in the provider model? When fetching an algorithm in OpenSSL the algorithm may be implemented by more than one loaded provider. If that is the case it is undefined which implemented is used unless the fetch was constrained by a property query, e.g. fips=yes -- note this may still match more than one provider if those providers set fips=yes on their algorithm implementations. You can limit to a specifically named provider by property (e.g. provider=fips) but remember that OpenSSL puts no special meaning on the provider names themselves. The property query can either be set on every individual fetch call or as a default property (which applies to all fetch calls that don't specify their own). For OpenSSL 3 Node.js treats "fips mode" as the equivalent of having set a default query of fips=yes.

The genericness of the provider model coupled with the ability to dynamically change the loaded providers also means that it is possible that an algorithm fetch returns no matching implementations. Such a corner case was reported in #46200 -- if fips=yes is set as the default property query and no implementations that have that property are available (e.g. no provider has been loaded that provides a compatible implementation) then in that reported case it was not possible to initialize the cryptographically secure pseudorandom number generator (CSPRNG).

With regards to the code being commented on, I think the intention seems to be to determine if it might be possible to load a provider called "fips" (i.e. the provider is on the filesystem and could be loaded). This is a highly specific assumption -- OpenSSL 3 makes no guarantee that a provider called "fips" actually implements any FIPS compatible algorithms -- so is probably a flawed assumption to be making. I believe OpenSSL's OSSL_PROVIDER_load/OSSL_PROVIDER_unload operates via reference counting (so the provider may not actually be unloaded if it had already been loaded prior to the OSSL_PROVIDER_load call on L125).

[richardlau]

With shared OpenSSL >= 3, I'm encountering a core dump/assertion failure when trying to use --force-fips/--enable-fips:

$ python configure.py --shared-openssl --shared-zlib --shared-brotli --shared-libuv --shared-nghttp2 --without-corepack --openssl-use-def-ca-store --openssl-default-cipher-list=PROFILE=SYSTEM
INFO: configure completed successfully
$ make -j4
...
$ ./node --version
v21.0.0-pre
$ ./node --force-fips -p 'crypto.getFips()'
./node[495774]: ../src/node.cc:1070:std::unique_ptr<node::InitializationResultImpl> node::InitializeOncePerProcessInternal(const std::vector<std::__cxx11::basic_string<char> >&, ProcessInitializationFlags::Flags): Assertion `crypto::CSPRNG(nullptr, 0).is_ok()' failed.
 1: 0xc0e140 node::Abort() [./node]
 2: 0xc0e1be  [./node]
 3: 0xbc7af0  [./node]
 4: 0xbc7ffc node::Start(int, char**) [./node]
 5: 0x7f7f7ba49b4a  [/lib64/libc.so.6]
 6: 0x7f7f7ba49c0b __libc_start_main [/lib64/libc.so.6]
 7: 0xb22925 _start [./node]
[1]    496569 IOT instruction (core dumped)  ./node --force-fips -p 'crypto.getFips()'

I suspect the issue might be that Node.js is not picking up the OpenSSL configuration as the default for Node.js is now to look for a configuration called nodejs_conf and will not load e.g. openssl_conf without it being changed at the configure step. See https://github.com/nodejs/node/blob/main/BUILDING.md#configure-openssl-appname. We deliberately changed this in #43124.

In this case, where no configuration is loaded (because no nodejs_conf section exists in the OpenSSL conf file) OpenSSL will default to the default provider which does not provide any FIPS compatible algorithms which means that when we enable FIPS and set the fips=yes default property no matching implementations will be found and you're effectively running into #46200 (albeit we now throw an assertion to fail rather than loop infinitely like we did previously).

[richardlau]

I suspect the issue might be that Node.js is not picking up the OpenSSL configuration as the default for Node.js is now to look for a configuration called nodejs_conf and will not load e.g. openssl_conf without it being changed at the configure step.

Slight correction, at runtime you can pass --openssl-shared-config as a Node.js command line option to make Node.js read from an openssl_conf section from the config file but you'd have to do that for every invocation of Node.js so it would be better to set the configure time flag.

[khardix]

So, finally back enough to respond 🙂

---

I suspect the issue might be that Node.js is not picking up the OpenSSL configuration as the default for Node.js is now to look for a configuration called nodejs_conf and will not load e.g. openssl_conf without it being changed at the configure step.

Thanks for the pointer – that does seem to help when I have my system configured to run in FIPS mode (still on Fedora):

# fips-mode-setup --check
FIPS mode is enabled.
Initramfs fips module is enabled.
The current crypto policy (FIPS) is based on the FIPS policy.

$ ./node --openssl-shared-config --force-fips -p crypto.getFips()
1
$ ./node --force-fips -p crypto.getFips()
1
$ ./node -p crypto.getFips()
1

I will update our build scripts to use the proper configure option.

---

However, this still does not help when the system is not in FIPS mode:

# fips-mode-setup --check
FIPS mode is disabled.
Initramfs fips module is enabled.
The current crypto policy (DEFAULT) neither is the FIPS policy nor is based on the FIPS policy.
Inconsistent state detected.

$ ./node --openssl-shared-config --force-fips -p 'crypto.getFips()'
./node[215531]: ../src/node.cc:1070:std::unique_ptr<node::InitializationResultImpl> node::InitializeOncePerProcessInternal(const std::vector<std::__cxx11::basic_string<char> >&, ProcessInitializationFlags::Flags): Assertion `crypto::CSPRNG(nullptr, 0).is_ok()' failed.
 1: 0xc0e160 node::Abort() [./node]
 2: 0xc0e1de  [./node]
 3: 0xbc7af0  [./node]
 4: 0xbc7ffc node::Start(int, char**) [./node]
 5: 0x7f7468849b4a  [/lib64/libc.so.6]
 6: 0x7f7468849c0b __libc_start_main [/lib64/libc.so.6]
 7: 0xb22925 _start [./node]

The main culprit seems to be that in this case, even the shared configuration makes no effort to load the fips provider.
This leads us to a situation when:

1. The fips provider is installed on the system, and thus detected in the ProcessFipsOptions() function as loadable → fips=yes property is enabled by default.
2. The provider is actually unloaded (because it was not loaded by the configuration) and the assertion fails.

The question now is what do we want to do with this. I would have thought that if we are going to provide enable/force flags for anything, it should actually enable and or force that thing, possibly even overriding any configuration file.
That would lead me to push for the removal of the unload of the fips provider as currently proposed.
Are there any downsides to doing it this way?

Alternatively, we need a better check for the availability of the fips provider, and only set the fips=yes property when it is already available in the process, without the load/unload attempt.

Thoughts? @richardlau

[richardlau]

My main concern is "the fips provider" is no longer a binary true/false. In a Linux distro like Fedora you could probably make some assumptions that, yes, there is a provider called "fips" that should be available and loaded, but in the general case it is a bad assumption that there is a provider called "fips", or (even if there is such a named provider) that it is providing FIPS capable functionality, or that some other named provider is available providing FIPS functionality.

But maybe I'm overthinking this and we can restrict what --force-fips does -- we can amend the documentation to make this only work with a provider called "fips" that is assumed to provide FIPS functionality and then make the code behave accordingly.

[khardix]

But maybe I'm overthinking this and we can restrict what --force-fips does -- we can amend the documentation to make this only work with a provider called "fips" that is assumed to provide FIPS functionality and then make the code behave accordingly.

Hm, given that it seems the entire code path is dependent on lot of assumptions that we are not comfortable making when working with libssl other than the bundled one (--shared-openssl configure option), what about not providing the --*-fips options when using shared OpenSSL? Probably not dropping them outright, but including a warning/error/deprecation notice that these are not supported in that configuration could be the best of both worlds.

My POV here is that a distribution could (and in case of Fedora/RHEL definitely does) have an opinion on how FIPS should be enabled and handled; in RHEL docs, there should currently be a paragraph strongly discouraging the user from ever touching the FIPS options on node, even should they work as assumed.

We are currently having a discussion on downstream patching the node to effectively disabling the options anyway, but this seems like a good point to ask if that could be included here as well. 😇

[richardlau]

Hm, given that it seems the entire code path is dependent on lot of assumptions that we are not comfortable making when working with libssl other than the bundled one (--shared-openssl configure option), what about not providing the --*-fips options when using shared OpenSSL? Probably not dropping them outright, but including a warning/error/deprecation notice that these are not supported in that configuration could be the best of both worlds.

My POV here is that a distribution could (and in case of Fedora/RHEL definitely does) have an opinion on how FIPS should be enabled and handled; in RHEL docs, there should currently be a paragraph strongly discouraging the user from ever touching the FIPS options on node, even should they work as assumed.

We are currently having a discussion on downstream patching the node to effectively disabling the options anyway, but this seems like a good point to ask if that could be included here as well. 😇

I'd be okay with that. When I rewrote the FIPS documentation (#48194) for this project and was testing the instructions I did observe that openssl fipsinstall is intentionally disabled in Fedora/RHEL's openssl meaning that there was no (easy) way of taking a prebuilt Node.js binary from nodejs.org and enable FIPS using the FIPS provider binaries from the system, which I think further reinforces the idea that enabling/disabling FIPS for Node.js in a Linux distribution is different from FIPS for the prebuilt community Node.js binaries.

cc @nodejs/crypto @nodejs/distros

[mhdawson]

Hm, given that it seems the entire code path is dependent on lot of assumptions that we are not comfortable making when working with libssl other than the bundled one (--shared-openssl configure option), what about not providing the --*-fips options when using shared OpenSSL? Probably not dropping them outright, but including a warning/error/deprecation notice that these are not supported in that configuration could be the best of both worlds.

Something along these lines makes sense to me. I might be SemVer major (although it would not affect shipping binaries, just people who build themselves), but I think heading down the path where configuration of FIPs when using a shared OpenSSL libary is done outside of Node.js itself makes sense.

@kapouer
@sgallagher

does that make sense to you as well?

[khardix]

Another draft for your consideration.🙂 I've added a define NODE_OPENSSL_IS_SHARED to detect if a shared OpenSSL is in use, and if so, added a warning to the initialization if any of the FIPS options are encountered. Together with the previous commit, this should mostly preserve the current behavior, while turning the assertion failure into a regular error and also pointing the user towards why things are probably broken.

A note should also be added to the docs about the fips flags status; I will add that if I get general approval on the proposed changes.

WDYT? @mhdawson @richardlau

[mhdawson]

@khardix I do think the update would be SemVer major, and would still like to get confirmation from at least one more distro that it makese sense to them as well.