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

[v14.x backport] crypto: make FIPS related options always awailable #40241

Closed
wants to merge 4 commits into from
Closed

[v14.x backport] crypto: make FIPS related options always awailable #40241

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b35f7b
crypto: make FIPS related options always awailable
voxik Aug 25, 2020
9f7f84c
squash! crypto: make FIPS related options always awailable
danbev Sep 28, 2021
c59618a
squash! crypto: make FIPS related options always awailable
danbev Sep 29, 2021
e942e28
squash! crypto: make FIPS related options always awailable
danbev Sep 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions doc/api/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ priority than `--dns-result-order`.
added: v6.0.0
-->

Enable FIPS-compliant crypto at startup. (Requires Node.js to be built with
`./configure --openssl-fips`.)
Enable FIPS-compliant crypto at startup. (Requires Node.js to be built
against FIPS-compatible OpenSSL.)

### `--enable-source-maps`
<!-- YAML
Expand Down Expand Up @@ -623,8 +623,8 @@ added: v6.9.0
-->

Load an OpenSSL configuration file on startup. Among other uses, this can be
used to enable FIPS-compliant crypto if Node.js is built with
`./configure --openssl-fips`.
used to enable FIPS-compliant crypto if Node.js is built
Copy link
Member

Choose a reason for hiding this comment

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

Will ./configure --openssl-fips still be accepted. Want to understand if this could break existing workflows/builds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it will still be accepted at configuration time to specify the directory where the FIPS fipscanister.o is located.

Copy link
Member

Choose a reason for hiding this comment

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

