Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make FIPS related options and functionality always available. #35019

Closed
wants to merge 1 commit into from
Closed

Make FIPS related options and functionality always available. #35019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 0 additions & 3 deletions node.gypi
View file
Open in desktop
Expand Up @@ -337,9 +337,6 @@
[ 'node_use_openssl=="true"', {
'defines': [ 'HAVE_OPENSSL=1' ],
'conditions': [
['openssl_fips != "" or openssl_is_fips=="true"', {
'defines': [ 'NODE_FIPS_MODE' ],
}],
[ 'node_shared_openssl=="false"', {
'dependencies': [
'./deps/openssl/openssl.gyp:openssl',
Expand Down
6 changes: 3 additions & 3 deletions src/node.cc
View file
Open in desktop
Expand Up @@ -1028,11 +1028,11 @@ InitializationResult InitializeOncePerProcess(int argc, char** argv) {
if (credentials::SafeGetenv("NODE_EXTRA_CA_CERTS", &extra_ca_certs))
crypto::UseExtraCaCerts(extra_ca_certs);
}
#ifdef NODE_FIPS_MODE
// In the case of FIPS builds we should make sure
// the random source is properly initialized first.
OPENSSL_init();
#endif // NODE_FIPS_MODE
if (FIPS_mode()) {
OPENSSL_init();
}
// V8 on Windows doesn't have a good source of entropy. Seed it from
// OpenSSL's pool.
V8::SetEntropySource(crypto::EntropySource);
Expand Down
2 changes: 0 additions & 2 deletions src/node_config.cc
View file
Open in desktop
Expand Up @@ -42,9 +42,7 @@ static void Initialize(Local<Object> target,
READONLY_FALSE_PROPERTY(target, "hasOpenSSL");
#endif // HAVE_OPENSSL

#ifdef NODE_FIPS_MODE
READONLY_TRUE_PROPERTY(target, "fipsMode");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this reflect FIPS_mode() instead of always being set to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is very good point looking into this. Do you think it would be OK to basically remove the fipsMode flag and its usage in crypto.js?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is internal it's probably okay to remove, but you'll need to update everywhere that refers to it, i.e. any of the files under lib or test that do:

const { fipsMode } = internalBinding('config');

#endif

#ifdef NODE_HAVE_I18N_SUPPORT

Expand Down
14 changes: 0 additions & 14 deletions src/node_crypto.cc
View file
Open in desktop
Expand Up @@ -3611,12 +3611,10 @@ void CipherBase::Init(const char* cipher_type,
HandleScope scope(env()->isolate());
MarkPopErrorOnReturn mark_pop_error_on_return;

#ifdef NODE_FIPS_MODE
if (FIPS_mode()) {
return env()->ThrowError(
"crypto.createCipher() is not supported in FIPS mode.");
}
#endif // NODE_FIPS_MODE

const EVP_CIPHER* const cipher = EVP_get_cipherbyname(cipher_type);
if (cipher == nullptr)
Expand Down Expand Up @@ -3802,13 +3800,11 @@ bool CipherBase::InitAuthenticated(const char* cipher_type, int iv_len,
return false;
}

#ifdef NODE_FIPS_MODE
// TODO(tniessen) Support CCM decryption in FIPS mode
if (mode == EVP_CIPH_CCM_MODE && kind_ == kDecipher && FIPS_mode()) {
env()->ThrowError("CCM decryption not supported in FIPS mode");
return false;
}
#endif

// Tell OpenSSL about the desired length.
if (!EVP_CIPHER_CTX_ctrl(ctx_.get(), EVP_CTRL_AEAD_SET_TAG, auth_tag_len,
Expand Down Expand Up @@ -4683,7 +4679,6 @@ static AllocatedBuffer Node_SignFinal(Environment* env,
}

static inline bool ValidateDSAParameters(EVP_PKEY* key) {
#ifdef NODE_FIPS_MODE
/* Validate DSA2 parameters from FIPS 186-4 */
if (FIPS_mode() && EVP_PKEY_DSA == EVP_PKEY_base_id(key)) {
DSA* dsa = EVP_PKEY_get0_DSA(key);
Expand All @@ -4699,7 +4694,6 @@ static inline bool ValidateDSAParameters(EVP_PKEY* key) {
(L == 2048 && N == 256) ||
(L == 3072 && N == 256);
}
#endif // NODE_FIPS_MODE

return true;
}
Expand Down Expand Up @@ -6859,7 +6853,6 @@ void InitCryptoOnce() {
settings = nullptr;
#endif

#ifdef NODE_FIPS_MODE
/* Override FIPS settings in cnf file, if needed. */
unsigned long err = 0; // NOLINT(runtime/int)
if (per_process::cli_options->enable_fips_crypto ||
Expand All @@ -6874,8 +6867,6 @@ void InitCryptoOnce() {
ERR_error_string(err, nullptr));
UNREACHABLE();
}
#endif // NODE_FIPS_MODE


// Turn off compression. Saves memory and protects against CRIME attacks.
// No-op with OPENSSL_NO_COMP builds of OpenSSL.
Expand Down Expand Up @@ -6920,7 +6911,6 @@ void SetEngine(const FunctionCallbackInfo<Value>& args) {
}
#endif // !OPENSSL_NO_ENGINE

#ifdef NODE_FIPS_MODE
void GetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(FIPS_mode() ? 1 : 0);
}
Expand All @@ -6938,8 +6928,6 @@ void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
return ThrowCryptoError(env, err);
}
}
#endif /* NODE_FIPS_MODE */


void Initialize(Local<Object> target,
Local<Value> unused,
Expand Down Expand Up @@ -6976,10 +6964,8 @@ void Initialize(Local<Object> target,
env->SetMethod(target, "setEngine", SetEngine);
#endif // !OPENSSL_NO_ENGINE

#ifdef NODE_FIPS_MODE
env->SetMethodNoSideEffect(target, "getFipsCrypto", GetFipsCrypto);
env->SetMethod(target, "setFipsCrypto", SetFipsCrypto);
#endif

env->SetMethod(target, "pbkdf2", PBKDF2);
env->SetMethod(target, "generateKeyPairRSA", GenerateKeyPairRSA);
Expand Down
2 changes: 0 additions & 2 deletions src/node_options.cc
View file
Open in desktop
Expand Up @@ -749,7 +749,6 @@ PerProcessOptionsParser::PerProcessOptionsParser(
&PerProcessOptions::ssl_openssl_cert_store);
Implies("--use-openssl-ca", "[ssl_openssl_cert_store]");
ImpliesNot("--use-bundled-ca", "[ssl_openssl_cert_store]");
#if NODE_FIPS_MODE
AddOption("--enable-fips",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment in the related issue

I did not really test the "vanilla" Node build, but from the above combinations
I assume that the --enable-fips option would not exist and without it,
the crypto.getFips() would still return 0, making this case equivalent to point 3.

is not correct. This change means that the option will be available in the "vanilla" node builds, but that it will fail if you try to enable it. That is likely the cause of a number of the test failures seen in the github actions runs.

@voxik, I thought the whole point was that the option would always be available for consistency across binaries, but would only work when either the binary was dynamically linked and on a system where FIPs was supported or at some point in the future if/when Node.js supports a FIPs mode again natively.

If my understanding is correct there needs to be additional documentation explaining that, both so that its clear and so that the tests will pass.

Assuming my understanding is correct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is right. I think that the comment by @richardlau about fipsMode goes closer to the root cause, because there is apparently still enough functionality controlled by build options.

"enable FIPS crypto at startup",
&PerProcessOptions::enable_fips_crypto,
Expand All @@ -758,7 +757,6 @@ PerProcessOptionsParser::PerProcessOptionsParser(
"force FIPS crypto (cannot be disabled)",
&PerProcessOptions::force_fips_crypto,
kAllowedInEnvironment);
#endif
#endif
AddOption("--use-largepages",
"Map the Node.js static code to large pages. Options are "
Expand Down
2 changes: 0 additions & 2 deletions src/node_options.h
View file
Open in desktop
Expand Up @@ -237,10 +237,8 @@ class PerProcessOptions : public Options {
#endif
bool use_openssl_ca = false;
bool use_bundled_ca = false;
#if NODE_FIPS_MODE
bool enable_fips_crypto = false;
bool force_fips_crypto = false;
#endif
#endif

// Per-process because reports can be triggered outside a known V8 context.
Expand Down