using --enable-fips or --force-fips with crypto.fips=1 fails

[stevewallin]

• Version:6.1.0.0
• Platform:Linux 8df1860c1f5b 4.9.12-moby deps: update openssl to 1.0.1j #1 SMP Tue Feb 28 12:11:36 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem:crypto

When using a fips enabled build of node:

• using only application control of fips with crypto.fips = 0 or 1 works as expected
• --enable-fips and crypto.fips=0 - working as expected (FIPS disabled)
• --force-fips and crypto.fips=0 - working as expected with following error

crypto.fips = 0;
            ^

Error: Cannot set FIPS mode, it was forced with --force-fips at startup.)
with --enable-fips or --force-fips and crypto.fips = 1 the app fails (
crypto.fips = 1;
            ^

Error: error:2D078072:FIPS routines:FIPS_module_mode_set:fips mode already set

I'm trying to build a migration plan for adopting fips and having both crypto.fips = 1 in the application and --enable-fips or --force-fips seems like a valid use case to me ?

[mscdex]

/cc @nodejs/crypto

[indutny]

cc @mhdawson

[stevewallin]

also crypto.getHashes() and .getCiphers() return the same list with fips disabled and enabled. I'm expecting to be able to get the fips subset of available algorithms somehow ?

[mhdawson]

Steve had mentioned this to me, its known that there could be an improvement in this area.

[stevewallin]

Michael provided this work round to protect the app from failing when using fips = 1 and the command line options...

if (crypto.fips === 0) { crypto.fips = 1; }

to test if fips is off before setting so doesn't try to override in the failing case

[mhdawson]

I'd have to check the code on the Node.js side, but my first guess is that we get those from openssl and openssl is not very good at filtering/failing gracefully when algs are disabled when FIPs is enabled.

We did fix up a few of the low hanging cases in terms of returning better errors, but still far from optimal.

[bnoordhuis]

I didn't test but wouldn't this patch resolve it? It calls FIPS_mode_set() only when the desired mode is different from the active mode.

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 7005080..4b0e405 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -6002,21 +6002,24 @@ void GetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
   } else {
     args.GetReturnValue().Set(0);
   }
 }
 
 void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 #ifdef NODE_FIPS_MODE
-  bool mode = args[0]->BooleanValue();
+  const bool enabled = FIPS_mode();
+  const bool enable = args[0]->BooleanValue();
+  if (enable == enabled)
+    return;  // No action needed.
   if (force_fips_crypto) {
     return env->ThrowError(
         "Cannot set FIPS mode, it was forced with --force-fips at startup.");
-  } else if (!FIPS_mode_set(mode)) {
+  } else if (!FIPS_mode_set(enable)) {
     unsigned long err = ERR_get_error();  // NOLINT(runtime/int)
     return ThrowCryptoError(env, err);
   }
 #else
   return env->ThrowError("Cannot set FIPS mode in a non-FIPS build.");
 #endif /* NODE_FIPS_MODE */
 }
 

[stevewallin]

it does look like we pass through the request to openssl in /src/node_crypto.cc and so may not be easily filtered to the current available set

void GetHashes(const FunctionCallbackInfo<Value>& args) {
  Environment* env = Environment::GetCurrent(args);
  CipherPushContext ctx(env);
  EVP_MD_do_all_sorted(array_push_back<EVP_MD>, &ctx);
  args.GetReturnValue().Set(ctx.arr);
}

[gibfahn]

#12210 raised with @bnoordhuis's patch. Note that this only fixes the original issue raised in this PR. If that lands I'll reopen to address the second issue:

also crypto.getHashes() and .getCiphers() return the same list with fips disabled and enabled. I'm expecting to be able to get the fips subset of available algorithms somehow ?

[Trott]

@gibfahn Are you still planning to get the resolution to these two issues across the finish line? If so, maybe let's add you as the assignee? And if not, perhaps we can add a help wanted label?

[gibfahn]

Reopening, the first issue was fixed but I don't think there's a good way to get the list of available ciphers.

also crypto.getHashes() and .getCiphers() return the same list with fips disabled and enabled. I'm expecting to be able to get the fips subset of available algorithms somehow ?

[joaocgreis]

This got closed by updating master in a private repo. Thanks GitHub.