@mhdawson The ubi81_sharedlibs_openssl111fips_x64 CI job runs with configure --openssl-is-fips (we didn't update the job when #36341 landed).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the quick confirmation.

Copy link
Member

Choose a reason for hiding this comment

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

ah sorry confused --open-is-fips with --openssl-fips configure options.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think --openssl-fips works before on Node.js 14 based on configure.py?

node/configure.py

Lines 1382 to 1383 in 95b9240

if options.openssl_fips or options.openssl_fips == '':
error('FIPS is not supported in this version of Node.js')

(--openssl-is-fips does work and is tested in the CI.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I recall correctly it is not possible to enable FIPS with the OpenSSL version that is statically linked with Node.js. This is still the case with upstream/master.
But it is possible to dynamically link to an OpenSSL version that does support FIPS (which is the use case at Red Hat).

Copy link
Member

Choose a reason for hiding this comment

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

@danbev that is my understanding as well.

against FIPS-enabled OpenSSL.

### `--pending-deprecation`
<!-- YAML
Expand Down
22 changes: 4 additions & 18 deletions lib/crypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,10 @@ assertCrypto();

const {
ERR_CRYPTO_FIPS_FORCED,
ERR_CRYPTO_FIPS_UNAVAILABLE
} = require('internal/errors').codes;
const constants = internalBinding('constants').crypto;
const { getOptionValue } = require('internal/options');
const pendingDeprecation = getOptionValue('--pending-deprecation');
const { fipsMode } = internalBinding('config');
const fipsForced = getOptionValue('--force-fips');
const {
getFipsCrypto,
Expand Down Expand Up @@ -193,10 +191,8 @@ module.exports = {
sign: signOneShot,
setEngine,
timingSafeEqual,
getFips: !fipsMode ? getFipsDisabled :
fipsForced ? getFipsForced : getFipsCrypto,
setFips: !fipsMode ? setFipsDisabled :
fipsForced ? setFipsForced : setFipsCrypto,
getFips: fipsForced ? getFipsForced : getFipsCrypto,
setFips: fipsForced ? setFipsForced : setFipsCrypto,
verify: verifyOneShot,

// Classes
Expand All @@ -215,19 +211,11 @@ module.exports = {
Verify
};

function setFipsDisabled() {
throw new ERR_CRYPTO_FIPS_UNAVAILABLE();
}

function setFipsForced(val) {
if (val) return;
throw new ERR_CRYPTO_FIPS_FORCED();
}

function getFipsDisabled() {
return 0;
}

function getFipsForced() {
return 1;
}
Expand All @@ -249,10 +237,8 @@ ObjectDefineProperties(module.exports, {
},
// crypto.fips is deprecated. DEP0093. Use crypto.getFips()/crypto.setFips()
fips: {
get: !fipsMode ? getFipsDisabled :
fipsForced ? getFipsForced : getFipsCrypto,
set: !fipsMode ? setFipsDisabled :
fipsForced ? setFipsForced : setFipsCrypto
get: fipsForced ? getFipsForced : getFipsCrypto,
set: fipsForced ? setFipsForced : setFipsCrypto
},
DEFAULT_ENCODING: {
enumerable: false,
Expand Down
3 changes: 0 additions & 3 deletions node.gypi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,9 +319,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
Original file line number Diff line number Diff line change
Expand Up @@ -1040,11 +1040,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
Original file line number Diff line number Diff line change
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");
#endif

#ifdef NODE_HAVE_I18N_SUPPORT

Expand Down
28 changes: 16 additions & 12 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@
#ifndef OPENSSL_NO_ENGINE
# include <openssl/engine.h>
#endif // !OPENSSL_NO_ENGINE

#ifdef OPENSSL_FIPS
# include <openssl/fips.h>
#endif // OPENSSL_FIPS

#include <openssl/evp.h>
#include <openssl/pem.h>
#include <openssl/x509v3.h>
Expand Down Expand Up @@ -183,6 +188,16 @@ static int PasswordCallback(char* buf, int size, int rwflag, void* u) {
return -1;
}

void TestFipsCrypto(const v8::FunctionCallbackInfo<v8::Value>& args) {
#ifdef OPENSSL_FIPS
const auto enabled = FIPS_selftest() ? 1 : 0;
#else // OPENSSL_FIPS
const auto enabled = 0;
#endif // OPENSSL_FIPS

args.GetReturnValue().Set(enabled);
}

// Loads OpenSSL engine by engine id and returns it. The loaded engine
// gets a reference so remember the corresponding call to ENGINE_free.
// In case of error the appropriate js exception is scheduled
Expand Down Expand Up @@ -3618,12 +3633,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 @@ -3809,13 +3822,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 @@ -4690,7 +4701,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 @@ -4706,7 +4716,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 @@ -6866,7 +6875,6 @@ void InitCryptoOnce() {
settings = nullptr;
#endif

#ifdef NODE_FIPS_MODE
Copy link
Member

Choose a reason for hiding this comment

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

There were changes in the original PR that don't seem to have made it into this block. In crypto_util.cc ->

diff --git a/src/crypto/crypto_util.cc b/src/crypto/crypto_util.cc
index 34058d2dcae9..0d44d0bb4e16 100644
--- a/src/crypto/crypto_util.cc
+++ b/src/crypto/crypto_util.cc
@@ -142,10 +142,9 @@ void InitCryptoOnce() {
     }
   }
   if (0 != err) {
-    fprintf(stderr,
-            "openssl fips failed: %s\n",
-            ERR_error_string(err, nullptr));
-    UNREACHABLE();
+    auto* isolate = Isolate::GetCurrent();
+    auto* env = Environment::GetCurrent(isolate);
+    return ThrowCryptoError(env, err);
   }

@danbev was that intentional?

Copy link
Contributor Author

@danbev danbev Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That was not intentional 😞 I'll take a closer look add it tomorrow. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a commit with the missing code now, please take a look.

/* 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 @@ -6881,7 +6889,6 @@ void InitCryptoOnce() {
ERR_error_string(err, nullptr));
UNREACHABLE();
}
#endif // NODE_FIPS_MODE


// Turn off compression. Saves memory and protects against CRIME attacks.
Expand Down Expand Up @@ -6927,7 +6934,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 @@ -6945,7 +6951,6 @@ void SetFipsCrypto(const FunctionCallbackInfo<Value>& args) {
return ThrowCryptoError(env, err);
}
}
#endif /* NODE_FIPS_MODE */

namespace {
// SecureBuffer uses openssl to allocate a Uint8Array using
Expand Down Expand Up @@ -7013,10 +7018,9 @@ 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->SetMethodNoSideEffect(target, "testFipsCrypto", TestFipsCrypto);

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
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,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",
"enable FIPS crypto at startup",
&PerProcessOptions::enable_fips_crypto,
Expand All @@ -775,7 +774,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
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,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
7 changes: 2 additions & 5 deletions test/parallel/test-cli-node-print-help.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@ const common = require('../common');

const assert = require('assert');
const { exec } = require('child_process');
const { internalBinding } = require('internal/test/binding');
const { fipsMode } = internalBinding('config');
let stdOut;


Expand All @@ -28,9 +26,8 @@ function validateNodePrintHelp() {
const cliHelpOptions = [
{ compileConstant: HAVE_OPENSSL,
flags: [ '--openssl-config=...', '--tls-cipher-list=...',
'--use-bundled-ca', '--use-openssl-ca' ] },
{ compileConstant: fipsMode,
flags: [ '--enable-fips', '--force-fips' ] },
'--use-bundled-ca', '--use-openssl-ca',
'--enable-fips', '--force-fips' ] },
{ compileConstant: NODE_HAVE_I18N_SUPPORT,
flags: [ '--icu-data-dir=...', 'NODE_ICU_DATA' ] },
{ compileConstant: HAVE_INSPECTOR,
Expand Down
Loading