From 901e926d43a875af0bf2164c62770f7a835edf19 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Fri, 27 Jan 2017 11:49:14 -0800 Subject: [PATCH 1/2] src: add SafeGetenv() to internal API Allow it to be used anywhere in src/ that env variables with security implications are accessed. --- src/node.cc | 2 +- src/node_internals.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index 8c5e811d6e9130..d57665a9fd53ae 100644 --- a/src/node.cc +++ b/src/node.cc @@ -924,7 +924,7 @@ Local UVException(Isolate* isolate, // Look up environment variable unless running as setuid root. -inline bool SafeGetenv(const char* key, std::string* text) { +bool SafeGetenv(const char* key, std::string* text) { #ifndef _WIN32 // TODO(bnoordhuis) Should perhaps also check whether getauxval(AT_SECURE) // is non-zero on Linux. diff --git a/src/node_internals.h b/src/node_internals.h index 9d57becc26ceb9..52fa17a8129168 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -114,6 +114,8 @@ void RegisterSignalHandler(int signal, bool reset_handler = false); #endif +bool SafeGetenv(const char* key, std::string* text); + template constexpr size_t arraysize(const T(&)[N]) { return N; } From 59afa275adc8c91d564bdde92390dd9341c919c8 Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Wed, 25 Jan 2017 14:13:34 -0800 Subject: [PATCH 2/2] crypto: support OPENSSL_CONF again A side-effect of https://github.com/nodejs/node-private/pull/82 was to remove support for OPENSSL_CONF, as well as removing the default read of a configuration file on startup. Partly revert this, allowing OPENSSL_CONF to be used to specify a configuration file to read on startup, but do not read a file by default. If the --openssl-config command line option is provided, its value is used, not the OPENSSL_CONF environment variable. Fix: https://github.com/nodejs/node/issues/10938 PR-URL: https://github.com/nodejs/node/pull/11006 Reviewed-By: Michael Dawson Reviewed-By: Ben Noordhuis --- doc/api/cli.md | 13 +++++++++++++ doc/node.1 | 10 ++++++++++ src/node.cc | 14 ++++++++++---- src/node_crypto.cc | 4 ++-- src/node_internals.h | 2 +- test/parallel/test-crypto-fips.js | 25 ++++++++++++++++++++++--- 6 files changed, 58 insertions(+), 10 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index aee7acbca00ca3..f3d3e880c55671 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -387,6 +387,18 @@ misformatted, but any errors are otherwise ignored. Note that neither the well known nor extra certificates are used when the `ca` options property is explicitly specified for a TLS or HTTPS client or server. +### `OPENSSL_CONF=file` + + +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`. + +If the [`--openssl-config`][] command line option is used, the environment +variable is ignored. + ### `SSL_CERT_DIR=dir` If `--use-openssl-ca` is enabled, this overrides and sets OpenSSL's directory @@ -421,3 +433,4 @@ equivalent to using the `--redirect-warnings=file` command-line flag. [debugger]: debugger.html [REPL]: repl.html [SlowBuffer]: buffer.html#buffer_class_slowbuffer +[`--openssl-config`]: #cli_openssl_config_file diff --git a/doc/node.1 b/doc/node.1 index fe1b0e7fa1aba3..9aa514ccf36f30 100644 --- a/doc/node.1 +++ b/doc/node.1 @@ -256,6 +256,16 @@ asynchronous when outputting to a TTY on platforms which support async stdio. Setting this will void any guarantee that stdio will not be interleaved or dropped at program exit. \fBAvoid use.\fR +.TP +.BR OPENSSL_CONF = \fIfile\fR +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 +\fB./configure \-\-openssl\-fips\fR. + +If the +\fB\-\-openssl\-config\fR +command line option is used, the environment variable is ignored. + .TP .BR SSL_CERT_DIR = \fIdir\fR If \fB\-\-use\-openssl\-ca\fR is enabled, this overrides and sets OpenSSL's directory diff --git a/src/node.cc b/src/node.cc index d57665a9fd53ae..040f9f597ad559 100644 --- a/src/node.cc +++ b/src/node.cc @@ -176,7 +176,7 @@ bool ssl_openssl_cert_store = bool enable_fips_crypto = false; bool force_fips_crypto = false; # endif // NODE_FIPS_MODE -const char* openssl_config = nullptr; +std::string openssl_config; // NOLINT(runtime/string) #endif // HAVE_OPENSSL // true if process warnings should be suppressed @@ -3561,8 +3561,9 @@ static void PrintHelp() { " --enable-fips enable FIPS crypto at startup\n" " --force-fips force FIPS crypto (cannot be disabled)\n" #endif /* NODE_FIPS_MODE */ - " --openssl-config=path load OpenSSL configuration file from\n" - " the specified path\n" + " --openssl-config=file load OpenSSL configuration from the\n" + " specified file (overrides\n" + " OPENSSL_CONF)\n" #endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) " --icu-data-dir=dir set ICU data load path to dir\n" @@ -3597,6 +3598,8 @@ static void PrintHelp() { " file\n" "NODE_REDIRECT_WARNINGS write warnings to path instead of\n" " stderr\n" + "OPENSSL_CONF load OpenSSL configuration from file\n" + "\n" "Documentation can be found at https://nodejs.org/\n"); } @@ -3746,7 +3749,7 @@ static void ParseArgs(int* argc, force_fips_crypto = true; #endif /* NODE_FIPS_MODE */ } else if (strncmp(arg, "--openssl-config=", 17) == 0) { - openssl_config = arg + 17; + openssl_config.assign(arg + 17); #endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) } else if (strncmp(arg, "--icu-data-dir=", 15) == 0) { @@ -4246,6 +4249,9 @@ void Init(int* argc, if (config_warning_file.empty()) SafeGetenv("NODE_REDIRECT_WARNINGS", &config_warning_file); + if (openssl_config.empty()) + SafeGetenv("OPENSSL_CONF", &openssl_config); + // Parse a few arguments which are specific to Node. int v8_argc; const char** v8_argv; diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 060cbe725faf65..27361178a490d6 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5893,14 +5893,14 @@ void InitCryptoOnce() { OPENSSL_no_config(); // --openssl-config=... - if (openssl_config != nullptr) { + if (!openssl_config.empty()) { OPENSSL_load_builtin_modules(); #ifndef OPENSSL_NO_ENGINE ENGINE_load_builtin_engines(); #endif ERR_clear_error(); CONF_modules_load_file( - openssl_config, + openssl_config.c_str(), nullptr, CONF_MFLAGS_DEFAULT_SECTION); int err = ERR_get_error(); diff --git a/src/node_internals.h b/src/node_internals.h index 52fa17a8129168..64d82deec1b050 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -37,7 +37,7 @@ namespace node { // Set in node.cc by ParseArgs with the value of --openssl-config. // Used in node_crypto.cc when initializing OpenSSL. -extern const char* openssl_config; +extern std::string openssl_config; // Set in node.cc by ParseArgs when --preserve-symlinks is used. // Used in node_config.cc to set a constant on process.binding('config') diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index 6fd3352740c8c3..ae510741943ca8 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -37,8 +37,9 @@ function testHelper(stream, args, expectedOutput, cmd, env) { env: env }); - console.error('Spawned child [pid:' + child.pid + '] with cmd ' + - cmd + ' and args \'' + args + '\''); + console.error('Spawned child [pid:' + child.pid + '] with cmd \'' + + cmd + '\' expect %j with args \'' + args + '\'' + + ' OPENSSL_CONF=%j', expectedOutput, env.OPENSSL_CONF); function childOk(child) { console.error('Child #' + ++num_children_ok + @@ -92,10 +93,26 @@ testHelper( compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, 'require("crypto").fips', process.env); -// OPENSSL_CONF should _not_ be able to turn on FIPS mode + +// OPENSSL_CONF should be able to turn on FIPS mode testHelper( 'stdout', [], + compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, + 'require("crypto").fips', + addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); + +// --openssl-config option should override OPENSSL_CONF +testHelper( + 'stdout', + [`--openssl-config=${CNF_FIPS_ON}`], + compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, + 'require("crypto").fips', + addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); + +testHelper( + 'stdout', + [`--openssl-config=${CNF_FIPS_OFF}`], FIPS_DISABLED, 'require("crypto").fips', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); @@ -107,6 +124,7 @@ testHelper( compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING, 'require("crypto").fips', process.env); + // OPENSSL_CONF should _not_ make a difference to --enable-fips testHelper( compiledWithFips() ? 'stdout' : 'stderr', @@ -122,6 +140,7 @@ testHelper( compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING, 'require("crypto").fips', process.env); + // Using OPENSSL_CONF should not make a difference to --force-fips testHelper( compiledWithFips() ? 'stdout' : 'stderr',