Repository navigation
Enabling FIPS mode on plain Ubuntu 22.04 and using crypto leads to infinite hang in CSPRNG #46200
Description
Activity
@nodejs/crypto
Quoting myself from an internal discussion of 5cc36c3:
FWIW this can result in an endless loop when
RAND_bytes()fails for some reason other than missing entropy, but that probably does not happen in practice.@richardlau mentioned running into this case when experimenting with FIPS.
Yes, I ran into this last year when attempting to extend #44148 to cover the FIPS provider. I got sidetracked onto other Build things, but from memory:
The issue here is that one of the RAND_* calls under the covers attempts to load an algorithm from the providers. On OpenSSL 3, Node.js'
crypto.setFips()calls EVP_default_properties_enable_fips, which effectively filters the algorithms to those from the FIPS provider:
.node/src/crypto/crypto_util.cc
Line 218 in a691002
However, it is possible to load Node.js without the FIPS provider configured (either not there at all or incorrectly configured in the openssl conf being loaded) and still enable the filter, in which case no matching algorithm will be available and we end up in the observed loop.(Note that we do not currently attempt to load the FIPS provider in
crypto.setFips()(but do if the command line options are used, although we then unload, which I'm uncertain is what we want:).)node/src/crypto/crypto_util.cc
Lines 96 to 113 in a691002
I think I looked at, or was going to look at, adding a OSSL_provider_available() (which I also remember confusingly checks if the provider is actually loaded rather than being available to load) check to
crypto.setFips(). I don't remember if that actually worked, or if I had second thoughts because theoretically you could have FIPS providers that were not called "fips" (i.e. someone added their own provider). Supporting FIPS when your FIPS provider is not called "fips" could be an edge case that we choose not to support via a Node.js API (i.e. you'd have to do it via openssl config).Or another option may have been to unconditionally load the "fips" provider (an assumption here being this is always the provider called "fips") in
crypto.setFips()when enabling fips -- I think OpenSSL does reference counting so loading the provider multiple times should be okay. This should error out in the recreate in this issue's description. I think my question then was whether disabling FIPS viacrypto.setFips()should also attempt to unload the "fips" provider.I've just written all of that and then went back to find:
@richardlau mentioned running into this case when experimenting with FIPS.
and I actually quoted an example (contrived) without FIPS being involved 😆:
$ cat test/fixtures/openssl3-conf/base_only.cnf nodejs_conf = nodejs_init [nodejs_init] providers = provider_sect # List of providers to load [provider_sect] base = base_sect [base_sect] activate = 1 $ OPENSSL_CONF=test/fixtures/openssl3-conf/base_only.cnf ./nodeSame basic cause -- the RAND_* function that is attempting to load an algorithm doesn't find a matching one and then our code continually loops. I don't recall if I identified the algorithm it was looking for.
@richardlau Sooo … I definitely don’t feel like I am familiar enough with this subject to have an opinion about how to move forward, exactly.
I don’t think this would be a full solution to this problem, but would it make sense to at least detect this specific error in the CSPRNG call and return instead of infinite looping?
diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 780dab082459..2927645e6405 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -62,9 +62,22 @@ int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) { MUST_USE_RESULT CSPRNGResult CSPRNG(void* buffer, size_t length) { do { - if (1 == RAND_status()) - if (1 == RAND_bytes(static_cast<unsigned char*>(buffer), length)) + if (1 == RAND_status()) { + if (1 == RAND_bytes(static_cast<unsigned char*>(buffer), length)) { return {true}; + } else { +#if OPENSSL_VERSION_MAJOR >= 3 + auto code = ERR_peek_last_error(); + // A misconfigured OpenSSL 3 installation may report 1 from RAND_poll() + // and RAND_status() but fail in RAND_bytes() if it cannot look up + // a matching algorithm for the CSPRNG. + if (ERR_GET_LIB(code) == ERR_LIB_RAND && + ERR_GET_REASON(code) == RAND_R_UNABLE_TO_FETCH_DRBG) { + return {false}; + } + } +#endif + } } while (1 == RAND_poll()); return {false};
or would that be too naïve? I think for me it would solve the problem because then it would be possible to try to see what
crypto.randomBytes()does immediately after enabling FIPS and seeing if it works, which is at least easily detectable, unlike an infinite loop.@addaleax I've been playing around with something similar: richardlau@def2b01
(I like your version of using `ERR_GET_LIB` and `ERR_GET_REASON` more.) Hadn't opened the PR yet because I was still working on the testcases (the one in my commit doesn't work properly if FIPS is enabled).Details
diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc index 780dab082459..be467457887d 100644 --- a/src/crypto/crypto_util.cc +++ b/src/crypto/crypto_util.cc @@ -62,6 +62,12 @@ int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) { MUST_USE_RESULT CSPRNGResult CSPRNG(void* buffer, size_t length) { do { +#if OPENSSL_VERSION_MAJOR >= 3 + const uint32_t err = ERR_peek_error(); + if (err == ERR_PACK(ERR_LIB_RAND, 0, RAND_R_UNABLE_TO_FETCH_DRBG)) { + return {false}; + } +#endif if (1 == RAND_status()) if (1 == RAND_bytes(static_cast<unsigned char*>(buffer), length)) return {true};
I'm a bit unsure myself whether this checking for this specific error is distinct enough from the missing entropy case the loop is meant to address, but I'm also tending towards having a detectable error in these cases.
Reacted by Anna HenningsenI'm a bit unsure myself whether this checking for this specific error is distinct enough from the missing entropy case the loop is meant to address
I’d actually feel like in an ideal world, we’d be checking for the missing-entropy error and only in that case continue to run the loop, but at the same time that comes with a larger risk of unintentional breakage (or at least it feels like that to me).
Reacted by Richard Lau- added a commit that references this issue
on Jan 19, 2023 - added a commit that references this issue
on Jan 20, 2023 - added 2 commits that reference this issue
on Mar 3, 2023 - added a commit that references this issue
on Mar 27, 2023
Version
v18.13.0, v19.4.0, main
Platform
Ubuntu 22.04 without modifications; Linux desktop-ua 5.15.0-57-generic #63-Ubuntu SMP Thu Nov 24 13:43:17 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
crypto
What steps will reproduce the bug?
Dockerfile
How often does it reproduce? Is there a required condition?
Always. No.
What is the expected behavior?
Some type of error indicating that OpenSSL is not configured properly for FIPS mode on the machine, which I assume this is the root cause here.
(I am not expecting this to really work and give me random bytes.)
What do you see instead?
Infinite hang.
Additional information
I think this is a problem that other people have run into before, e.g. #38633 (review) cc @danbev @richardlau
In the debugger, it’s visible that
RAND_pollandRAND_statuskeep returning1butRAND_byteskeeps returning0(code).