From 20d819415df283bc754d9dc4440d0df1cdd40546 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Fri, 22 Jan 2016 18:10:09 -0500 Subject: [PATCH 01/14] crypto: Improve control of FIPS mode Default to FIPS off even in FIPS builds. Add JS API to check and control FIPS mode. Add command line arguments to force FIPS on/off. Respect OPENSSL_CONF variable and read the config. Add testing for new features. --- doc/api/crypto.markdown | 8 + lib/crypto.js | 9 + src/node.cc | 20 +- src/node.h | 4 + src/node_crypto.cc | 77 ++++++-- test/common.js | 6 - test/fixtures/openssl_fips_disabled.cnf | 12 ++ test/fixtures/openssl_fips_enabled.cnf | 12 ++ test/parallel/test-crypto-authenticated.js | 4 +- test/parallel/test-crypto-binary-default.js | 6 +- test/parallel/test-crypto-cipher-decipher.js | 6 +- test/parallel/test-crypto-dh-odd-key.js | 2 +- test/parallel/test-crypto-dh.js | 2 +- test/parallel/test-crypto-ecb.js | 2 +- test/parallel/test-crypto-fips.js | 194 +++++++++++++++++++ test/parallel/test-crypto-hash.js | 2 +- test/parallel/test-crypto-hmac.js | 4 +- test/parallel/test-crypto-stream.js | 2 +- test/parallel/test-crypto.js | 4 +- test/parallel/test-dsa-fips-invalid-key.js | 9 +- test/parallel/test-tls-ocsp-callback.js | 2 +- test/pummel/test-crypto-dh.js | 6 +- 22 files changed, 342 insertions(+), 51 deletions(-) create mode 100644 test/fixtures/openssl_fips_disabled.cnf create mode 100644 test/fixtures/openssl_fips_enabled.cnf create mode 100644 test/parallel/test-crypto-fips.js diff --git a/doc/api/crypto.markdown b/doc/api/crypto.markdown index 72f241b03acb19..8cdd8f0713e5df 100644 --- a/doc/api/crypto.markdown +++ b/doc/api/crypto.markdown @@ -815,6 +815,14 @@ with legacy programs that expect `'binary'` to be the default encoding. New applications should expect the default to be `'buffer'`. This property may become deprecated in a future Node.js release. +### crypto.hasFipsCrypto() + +Identifies whether a FIPS compliant crypto provider is currently in use. + +### crypto.setFipsCrypto(mode) + +Sets FIPS compliant cryptographic provider on or off. + ### crypto.createCipher(algorithm, password) Creates and returns a `Cipher` object that uses the given `algorithm` and diff --git a/lib/crypto.js b/lib/crypto.js index 4b0539406e5ac6..3e414a693105f6 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -11,6 +11,8 @@ try { var getCiphers = binding.getCiphers; var getHashes = binding.getHashes; var getCurves = binding.getCurves; + var hasFipsCrypto = binding.hasFipsCrypto; + var setFipsCrypto = binding.setFipsCrypto; } catch (e) { throw new Error('Node.js is not compiled with openssl crypto support'); } @@ -645,6 +647,13 @@ exports.getCurves = function() { return filterDuplicates(getCurves()); }; +exports.hasFipsCrypto = function() { + return hasFipsCrypto(); +}; + +exports.setFipsCrypto = function(mode) { + return setFipsCrypto(mode); +}; function filterDuplicates(names) { // Drop all-caps names in favor of their lowercase aliases, diff --git a/src/node.cc b/src/node.cc index 05984745de080f..e61bc21b9bd4c8 100644 --- a/src/node.cc +++ b/src/node.cc @@ -161,6 +161,12 @@ static const char* icu_data_dir = nullptr; // used by C++ modules as well bool no_deprecation = false; +#if HAVE_OPENSSL && NODE_FIPS_MODE +// used by crypto module +bool disable_fips_crypto = false; +bool enable_fips_crypto = false; +#endif + // process-relative uptime base, initialized at start-up static double prog_start_time; static bool debugger_running; @@ -3282,7 +3288,11 @@ static void PrintHelp() { " --v8-options print v8 command line options\n" #if HAVE_OPENSSL " --tls-cipher-list=val use an alternative default TLS cipher list\n" -#endif +#if NODE_FIPS_MODE + " --disable-fips force use of crypto without FIPS compliance" + " --enable-fips force use of crypto with FIPS compliance" +#endif /* NODE_FIPS_MODE */ +#endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) " --icu-data-dir=dir set ICU data load path to dir\n" " (overrides NODE_ICU_DATA)\n" @@ -3424,7 +3434,13 @@ static void ParseArgs(int* argc, #if HAVE_OPENSSL } else if (strncmp(arg, "--tls-cipher-list=", 18) == 0) { default_cipher_list = arg + 18; -#endif +#if NODE_FIPS_MODE + } else if (strcmp(arg, "--disable-fips") == 0) { + disable_fips_crypto = true; + } else if (strcmp(arg, "--enable-fips") == 0) { + enable_fips_crypto = true; +#endif /* NODE_FIPS_MODE */ +#endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) } else if (strncmp(arg, "--icu-data-dir=", 15) == 0) { icu_data_dir = arg + 15; diff --git a/src/node.h b/src/node.h index ef1f629d20aa0e..479f74fced7e1e 100644 --- a/src/node.h +++ b/src/node.h @@ -179,6 +179,10 @@ typedef intptr_t ssize_t; namespace node { NODE_EXTERN extern bool no_deprecation; +#if HAVE_OPENSSL && NODE_FIPS_MODE +NODE_EXTERN extern bool disable_fips_crypto; +NODE_EXTERN extern bool enable_fips_crypto; +#endif NODE_EXTERN int Start(int argc, char *argv[]); NODE_EXTERN void Init(int* argc, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 5fe3632b109fa7..b331b698f3eaf2 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3857,24 +3857,26 @@ SignBase::Error Sign::SignFinal(const char* key_pem, goto exit; #ifdef NODE_FIPS_MODE - /* Validate DSA2 parameters from FIPS 186-4 */ - if (EVP_PKEY_DSA == pkey->type) { - size_t L = BN_num_bits(pkey->pkey.dsa->p); - size_t N = BN_num_bits(pkey->pkey.dsa->q); - bool result = false; - - if (L == 1024 && N == 160) - result = true; - else if (L == 2048 && N == 224) - result = true; - else if (L == 2048 && N == 256) - result = true; - else if (L == 3072 && N == 256) - result = true; - - if (!result) { - fatal = true; - goto exit; + if (FIPS_mode()) { + /* Validate DSA2 parameters from FIPS 186-4 */ + if (EVP_PKEY_DSA == pkey->type) { + size_t L = BN_num_bits(pkey->pkey.dsa->p); + size_t N = BN_num_bits(pkey->pkey.dsa->q); + bool result = false; + + if (L == 1024 && N == 160) + result = true; + else if (L == 2048 && N == 224) + result = true; + else if (L == 2048 && N == 256) + result = true; + else if (L == 3072 && N == 256) + result = true; + + if (!result) { + fatal = true; + goto exit; + } } } #endif // NODE_FIPS_MODE @@ -5665,14 +5667,21 @@ void InitCryptoOnce() { SSL_library_init(); OpenSSL_add_all_algorithms(); SSL_load_error_strings(); + OPENSSL_config(NULL); crypto_lock_init(); CRYPTO_set_locking_callback(crypto_lock_cb); CRYPTO_THREADID_set_callback(crypto_threadid_cb); #ifdef NODE_FIPS_MODE - if (!FIPS_mode_set(1)) { - int err = ERR_get_error(); + /* Override FIPS settings in cnf file. */ + int err = 0; + if (disable_fips_crypto && !FIPS_mode_set(0)) { + err = ERR_get_error(); + } else if (!disable_fips_crypto && enable_fips_crypto && !FIPS_mode_set(1)) { + err = ERR_get_error(); + } + if (0 != err) { fprintf(stderr, "openssl fips failed: %s\n", ERR_error_string(err, NULL)); UNREACHABLE(); } @@ -5739,6 +5748,32 @@ void SetEngine(const FunctionCallbackInfo& args) { } #endif // !OPENSSL_NO_ENGINE +void HasFipsCrypto(const FunctionCallbackInfo& args) { + if (FIPS_mode()) { + args.GetReturnValue().Set(1); + } else { + args.GetReturnValue().Set(0); + } +} + +void SetFipsCrypto(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); +#ifdef NODE_FIPS_MODE + bool mode = args[0]->BooleanValue(); + if (disable_fips_crypto) { + return env->ThrowError( + "Cannot set FIPS mode, it was forced with --disable-fips at startup."); + } else if (enable_fips_crypto) { + return env->ThrowError( + "Cannot set FIPS mode, it was forced with --enable-fips at startup."); + } else if (!FIPS_mode_set(mode)) { + unsigned long err = ERR_get_error(); + return ThrowCryptoError(env, err); + } +#else + return env->ThrowError("Cannot set FIPS mode in a non-FIPS build."); +#endif /* NODE_FIPS_MODE */ +} // FIXME(bnoordhuis) Handle global init correctly. void InitCrypto(Local target, @@ -5763,6 +5798,8 @@ void InitCrypto(Local target, #ifndef OPENSSL_NO_ENGINE env->SetMethod(target, "setEngine", SetEngine); #endif // !OPENSSL_NO_ENGINE + env->SetMethod(target, "hasFipsCrypto", HasFipsCrypto); + env->SetMethod(target, "setFipsCrypto", SetFipsCrypto); env->SetMethod(target, "PBKDF2", PBKDF2); env->SetMethod(target, "randomBytes", RandomBytes); env->SetMethod(target, "getSSLCiphers", GetSSLCiphers); diff --git a/test/common.js b/test/common.js index b7c64d2852af13..764ab13835b0c1 100644 --- a/test/common.js +++ b/test/common.js @@ -159,12 +159,6 @@ Object.defineProperty(exports, 'hasCrypto', { } }); -Object.defineProperty(exports, 'hasFipsCrypto', { - get: function() { - return process.config.variables.openssl_fips ? true : false; - } -}); - if (exports.isWindows) { exports.PIPE = '\\\\.\\pipe\\libuv-test'; if (process.env.TEST_THREAD_ID) { diff --git a/test/fixtures/openssl_fips_disabled.cnf b/test/fixtures/openssl_fips_disabled.cnf new file mode 100644 index 00000000000000..8668370fac52f7 --- /dev/null +++ b/test/fixtures/openssl_fips_disabled.cnf @@ -0,0 +1,12 @@ +# Skeleton openssl.cnf for testing with FIPS + +openssl_conf = openssl_conf_section +authorityKeyIdentifier=keyid:always,issuer:always + +[openssl_conf_section] + # Configuration module list +alg_section = evp_sect + +[ evp_sect ] +# Set to "yes" to enter FIPS mode if supported +fips_mode = no diff --git a/test/fixtures/openssl_fips_enabled.cnf b/test/fixtures/openssl_fips_enabled.cnf new file mode 100644 index 00000000000000..9c1a90f5087727 --- /dev/null +++ b/test/fixtures/openssl_fips_enabled.cnf @@ -0,0 +1,12 @@ +# Skeleton openssl.cnf for testing with FIPS + +openssl_conf = openssl_conf_section +authorityKeyIdentifier=keyid:always,issuer:always + +[openssl_conf_section] + # Configuration module list +alg_section = evp_sect + +[ evp_sect ] +# Set to "yes" to enter FIPS mode if supported +fips_mode = yes diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js index fa9a78c26e5b59..2146c014ccc69e 100644 --- a/test/parallel/test-crypto-authenticated.js +++ b/test/parallel/test-crypto-authenticated.js @@ -93,7 +93,7 @@ for (var i in TEST_CASES) { (function() { if (!test.password) return; - if (common.hasFipsCrypto) { + if (crypto.hasFipsCrypto()) { assert.throws(function() { crypto.createCipher(test.algo, test.password); }, /not supported in FIPS mode/); @@ -114,7 +114,7 @@ for (var i in TEST_CASES) { (function() { if (!test.password) return; - if (common.hasFipsCrypto) { + if (crypto.hasFipsCrypto()) { assert.throws(function() { crypto.createDecipher(test.algo, test.password); }, /not supported in FIPS mode/); diff --git a/test/parallel/test-crypto-binary-default.js b/test/parallel/test-crypto-binary-default.js index 3cb98db80aca55..299559f050ecc2 100644 --- a/test/parallel/test-crypto-binary-default.js +++ b/test/parallel/test-crypto-binary-default.js @@ -345,7 +345,7 @@ var a2 = crypto.createHash('sha256').update('Test123').digest('base64'); var a3 = crypto.createHash('sha512').update('Test123').digest(); // binary var a4 = crypto.createHash('sha1').update('Test123').digest('buffer'); -if (!common.hasFipsCrypto) { +if (!crypto.hasFipsCrypto()) { var a0 = crypto.createHash('md5').update('Test123').digest('binary'); assert.equal(a0, 'h\u00ea\u00cb\u0097\u00d8o\fF!\u00fa+\u000e\u0017\u00ca' + '\u00bd\u008c', 'Test MD5 as binary'); @@ -495,7 +495,7 @@ function testCipher4(key, iv) { assert.equal(txt, plaintext, 'encryption and decryption with key and iv'); } -if (!common.hasFipsCrypto) { +if (!crypto.hasFipsCrypto()) { testCipher1('MySecretKey123'); testCipher1(new Buffer('MySecretKey123')); @@ -519,7 +519,7 @@ assert.throws(function() { // Test Diffie-Hellman with two parties sharing a secret, // using various encodings as we go along -var dh1 = crypto.createDiffieHellman(common.hasFipsCrypto ? 1024 : 256); +var dh1 = crypto.createDiffieHellman(crypto.hasFipsCrypto() ? 1024 : 256); var p1 = dh1.getPrime('buffer'); var dh2 = crypto.createDiffieHellman(p1, 'base64'); var key1 = dh1.generateKeys(); diff --git a/test/parallel/test-crypto-cipher-decipher.js b/test/parallel/test-crypto-cipher-decipher.js index 5f867739abe7ed..8f4c153e3ab27c 100644 --- a/test/parallel/test-crypto-cipher-decipher.js +++ b/test/parallel/test-crypto-cipher-decipher.js @@ -6,11 +6,13 @@ if (!common.hasCrypto) { console.log('1..0 # Skipped: missing crypto'); return; } -if (common.hasFipsCrypto) { + +var crypto = require('crypto'); + +if (crypto.hasFipsCrypto()) { console.log('1..0 # Skipped: not supported in FIPS mode'); return; } -var crypto = require('crypto'); function testCipher1(key) { // Test encryption and decryption diff --git a/test/parallel/test-crypto-dh-odd-key.js b/test/parallel/test-crypto-dh-odd-key.js index 503ba2fe089e3e..8947a41f03d03b 100644 --- a/test/parallel/test-crypto-dh-odd-key.js +++ b/test/parallel/test-crypto-dh-odd-key.js @@ -18,7 +18,7 @@ function test() { } // FIPS requires a length of at least 1024 -if (!common.hasFipsCrypto) { +if (!crypto.hasFipsCrypto()) { assert.doesNotThrow(function() { test(); }); } else { assert.throws(function() { test(); }, /key size too small/); diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js index 68394dd9bcaff9..c976bc6ce7a0c5 100644 --- a/test/parallel/test-crypto-dh.js +++ b/test/parallel/test-crypto-dh.js @@ -11,7 +11,7 @@ const crypto = require('crypto'); // Test Diffie-Hellman with two parties sharing a secret, // using various encodings as we go along -var dh1 = crypto.createDiffieHellman(common.hasFipsCrypto ? 1024 : 256); +var dh1 = crypto.createDiffieHellman(crypto.hasFipsCrypto() ? 1024 : 256); var p1 = dh1.getPrime('buffer'); var dh2 = crypto.createDiffieHellman(p1, 'buffer'); var key1 = dh1.generateKeys(); diff --git a/test/parallel/test-crypto-ecb.js b/test/parallel/test-crypto-ecb.js index d47ec8a8091f38..d859c8dff982a8 100644 --- a/test/parallel/test-crypto-ecb.js +++ b/test/parallel/test-crypto-ecb.js @@ -6,7 +6,7 @@ if (!common.hasCrypto) { console.log('1..0 # Skipped: missing crypto'); return; } -if (common.hasFipsCrypto) { +if (crypto.hasFipsCrypto()) { console.log('1..0 # Skipped: BF-ECB is not FIPS 140-2 compatible'); return; } diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js new file mode 100644 index 00000000000000..9aefec756fc336 --- /dev/null +++ b/test/parallel/test-crypto-fips.js @@ -0,0 +1,194 @@ +'use strict'; +var common = require('../common'); +var assert = require('assert'); +var spawn = require('child_process').spawn; +var path = require('path'); + +if (!common.hasCrypto) { + console.log('1..0 # Skipped: missing crypto'); + return; +} + +const EXIT_FAILURE = -1; +const FIPS_ENABLED = 1; +const FIPS_DISABLED = 0; +const FIPS_ERROR_STRING = 'Error: Cannot set FIPS mode'; +const OPTION_ERROR_STRING = 'bad option'; +const CNF_FIPS_ON = common.fixturesDir + '/openssl_fips_enabled.cnf'; +const CNF_FIPS_OFF = common.fixturesDir + '/openssl_fips_disabled.cnf'; +var num_children_spawned = 0; +var num_children_ok = 0; + +function compiledWithFips() { + return process.config.variables.openssl_fips ? true : false; +} + +function addToEnv(newVar, value) { + var envCopy = {}; + for (const e in process.env) { + envCopy[e] = process.env[e]; + } + envCopy[newVar] = value; + return envCopy; +} + +function getResponse(data) +{ + return data.toString().replace('\n', '').replace('>', '').trim(); +} + +function childOk(child) { + console.log('Child ' + ++num_children_ok + '/' + num_children_spawned + + ' [pid:' + child.pid + '] OK.'); +} + +function testHelper(requiresFips, args, expectedOutput, cmd, env) { + const child = spawn(process.execPath, args.concat(['-i']), { + cwd: path.dirname(process.execPath), + env: env + }); + + console.log('Spawned child [pid:' + child.pid + '] with cmd ' + + cmd + ' and args \'' + args + '\''); + num_children_spawned++; + + // If using FIPS mode binary, or running a generic test. + if (compiledWithFips() || !requiresFips) { + child.stdin.setEncoding('utf-8'); + child.stdout.on('data', function(data) { + // Prompt and newline may occur in undefined order. + const response = getResponse(data); + if (response.length > 0) { + if (EXIT_FAILURE === expectedOutput) { + assert.notEqual(-1, response.indexOf(FIPS_ERROR_STRING)); + } else { + assert.equal(expectedOutput, response); + } + childOk(child); + } + }); + } else { + // If using a non-FIPS binary, expect a failure. + const error_handler = function(data, string) { + const response = getResponse(data); + if (response.length > 0) { + assert.notEqual(-1, response.indexOf(string)); + childOk(child); + } + }; + child.stdout.on('data', function(data) { + error_handler(data, FIPS_ERROR_STRING); + }); + /* Failure to start Node executable is reported on stderr */ + child.stderr.on('data', function(data) { + error_handler(data, OPTION_ERROR_STRING); + }); + } + // Wait for write to complete before exiting child, but won't be able to + // write to child stdin if cmd line args are rejected and process dies first, + // resulting in EPIPE, we avoid this by simply not writing anything. + if (null !== cmd) { + child.stdin.write(cmd + '\n;process.exit()\n'); + } +} + +// By default FIPS should be off in both FIPS and non-FIPS builds. +testHelper(false, + [], + FIPS_DISABLED, + 'require("crypto").hasFipsCrypto()', + process.env); + +// --enable-fips should force FIPS mode on +testHelper(true, + ['--enable-fips'], + compiledWithFips() ? FIPS_ENABLED : EXIT_FAILURE, + compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + process.env); + +//--disable-fips should force FIPS mode on +testHelper(true, + ['--disable-fips'], + compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, + 'require("crypto").hasFipsCrypto()', + process.env); + +// --disable-fips should take precedence over --enable-fips +testHelper(true, + ['--disable-fips', '--enable-fips'], + compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, + compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + process.env); + +// --disable-fips and --enable-fips order do not matter +testHelper(true, + ['--enable-fips', '--disable-fips'], + compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, + compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + process.env); + +// OpenSSL config file should be able to turn on FIPS mode +testHelper(false, + [], + compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, + 'require("crypto").hasFipsCrypto()', + addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); + +// --disable-fips should take precedence over OpenSSL config file +testHelper(true, + ['--disable-fips'], + compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, + compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); + +// --enable-fips should take precedence over OpenSSL config file +testHelper(true, + ['--enable-fips'], + compiledWithFips() ? FIPS_ENABLED : EXIT_FAILURE, + compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); + +// setFipsCrypto should be able to turn FIPS mode on +testHelper(true, [], FIPS_ENABLED, + '(require("crypto").setFipsCrypto(1),' + + 'require("crypto").hasFipsCrypto())', + process.env); + +// setFipsCrypto should be able to turn FIPS mode on and off +testHelper(true, + [], + FIPS_DISABLED, + '(require("crypto").setFipsCrypto(1),' + + 'require("crypto").setFipsCrypto(0),' + + 'require("crypto").hasFipsCrypto())', + process.env); + +// setFipsCrypto takes precedence over OpenSSL config file, FIPS on +testHelper(true, + [], + compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, + '(require("crypto").setFipsCrypto(1),' + + 'require("crypto").hasFipsCrypto())', + addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); + +// setFipsCrypto takes precedence over OpenSSL config file, FIPS off +testHelper(true, + [], + FIPS_DISABLED, + '(require("crypto").setFipsCrypto(0),' + + 'require("crypto").hasFipsCrypto())', + addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); + +// --disable-fips prevents use of setFipsCrypto API +testHelper(true, + ['--disable-fips'], + EXIT_FAILURE, + compiledWithFips() ? 'require("crypto").setFipsCrypto(1)' : null, + process.env); + +// --enable-fips prevents use of setFipsCrypto API +testHelper(true, + ['--enable-fips'], + EXIT_FAILURE, + compiledWithFips() ? 'require("crypto").setFipsCrypto(0)' : null, + process.env); diff --git a/test/parallel/test-crypto-hash.js b/test/parallel/test-crypto-hash.js index 90ff1fd727d104..af0790a4521ccb 100644 --- a/test/parallel/test-crypto-hash.js +++ b/test/parallel/test-crypto-hash.js @@ -37,7 +37,7 @@ a8.write(''); a8.end(); a8 = a8.read(); -if (!common.hasFipsCrypto) { +if (!crypto.hasFipsCrypto()) { var a0 = crypto.createHash('md5').update('Test123').digest('binary'); assert.equal(a0, 'h\u00ea\u00cb\u0097\u00d8o\fF!\u00fa+\u000e\u0017\u00ca' + '\u00bd\u008c', 'Test MD5 as binary'); diff --git a/test/parallel/test-crypto-hmac.js b/test/parallel/test-crypto-hmac.js index 600dd0dbb2c8bd..241a7f7b9a4117 100644 --- a/test/parallel/test-crypto-hmac.js +++ b/test/parallel/test-crypto-hmac.js @@ -62,7 +62,7 @@ var wikipedia = [ for (let i = 0, l = wikipedia.length; i < l; i++) { for (const hash in wikipedia[i]['hmac']) { // FIPS does not support MD5. - if (common.hasFipsCrypto && hash == 'md5') + if (crypto.hasFipsCrypto() && hash == 'md5') continue; const result = crypto.createHmac(hash, wikipedia[i]['key']) .update(wikipedia[i]['data']) @@ -349,7 +349,7 @@ var rfc2202_sha1 = [ } ]; -if (!common.hasFipsCrypto) { +if (!crypto.hasFipsCrypto()) { for (let i = 0, l = rfc2202_md5.length; i < l; i++) { assert.equal(rfc2202_md5[i]['hmac'], crypto.createHmac('md5', rfc2202_md5[i]['key']) diff --git a/test/parallel/test-crypto-stream.js b/test/parallel/test-crypto-stream.js index bf0fc2ca1d00df..2cf0845ef5e27c 100644 --- a/test/parallel/test-crypto-stream.js +++ b/test/parallel/test-crypto-stream.js @@ -26,7 +26,7 @@ Stream2buffer.prototype._write = function(data, encodeing, done) { return done(null); }; -if (!common.hasFipsCrypto) { +if (!crypto.hasFipsCrypto()) { // Create an md5 hash of "Hallo world" var hasher1 = crypto.createHash('md5'); hasher1.pipe(new Stream2buffer(common.mustCall(function end(err, hash) { diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 192e4287c47d03..43427df9cfa693 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -92,11 +92,11 @@ assertSorted(crypto.getCurves()); // throw, not assert in C++ land. assert.throws(function() { crypto.createCipher('aes192', 'test').update('0', 'hex'); -}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/); +}, crypto.hasFipsCrypto() ? /not supported in FIPS mode/ : /Bad input string/); assert.throws(function() { crypto.createDecipher('aes192', 'test').update('0', 'hex'); -}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/); +}, crypto.hasFipsCrypto() ? /not supported in FIPS mode/ : /Bad input string/); assert.throws(function() { crypto.createHash('sha1').update('0', 'hex'); diff --git a/test/parallel/test-dsa-fips-invalid-key.js b/test/parallel/test-dsa-fips-invalid-key.js index 2d30ef6891008a..29bd39c7c7863a 100644 --- a/test/parallel/test-dsa-fips-invalid-key.js +++ b/test/parallel/test-dsa-fips-invalid-key.js @@ -2,14 +2,19 @@ var common = require('../common'); var assert = require('assert'); -if (!common.hasFipsCrypto) { - console.log('1..0 # Skipped: node compiled without FIPS OpenSSL.'); +if (!common.hasCrypto) { + console.log('1..0 # Skipped: missing crypto'); return; } var crypto = require('crypto'); var fs = require('fs'); +if (!crypto.hasFipsCrypto()) { + console.log('1..0 # Skipped: node started without FIPS OpenSSL.'); + return; +} + var input = 'hello'; var dsapri = fs.readFileSync(common.fixturesDir + diff --git a/test/parallel/test-tls-ocsp-callback.js b/test/parallel/test-tls-ocsp-callback.js index e9443f45357995..6411adddbc8983 100644 --- a/test/parallel/test-tls-ocsp-callback.js +++ b/test/parallel/test-tls-ocsp-callback.js @@ -115,7 +115,7 @@ var tests = [ { ocsp: false } ]; -if (!common.hasFipsCrypto) { +if (!crypto.hasFipsCrypto()) { tests.push({ pfx: pfx, passphrase: 'sample', response: 'hello pfx' }); } diff --git a/test/pummel/test-crypto-dh.js b/test/pummel/test-crypto-dh.js index 596d107a287a28..d29713bb66a0f3 100644 --- a/test/pummel/test-crypto-dh.js +++ b/test/pummel/test-crypto-dh.js @@ -2,9 +2,7 @@ var common = require('../common'); var assert = require('assert'); -try { - var crypto = require('crypto'); -} catch (e) { +if (!common.hasCrypto) { console.log('1..0 # Skipped: node compiled without OpenSSL.'); return; } @@ -42,7 +40,7 @@ for (const name in hashes) { for (const name in hashes) { // modp1 is 768 bits, FIPS requires >= 1024 - if (name == 'modp1' && common.hasFipsCrypto) + if (name == 'modp1' && crypto.hasFipsCrypto()) continue; var group1 = crypto.getDiffieHellman(name); var group2 = crypto.getDiffieHellman(name); From a3a469b73001349008ec484c1db92c3ef06af04f Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 10 Feb 2016 15:01:43 -0500 Subject: [PATCH 02/14] WIP: Changes from first review * Fix indent * Listen for end event * Undo changes to tests, provide common.js wrapper --- src/node.cc | 4 +- src/node_crypto.cc | 4 +- test/common.js | 6 ++ test/parallel/test-crypto-authenticated.js | 4 +- test/parallel/test-crypto-binary-default.js | 6 +- test/parallel/test-crypto-cipher-decipher.js | 6 +- test/parallel/test-crypto-dh-odd-key.js | 2 +- test/parallel/test-crypto-dh.js | 2 +- test/parallel/test-crypto-ecb.js | 2 +- test/parallel/test-crypto-fips.js | 89 +++++++++++--------- test/parallel/test-crypto-hash.js | 2 +- test/parallel/test-crypto-hmac.js | 4 +- test/parallel/test-crypto-stream.js | 2 +- test/parallel/test-crypto.js | 4 +- test/parallel/test-dsa-fips-invalid-key.js | 9 +- test/parallel/test-tls-ocsp-callback.js | 2 +- test/pummel/test-crypto-dh.js | 2 +- 17 files changed, 81 insertions(+), 69 deletions(-) diff --git a/src/node.cc b/src/node.cc index e61bc21b9bd4c8..a55fbbb9e8c446 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3436,9 +3436,9 @@ static void ParseArgs(int* argc, default_cipher_list = arg + 18; #if NODE_FIPS_MODE } else if (strcmp(arg, "--disable-fips") == 0) { - disable_fips_crypto = true; + disable_fips_crypto = true; } else if (strcmp(arg, "--enable-fips") == 0) { - enable_fips_crypto = true; + enable_fips_crypto = true; #endif /* NODE_FIPS_MODE */ #endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index b331b698f3eaf2..41d5ade8cde380 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5762,10 +5762,10 @@ void SetFipsCrypto(const FunctionCallbackInfo& args) { bool mode = args[0]->BooleanValue(); if (disable_fips_crypto) { return env->ThrowError( - "Cannot set FIPS mode, it was forced with --disable-fips at startup."); + "Cannot set FIPS mode, it was forced with --disable-fips at startup."); } else if (enable_fips_crypto) { return env->ThrowError( - "Cannot set FIPS mode, it was forced with --enable-fips at startup."); + "Cannot set FIPS mode, it was forced with --enable-fips at startup."); } else if (!FIPS_mode_set(mode)) { unsigned long err = ERR_get_error(); return ThrowCryptoError(env, err); diff --git a/test/common.js b/test/common.js index 764ab13835b0c1..36d18c01da7fc9 100644 --- a/test/common.js +++ b/test/common.js @@ -159,6 +159,12 @@ Object.defineProperty(exports, 'hasCrypto', { } }); +Object.defineProperty(exports, 'hasFipsCrypto', { + get: function() { + return hasCrypto && require('crypto').hasFipsCrypto(); + } +}); + if (exports.isWindows) { exports.PIPE = '\\\\.\\pipe\\libuv-test'; if (process.env.TEST_THREAD_ID) { diff --git a/test/parallel/test-crypto-authenticated.js b/test/parallel/test-crypto-authenticated.js index 2146c014ccc69e..fa9a78c26e5b59 100644 --- a/test/parallel/test-crypto-authenticated.js +++ b/test/parallel/test-crypto-authenticated.js @@ -93,7 +93,7 @@ for (var i in TEST_CASES) { (function() { if (!test.password) return; - if (crypto.hasFipsCrypto()) { + if (common.hasFipsCrypto) { assert.throws(function() { crypto.createCipher(test.algo, test.password); }, /not supported in FIPS mode/); @@ -114,7 +114,7 @@ for (var i in TEST_CASES) { (function() { if (!test.password) return; - if (crypto.hasFipsCrypto()) { + if (common.hasFipsCrypto) { assert.throws(function() { crypto.createDecipher(test.algo, test.password); }, /not supported in FIPS mode/); diff --git a/test/parallel/test-crypto-binary-default.js b/test/parallel/test-crypto-binary-default.js index 299559f050ecc2..3cb98db80aca55 100644 --- a/test/parallel/test-crypto-binary-default.js +++ b/test/parallel/test-crypto-binary-default.js @@ -345,7 +345,7 @@ var a2 = crypto.createHash('sha256').update('Test123').digest('base64'); var a3 = crypto.createHash('sha512').update('Test123').digest(); // binary var a4 = crypto.createHash('sha1').update('Test123').digest('buffer'); -if (!crypto.hasFipsCrypto()) { +if (!common.hasFipsCrypto) { var a0 = crypto.createHash('md5').update('Test123').digest('binary'); assert.equal(a0, 'h\u00ea\u00cb\u0097\u00d8o\fF!\u00fa+\u000e\u0017\u00ca' + '\u00bd\u008c', 'Test MD5 as binary'); @@ -495,7 +495,7 @@ function testCipher4(key, iv) { assert.equal(txt, plaintext, 'encryption and decryption with key and iv'); } -if (!crypto.hasFipsCrypto()) { +if (!common.hasFipsCrypto) { testCipher1('MySecretKey123'); testCipher1(new Buffer('MySecretKey123')); @@ -519,7 +519,7 @@ assert.throws(function() { // Test Diffie-Hellman with two parties sharing a secret, // using various encodings as we go along -var dh1 = crypto.createDiffieHellman(crypto.hasFipsCrypto() ? 1024 : 256); +var dh1 = crypto.createDiffieHellman(common.hasFipsCrypto ? 1024 : 256); var p1 = dh1.getPrime('buffer'); var dh2 = crypto.createDiffieHellman(p1, 'base64'); var key1 = dh1.generateKeys(); diff --git a/test/parallel/test-crypto-cipher-decipher.js b/test/parallel/test-crypto-cipher-decipher.js index 8f4c153e3ab27c..5f867739abe7ed 100644 --- a/test/parallel/test-crypto-cipher-decipher.js +++ b/test/parallel/test-crypto-cipher-decipher.js @@ -6,13 +6,11 @@ if (!common.hasCrypto) { console.log('1..0 # Skipped: missing crypto'); return; } - -var crypto = require('crypto'); - -if (crypto.hasFipsCrypto()) { +if (common.hasFipsCrypto) { console.log('1..0 # Skipped: not supported in FIPS mode'); return; } +var crypto = require('crypto'); function testCipher1(key) { // Test encryption and decryption diff --git a/test/parallel/test-crypto-dh-odd-key.js b/test/parallel/test-crypto-dh-odd-key.js index 8947a41f03d03b..503ba2fe089e3e 100644 --- a/test/parallel/test-crypto-dh-odd-key.js +++ b/test/parallel/test-crypto-dh-odd-key.js @@ -18,7 +18,7 @@ function test() { } // FIPS requires a length of at least 1024 -if (!crypto.hasFipsCrypto()) { +if (!common.hasFipsCrypto) { assert.doesNotThrow(function() { test(); }); } else { assert.throws(function() { test(); }, /key size too small/); diff --git a/test/parallel/test-crypto-dh.js b/test/parallel/test-crypto-dh.js index c976bc6ce7a0c5..68394dd9bcaff9 100644 --- a/test/parallel/test-crypto-dh.js +++ b/test/parallel/test-crypto-dh.js @@ -11,7 +11,7 @@ const crypto = require('crypto'); // Test Diffie-Hellman with two parties sharing a secret, // using various encodings as we go along -var dh1 = crypto.createDiffieHellman(crypto.hasFipsCrypto() ? 1024 : 256); +var dh1 = crypto.createDiffieHellman(common.hasFipsCrypto ? 1024 : 256); var p1 = dh1.getPrime('buffer'); var dh2 = crypto.createDiffieHellman(p1, 'buffer'); var key1 = dh1.generateKeys(); diff --git a/test/parallel/test-crypto-ecb.js b/test/parallel/test-crypto-ecb.js index d859c8dff982a8..d47ec8a8091f38 100644 --- a/test/parallel/test-crypto-ecb.js +++ b/test/parallel/test-crypto-ecb.js @@ -6,7 +6,7 @@ if (!common.hasCrypto) { console.log('1..0 # Skipped: missing crypto'); return; } -if (crypto.hasFipsCrypto()) { +if (common.hasFipsCrypto) { console.log('1..0 # Skipped: BF-ECB is not FIPS 140-2 compatible'); return; } diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index 9aefec756fc336..3da6a296cd4daf 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -14,10 +14,11 @@ const FIPS_ENABLED = 1; const FIPS_DISABLED = 0; const FIPS_ERROR_STRING = 'Error: Cannot set FIPS mode'; const OPTION_ERROR_STRING = 'bad option'; -const CNF_FIPS_ON = common.fixturesDir + '/openssl_fips_enabled.cnf'; -const CNF_FIPS_OFF = common.fixturesDir + '/openssl_fips_disabled.cnf'; +const CNF_FIPS_ON = path.join(common.fixturesDir, 'openssl_fips_enabled.cnf'); +const CNF_FIPS_OFF = path.join(common.fixturesDir, 'openssl_fips_disabled.cnf'); var num_children_spawned = 0; var num_children_ok = 0; +const child_responses = {}; function compiledWithFips() { return process.config.variables.openssl_fips ? true : false; @@ -32,13 +33,8 @@ function addToEnv(newVar, value) { return envCopy; } -function getResponse(data) -{ - return data.toString().replace('\n', '').replace('>', '').trim(); -} - function childOk(child) { - console.log('Child ' + ++num_children_ok + '/' + num_children_spawned + + console.error('Child ' + ++num_children_ok + '/' + num_children_spawned + ' [pid:' + child.pid + '] OK.'); } @@ -48,46 +44,62 @@ function testHelper(requiresFips, args, expectedOutput, cmd, env) { env: env }); - console.log('Spawned child [pid:' + child.pid + '] with cmd ' + - cmd + ' and args \'' + args + '\''); + console.error('Spawned child [pid:' + child.pid + '] with cmd ' + + cmd + ' and args \'' + args + '\''); num_children_spawned++; + const response_handler = function(response, expectedOutput, expectedError) { + // We won't always have data on both stdout and stderr. + if (undefined !== response) { + if (EXIT_FAILURE === expectedOutput) { + assert.notEqual(-1, response.indexOf(expectedError)); + } else { + assert.equal(expectedOutput, response); + } + childOk(child); + } + }; + + // Buffer all data received from children on stderr/stdout. + const response_buffer = function(child, data) { + // Prompt and newline may occur in undefined order. + const response = data.toString().replace(/\n|>/g, '').trim(); + if (response.length > 0) { + child_responses[child] = response; + } + }; + + child.stdout.on('data', function(data) { + response_buffer(child.pid + '-stdout', data); + }); + + child.stderr.on('data', function(data) { + response_buffer(child.pid + '-stderr', data); + }); + // If using FIPS mode binary, or running a generic test. if (compiledWithFips() || !requiresFips) { - child.stdin.setEncoding('utf-8'); - child.stdout.on('data', function(data) { - // Prompt and newline may occur in undefined order. - const response = getResponse(data); - if (response.length > 0) { - if (EXIT_FAILURE === expectedOutput) { - assert.notEqual(-1, response.indexOf(FIPS_ERROR_STRING)); - } else { - assert.equal(expectedOutput, response); - } - childOk(child); - } + child.stdout.on('end', function(data) { + response_handler(child_responses[child.pid + '-stdout'], + expectedOutput, FIPS_ERROR_STRING); }); } else { - // If using a non-FIPS binary, expect a failure. - const error_handler = function(data, string) { - const response = getResponse(data); - if (response.length > 0) { - assert.notEqual(-1, response.indexOf(string)); - childOk(child); - } - }; - child.stdout.on('data', function(data) { - error_handler(data, FIPS_ERROR_STRING); + // If using a non-FIPS binary expect a failure. + child.stdout.on('end', function() { + response_handler(child_responses[child.pid + '-stdout'], + EXIT_FAILURE, FIPS_ERROR_STRING); }); /* Failure to start Node executable is reported on stderr */ - child.stderr.on('data', function(data) { - error_handler(data, OPTION_ERROR_STRING); + child.stderr.on('end', function() { + response_handler(child_responses[child.pid + '-stderr'], + EXIT_FAILURE, OPTION_ERROR_STRING); }); } - // Wait for write to complete before exiting child, but won't be able to - // write to child stdin if cmd line args are rejected and process dies first, - // resulting in EPIPE, we avoid this by simply not writing anything. + // Wait for write to complete before exiting child, but we won't be able to + // write to the child's stdin if cmd line args are rejected and process dies + // first, resulting in EPIPE. We can avoid EPIPE by not writing anything. if (null !== cmd) { + child.stdin.setEncoding('utf-8'); child.stdin.write(cmd + '\n;process.exit()\n'); } } @@ -110,7 +122,7 @@ testHelper(true, testHelper(true, ['--disable-fips'], compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - 'require("crypto").hasFipsCrypto()', + compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, process.env); // --disable-fips should take precedence over --enable-fips @@ -192,3 +204,4 @@ testHelper(true, EXIT_FAILURE, compiledWithFips() ? 'require("crypto").setFipsCrypto(0)' : null, process.env); + diff --git a/test/parallel/test-crypto-hash.js b/test/parallel/test-crypto-hash.js index af0790a4521ccb..90ff1fd727d104 100644 --- a/test/parallel/test-crypto-hash.js +++ b/test/parallel/test-crypto-hash.js @@ -37,7 +37,7 @@ a8.write(''); a8.end(); a8 = a8.read(); -if (!crypto.hasFipsCrypto()) { +if (!common.hasFipsCrypto) { var a0 = crypto.createHash('md5').update('Test123').digest('binary'); assert.equal(a0, 'h\u00ea\u00cb\u0097\u00d8o\fF!\u00fa+\u000e\u0017\u00ca' + '\u00bd\u008c', 'Test MD5 as binary'); diff --git a/test/parallel/test-crypto-hmac.js b/test/parallel/test-crypto-hmac.js index 241a7f7b9a4117..600dd0dbb2c8bd 100644 --- a/test/parallel/test-crypto-hmac.js +++ b/test/parallel/test-crypto-hmac.js @@ -62,7 +62,7 @@ var wikipedia = [ for (let i = 0, l = wikipedia.length; i < l; i++) { for (const hash in wikipedia[i]['hmac']) { // FIPS does not support MD5. - if (crypto.hasFipsCrypto() && hash == 'md5') + if (common.hasFipsCrypto && hash == 'md5') continue; const result = crypto.createHmac(hash, wikipedia[i]['key']) .update(wikipedia[i]['data']) @@ -349,7 +349,7 @@ var rfc2202_sha1 = [ } ]; -if (!crypto.hasFipsCrypto()) { +if (!common.hasFipsCrypto) { for (let i = 0, l = rfc2202_md5.length; i < l; i++) { assert.equal(rfc2202_md5[i]['hmac'], crypto.createHmac('md5', rfc2202_md5[i]['key']) diff --git a/test/parallel/test-crypto-stream.js b/test/parallel/test-crypto-stream.js index 2cf0845ef5e27c..bf0fc2ca1d00df 100644 --- a/test/parallel/test-crypto-stream.js +++ b/test/parallel/test-crypto-stream.js @@ -26,7 +26,7 @@ Stream2buffer.prototype._write = function(data, encodeing, done) { return done(null); }; -if (!crypto.hasFipsCrypto()) { +if (!common.hasFipsCrypto) { // Create an md5 hash of "Hallo world" var hasher1 = crypto.createHash('md5'); hasher1.pipe(new Stream2buffer(common.mustCall(function end(err, hash) { diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index 43427df9cfa693..192e4287c47d03 100644 --- a/test/parallel/test-crypto.js +++ b/test/parallel/test-crypto.js @@ -92,11 +92,11 @@ assertSorted(crypto.getCurves()); // throw, not assert in C++ land. assert.throws(function() { crypto.createCipher('aes192', 'test').update('0', 'hex'); -}, crypto.hasFipsCrypto() ? /not supported in FIPS mode/ : /Bad input string/); +}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/); assert.throws(function() { crypto.createDecipher('aes192', 'test').update('0', 'hex'); -}, crypto.hasFipsCrypto() ? /not supported in FIPS mode/ : /Bad input string/); +}, common.hasFipsCrypto ? /not supported in FIPS mode/ : /Bad input string/); assert.throws(function() { crypto.createHash('sha1').update('0', 'hex'); diff --git a/test/parallel/test-dsa-fips-invalid-key.js b/test/parallel/test-dsa-fips-invalid-key.js index 29bd39c7c7863a..2d30ef6891008a 100644 --- a/test/parallel/test-dsa-fips-invalid-key.js +++ b/test/parallel/test-dsa-fips-invalid-key.js @@ -2,19 +2,14 @@ var common = require('../common'); var assert = require('assert'); -if (!common.hasCrypto) { - console.log('1..0 # Skipped: missing crypto'); +if (!common.hasFipsCrypto) { + console.log('1..0 # Skipped: node compiled without FIPS OpenSSL.'); return; } var crypto = require('crypto'); var fs = require('fs'); -if (!crypto.hasFipsCrypto()) { - console.log('1..0 # Skipped: node started without FIPS OpenSSL.'); - return; -} - var input = 'hello'; var dsapri = fs.readFileSync(common.fixturesDir + diff --git a/test/parallel/test-tls-ocsp-callback.js b/test/parallel/test-tls-ocsp-callback.js index 6411adddbc8983..e9443f45357995 100644 --- a/test/parallel/test-tls-ocsp-callback.js +++ b/test/parallel/test-tls-ocsp-callback.js @@ -115,7 +115,7 @@ var tests = [ { ocsp: false } ]; -if (!crypto.hasFipsCrypto()) { +if (!common.hasFipsCrypto) { tests.push({ pfx: pfx, passphrase: 'sample', response: 'hello pfx' }); } diff --git a/test/pummel/test-crypto-dh.js b/test/pummel/test-crypto-dh.js index d29713bb66a0f3..8d732ed92efd10 100644 --- a/test/pummel/test-crypto-dh.js +++ b/test/pummel/test-crypto-dh.js @@ -40,7 +40,7 @@ for (const name in hashes) { for (const name in hashes) { // modp1 is 768 bits, FIPS requires >= 1024 - if (name == 'modp1' && crypto.hasFipsCrypto()) + if (name == 'modp1' && common.hasFipsCrypto) continue; var group1 = crypto.getDiffieHellman(name); var group2 = crypto.getDiffieHellman(name); From fc07d92f6f389cfd5cf1b3c586f97107e748d07d Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 10 Feb 2016 17:30:46 -0500 Subject: [PATCH 03/14] WIP: Fixes for FIPS runtime detection --- src/node_crypto.cc | 6 ++++-- test/common.js | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 41d5ade8cde380..e3393c9f1d6656 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3126,8 +3126,10 @@ void CipherBase::Init(const char* cipher_type, HandleScope scope(env()->isolate()); #ifdef NODE_FIPS_MODE - return env()->ThrowError( - "crypto.createCipher() is not supported in FIPS mode."); + if (FIPS_mode()) { + return env()->ThrowError( + "crypto.createCipher() is not supported in FIPS mode."); + } #endif // NODE_FIPS_MODE CHECK_EQ(cipher_, nullptr); diff --git a/test/common.js b/test/common.js index 36d18c01da7fc9..6441638b83fde0 100644 --- a/test/common.js +++ b/test/common.js @@ -161,7 +161,7 @@ Object.defineProperty(exports, 'hasCrypto', { Object.defineProperty(exports, 'hasFipsCrypto', { get: function() { - return hasCrypto && require('crypto').hasFipsCrypto(); + return exports.hasCrypto && require('crypto').hasFipsCrypto(); } }); From dcd6a08db7a358e8661c061dd7cf270deff4b5b5 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 10 Feb 2016 18:51:10 -0500 Subject: [PATCH 04/14] WIP: Use a property instead of functions --- doc/api/crypto.markdown | 9 +++---- lib/crypto.js | 11 ++++----- test/common.js | 2 +- test/parallel/test-crypto-fips.js | 39 ++++++++++++++++--------------- 4 files changed, 28 insertions(+), 33 deletions(-) diff --git a/doc/api/crypto.markdown b/doc/api/crypto.markdown index 8cdd8f0713e5df..e0cbf9fea69da2 100644 --- a/doc/api/crypto.markdown +++ b/doc/api/crypto.markdown @@ -815,13 +815,10 @@ with legacy programs that expect `'binary'` to be the default encoding. New applications should expect the default to be `'buffer'`. This property may become deprecated in a future Node.js release. -### crypto.hasFipsCrypto() +### crypto.fips -Identifies whether a FIPS compliant crypto provider is currently in use. - -### crypto.setFipsCrypto(mode) - -Sets FIPS compliant cryptographic provider on or off. +Property for checking and controlling whether a FIPS compliant crypto provider is +currently in use. Setting to true requires a FIPS build of Node.js. ### crypto.createCipher(algorithm, password) diff --git a/lib/crypto.js b/lib/crypto.js index 3e414a693105f6..664d88397a36aa 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -647,13 +647,10 @@ exports.getCurves = function() { return filterDuplicates(getCurves()); }; -exports.hasFipsCrypto = function() { - return hasFipsCrypto(); -}; - -exports.setFipsCrypto = function(mode) { - return setFipsCrypto(mode); -}; +Object.defineProperty(exports, 'fips', { + get: hasFipsCrypto, + set: setFipsCrypto +}); function filterDuplicates(names) { // Drop all-caps names in favor of their lowercase aliases, diff --git a/test/common.js b/test/common.js index 6441638b83fde0..9562b2859ad28a 100644 --- a/test/common.js +++ b/test/common.js @@ -161,7 +161,7 @@ Object.defineProperty(exports, 'hasCrypto', { Object.defineProperty(exports, 'hasFipsCrypto', { get: function() { - return exports.hasCrypto && require('crypto').hasFipsCrypto(); + return exports.hasCrypto && require('crypto').hasFipsCrypto; } }); diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index 3da6a296cd4daf..fbe4abf6bd97ef 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -52,6 +52,7 @@ function testHelper(requiresFips, args, expectedOutput, cmd, env) { // We won't always have data on both stdout and stderr. if (undefined !== response) { if (EXIT_FAILURE === expectedOutput) { + if(-1 === response.indexOf(expectedError)) assert.notEqual(-1, response.indexOf(expectedError)); } else { assert.equal(expectedOutput, response); @@ -108,100 +109,100 @@ function testHelper(requiresFips, args, expectedOutput, cmd, env) { testHelper(false, [], FIPS_DISABLED, - 'require("crypto").hasFipsCrypto()', + 'require("crypto").fips', process.env); // --enable-fips should force FIPS mode on testHelper(true, ['--enable-fips'], compiledWithFips() ? FIPS_ENABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + compiledWithFips() ? 'require("crypto").fips' : null, process.env); //--disable-fips should force FIPS mode on testHelper(true, ['--disable-fips'], compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + compiledWithFips() ? 'require("crypto").fips' : null, process.env); // --disable-fips should take precedence over --enable-fips testHelper(true, ['--disable-fips', '--enable-fips'], compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + compiledWithFips() ? 'require("crypto").fips' : null, process.env); // --disable-fips and --enable-fips order do not matter testHelper(true, ['--enable-fips', '--disable-fips'], compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + compiledWithFips() ? 'require("crypto").fips' : null, process.env); // OpenSSL config file should be able to turn on FIPS mode testHelper(false, [], compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, - 'require("crypto").hasFipsCrypto()', + 'require("crypto").fips', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); // --disable-fips should take precedence over OpenSSL config file testHelper(true, ['--disable-fips'], compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + compiledWithFips() ? 'require("crypto").fips' : null, addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); // --enable-fips should take precedence over OpenSSL config file testHelper(true, ['--enable-fips'], compiledWithFips() ? FIPS_ENABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").hasFipsCrypto()' : null, + compiledWithFips() ? 'require("crypto").fips' : null, addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); // setFipsCrypto should be able to turn FIPS mode on testHelper(true, [], FIPS_ENABLED, - '(require("crypto").setFipsCrypto(1),' + - 'require("crypto").hasFipsCrypto())', + '(require("crypto").fips = true,' + + 'require("crypto").fips)', process.env); // setFipsCrypto should be able to turn FIPS mode on and off testHelper(true, [], FIPS_DISABLED, - '(require("crypto").setFipsCrypto(1),' + - 'require("crypto").setFipsCrypto(0),' + - 'require("crypto").hasFipsCrypto())', + '(require("crypto").fips = true,' + + 'require("crypto").fips = false,' + + 'require("crypto").fips)', process.env); // setFipsCrypto takes precedence over OpenSSL config file, FIPS on testHelper(true, [], compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, - '(require("crypto").setFipsCrypto(1),' + - 'require("crypto").hasFipsCrypto())', + '(require("crypto").fips = true,' + + 'require("crypto").fips)', addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); // setFipsCrypto takes precedence over OpenSSL config file, FIPS off testHelper(true, [], FIPS_DISABLED, - '(require("crypto").setFipsCrypto(0),' + - 'require("crypto").hasFipsCrypto())', + '(require("crypto").fips = false,' + + 'require("crypto").fips)', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); // --disable-fips prevents use of setFipsCrypto API testHelper(true, ['--disable-fips'], EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").setFipsCrypto(1)' : null, + compiledWithFips() ? 'require("crypto").fips = true' : null, process.env); // --enable-fips prevents use of setFipsCrypto API testHelper(true, ['--enable-fips'], EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").setFipsCrypto(0)' : null, + compiledWithFips() ? 'require("crypto").fips = false' : null, process.env); From 85d0aeb949a3f3c6299e2a3238bfe426290453f4 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 10 Feb 2016 19:41:56 -0500 Subject: [PATCH 05/14] WIP: Fixes for 2nd review --- src/node_crypto.cc | 38 +++++++++++++++---------------- test/parallel/test-crypto-fips.js | 26 ++++++++++----------- 2 files changed, 31 insertions(+), 33 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index e3393c9f1d6656..c59befe2a6313e 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3859,26 +3859,24 @@ SignBase::Error Sign::SignFinal(const char* key_pem, goto exit; #ifdef NODE_FIPS_MODE - if (FIPS_mode()) { - /* Validate DSA2 parameters from FIPS 186-4 */ - if (EVP_PKEY_DSA == pkey->type) { - size_t L = BN_num_bits(pkey->pkey.dsa->p); - size_t N = BN_num_bits(pkey->pkey.dsa->q); - bool result = false; - - if (L == 1024 && N == 160) - result = true; - else if (L == 2048 && N == 224) - result = true; - else if (L == 2048 && N == 256) - result = true; - else if (L == 3072 && N == 256) - result = true; - - if (!result) { - fatal = true; - goto exit; - } + /* Validate DSA2 parameters from FIPS 186-4 */ + if (FIPS_mode() && (EVP_PKEY_DSA == pkey->type)) { + size_t L = BN_num_bits(pkey->pkey.dsa->p); + size_t N = BN_num_bits(pkey->pkey.dsa->q); + bool result = false; + + if (L == 1024 && N == 160) + result = true; + else if (L == 2048 && N == 224) + result = true; + else if (L == 2048 && N == 256) + result = true; + else if (L == 3072 && N == 256) + result = true; + + if (!result) { + fatal = true; + goto exit; } } #endif // NODE_FIPS_MODE diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index fbe4abf6bd97ef..89ed30806cae12 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -18,7 +18,6 @@ const CNF_FIPS_ON = path.join(common.fixturesDir, 'openssl_fips_enabled.cnf'); const CNF_FIPS_OFF = path.join(common.fixturesDir, 'openssl_fips_disabled.cnf'); var num_children_spawned = 0; var num_children_ok = 0; -const child_responses = {}; function compiledWithFips() { return process.config.variables.openssl_fips ? true : false; @@ -48,51 +47,52 @@ function testHelper(requiresFips, args, expectedOutput, cmd, env) { cmd + ' and args \'' + args + '\''); num_children_spawned++; - const response_handler = function(response, expectedOutput, expectedError) { + function response_handler(response, expectedOutput, expectedError) { // We won't always have data on both stdout and stderr. - if (undefined !== response) { + if (response.length > 0) { if (EXIT_FAILURE === expectedOutput) { - if(-1 === response.indexOf(expectedError)) - assert.notEqual(-1, response.indexOf(expectedError)); + if (-1 === response.indexOf(expectedError)) + assert.notEqual(-1, response.indexOf(expectedError)); } else { assert.equal(expectedOutput, response); } childOk(child); } - }; + } // Buffer all data received from children on stderr/stdout. - const response_buffer = function(child, data) { + const buffer = { stderr: '', stdout: '' }; + const response_buffer = function(stream, data) { // Prompt and newline may occur in undefined order. const response = data.toString().replace(/\n|>/g, '').trim(); if (response.length > 0) { - child_responses[child] = response; + buffer[stream] += response; } }; child.stdout.on('data', function(data) { - response_buffer(child.pid + '-stdout', data); + response_buffer('stdout', data); }); child.stderr.on('data', function(data) { - response_buffer(child.pid + '-stderr', data); + response_buffer('stderr', data); }); // If using FIPS mode binary, or running a generic test. if (compiledWithFips() || !requiresFips) { child.stdout.on('end', function(data) { - response_handler(child_responses[child.pid + '-stdout'], + response_handler(buffer['stdout'], expectedOutput, FIPS_ERROR_STRING); }); } else { // If using a non-FIPS binary expect a failure. child.stdout.on('end', function() { - response_handler(child_responses[child.pid + '-stdout'], + response_handler(buffer['stdout'], EXIT_FAILURE, FIPS_ERROR_STRING); }); /* Failure to start Node executable is reported on stderr */ child.stderr.on('end', function() { - response_handler(child_responses[child.pid + '-stderr'], + response_handler(buffer['stderr'], EXIT_FAILURE, OPTION_ERROR_STRING); }); } From 09be4aa0873d481282edfc8da4b45eb0a11e9f24 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 17 Feb 2016 17:55:42 -0500 Subject: [PATCH 06/14] WIP: Style changes following review --- test/parallel/test-crypto-fips.js | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index 89ed30806cae12..3919b12b823170 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -47,7 +47,7 @@ function testHelper(requiresFips, args, expectedOutput, cmd, env) { cmd + ' and args \'' + args + '\''); num_children_spawned++; - function response_handler(response, expectedOutput, expectedError) { + function responseHandler(response, expectedOutput, expectedError) { // We won't always have data on both stdout and stderr. if (response.length > 0) { if (EXIT_FAILURE === expectedOutput) { @@ -61,38 +61,36 @@ function testHelper(requiresFips, args, expectedOutput, cmd, env) { } // Buffer all data received from children on stderr/stdout. - const buffer = { stderr: '', stdout: '' }; - const response_buffer = function(stream, data) { - // Prompt and newline may occur in undefined order. - const response = data.toString().replace(/\n|>/g, '').trim(); - if (response.length > 0) { - buffer[stream] += response; - } - }; + var stderr = ''; + var stdout = ''; + + function filter(data) { + return data.toString().replace(/\n|>/g, '').trim(); + } child.stdout.on('data', function(data) { - response_buffer('stdout', data); + stdout += filter(data); }); child.stderr.on('data', function(data) { - response_buffer('stderr', data); + stderr += filter(data); }); // If using FIPS mode binary, or running a generic test. if (compiledWithFips() || !requiresFips) { child.stdout.on('end', function(data) { - response_handler(buffer['stdout'], + responseHandler(stdout, expectedOutput, FIPS_ERROR_STRING); }); } else { // If using a non-FIPS binary expect a failure. child.stdout.on('end', function() { - response_handler(buffer['stdout'], + responseHandler(stdout, EXIT_FAILURE, FIPS_ERROR_STRING); }); /* Failure to start Node executable is reported on stderr */ child.stderr.on('end', function() { - response_handler(buffer['stderr'], + responseHandler(stderr, EXIT_FAILURE, OPTION_ERROR_STRING); }); } From fbfe12168409fc6d5fbdf012240021f5e41ea54a Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 17 Feb 2016 19:04:00 -0500 Subject: [PATCH 07/14] WIP: Use sync spawn API to vastly simplify test code --- test/parallel/test-crypto-fips.js | 154 +++++++++++++----------------- 1 file changed, 65 insertions(+), 89 deletions(-) diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index 3919b12b823170..6703ef397d28b7 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -1,7 +1,7 @@ 'use strict'; var common = require('../common'); var assert = require('assert'); -var spawn = require('child_process').spawn; +var spawnSync = require('child_process').spawnSync; var path = require('path'); if (!common.hasCrypto) { @@ -9,14 +9,12 @@ if (!common.hasCrypto) { return; } -const EXIT_FAILURE = -1; const FIPS_ENABLED = 1; const FIPS_DISABLED = 0; const FIPS_ERROR_STRING = 'Error: Cannot set FIPS mode'; const OPTION_ERROR_STRING = 'bad option'; const CNF_FIPS_ON = path.join(common.fixturesDir, 'openssl_fips_enabled.cnf'); const CNF_FIPS_OFF = path.join(common.fixturesDir, 'openssl_fips_disabled.cnf'); -var num_children_spawned = 0; var num_children_ok = 0; function compiledWithFips() { @@ -32,175 +30,153 @@ function addToEnv(newVar, value) { return envCopy; } -function childOk(child) { - console.error('Child ' + ++num_children_ok + '/' + num_children_spawned + - ' [pid:' + child.pid + '] OK.'); -} - -function testHelper(requiresFips, args, expectedOutput, cmd, env) { - const child = spawn(process.execPath, args.concat(['-i']), { +function testHelper(stream, args, expectedOutput, cmd, env) { + const fullArgs = args.concat(['-e', 'console.log(' + cmd + ')']); + const child = spawnSync(process.execPath, fullArgs, { cwd: path.dirname(process.execPath), env: env }); console.error('Spawned child [pid:' + child.pid + '] with cmd ' + cmd + ' and args \'' + args + '\''); - num_children_spawned++; - function responseHandler(response, expectedOutput, expectedError) { + function childOk(child) { + console.error('Child #' + ++num_children_ok + + ' [pid:' + child.pid + '] OK.'); + } + + function responseHandler(buffer, expectedOutput) { + const response = buffer.toString(); // We won't always have data on both stdout and stderr. if (response.length > 0) { - if (EXIT_FAILURE === expectedOutput) { - if (-1 === response.indexOf(expectedError)) - assert.notEqual(-1, response.indexOf(expectedError)); + if (FIPS_ENABLED !== expectedOutput && FIPS_DISABLED !== expectedOutput) { + // In the case of expected errors just look for a substring. + assert.notEqual(-1, response.indexOf(expectedOutput)); } else { + // Normal path where we expect either FIPS enabled or disabled. assert.equal(expectedOutput, response); } childOk(child); } } - // Buffer all data received from children on stderr/stdout. - var stderr = ''; - var stdout = ''; - - function filter(data) { - return data.toString().replace(/\n|>/g, '').trim(); - } - - child.stdout.on('data', function(data) { - stdout += filter(data); - }); - - child.stderr.on('data', function(data) { - stderr += filter(data); - }); - - // If using FIPS mode binary, or running a generic test. - if (compiledWithFips() || !requiresFips) { - child.stdout.on('end', function(data) { - responseHandler(stdout, - expectedOutput, FIPS_ERROR_STRING); - }); - } else { - // If using a non-FIPS binary expect a failure. - child.stdout.on('end', function() { - responseHandler(stdout, - EXIT_FAILURE, FIPS_ERROR_STRING); - }); - /* Failure to start Node executable is reported on stderr */ - child.stderr.on('end', function() { - responseHandler(stderr, - EXIT_FAILURE, OPTION_ERROR_STRING); - }); - } - // Wait for write to complete before exiting child, but we won't be able to - // write to the child's stdin if cmd line args are rejected and process dies - // first, resulting in EPIPE. We can avoid EPIPE by not writing anything. - if (null !== cmd) { - child.stdin.setEncoding('utf-8'); - child.stdin.write(cmd + '\n;process.exit()\n'); - } + responseHandler(child[stream], expectedOutput); } // By default FIPS should be off in both FIPS and non-FIPS builds. -testHelper(false, +testHelper( + 'stdout', [], FIPS_DISABLED, 'require("crypto").fips', process.env); // --enable-fips should force FIPS mode on -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', ['--enable-fips'], - compiledWithFips() ? FIPS_ENABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips' : null, + compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING, + 'require("crypto").fips', process.env); //--disable-fips should force FIPS mode on -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', ['--disable-fips'], - compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips' : null, + compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, + 'require("crypto").fips', process.env); // --disable-fips should take precedence over --enable-fips -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', ['--disable-fips', '--enable-fips'], - compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips' : null, + compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, + 'require("crypto").fips', process.env); // --disable-fips and --enable-fips order do not matter -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', ['--enable-fips', '--disable-fips'], - compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips' : null, + compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, + 'require("crypto").fips', process.env); // OpenSSL config file should be able to turn on FIPS mode -testHelper(false, +testHelper( + 'stdout', [], compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, 'require("crypto").fips', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); // --disable-fips should take precedence over OpenSSL config file -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', ['--disable-fips'], - compiledWithFips() ? FIPS_DISABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips' : null, + compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, + 'require("crypto").fips', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); // --enable-fips should take precedence over OpenSSL config file -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', ['--enable-fips'], - compiledWithFips() ? FIPS_ENABLED : EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips' : null, + compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING, + 'require("crypto").fips', addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); // setFipsCrypto should be able to turn FIPS mode on -testHelper(true, [], FIPS_ENABLED, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', + [], + compiledWithFips() ? FIPS_ENABLED : FIPS_ERROR_STRING, '(require("crypto").fips = true,' + 'require("crypto").fips)', process.env); // setFipsCrypto should be able to turn FIPS mode on and off -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', [], - FIPS_DISABLED, + compiledWithFips() ? FIPS_DISABLED : FIPS_ERROR_STRING, '(require("crypto").fips = true,' + 'require("crypto").fips = false,' + 'require("crypto").fips)', process.env); // setFipsCrypto takes precedence over OpenSSL config file, FIPS on -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', [], - compiledWithFips() ? FIPS_ENABLED : FIPS_DISABLED, + compiledWithFips() ? FIPS_ENABLED : FIPS_ERROR_STRING, '(require("crypto").fips = true,' + 'require("crypto").fips)', addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); // setFipsCrypto takes precedence over OpenSSL config file, FIPS off -testHelper(true, +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', [], - FIPS_DISABLED, + compiledWithFips() ? FIPS_DISABLED : FIPS_ERROR_STRING, '(require("crypto").fips = false,' + 'require("crypto").fips)', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); // --disable-fips prevents use of setFipsCrypto API -testHelper(true, +testHelper( + 'stderr', ['--disable-fips'], - EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips = true' : null, + compiledWithFips() ? FIPS_ERROR_STRING : OPTION_ERROR_STRING, + 'require("crypto").fips = true', process.env); // --enable-fips prevents use of setFipsCrypto API -testHelper(true, +testHelper( + 'stderr', ['--enable-fips'], - EXIT_FAILURE, - compiledWithFips() ? 'require("crypto").fips = false' : null, + compiledWithFips() ? FIPS_ERROR_STRING : OPTION_ERROR_STRING, + 'require("crypto").fips = false', process.env); From a208fc417dc74ed413bcbf813820e9d66ac450d0 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 17 Feb 2016 19:46:15 -0500 Subject: [PATCH 08/14] WIP: Rework cmd line args and tests --- src/node.cc | 6 +-- src/node.h | 2 +- src/node_crypto.cc | 19 ++++---- test/parallel/test-crypto-fips.js | 80 +++++++++++++++---------------- 4 files changed, 51 insertions(+), 56 deletions(-) diff --git a/src/node.cc b/src/node.cc index a55fbbb9e8c446..655e2d28a7608e 100644 --- a/src/node.cc +++ b/src/node.cc @@ -163,8 +163,8 @@ bool no_deprecation = false; #if HAVE_OPENSSL && NODE_FIPS_MODE // used by crypto module -bool disable_fips_crypto = false; bool enable_fips_crypto = false; +bool force_fips_crypto = false; #endif // process-relative uptime base, initialized at start-up @@ -3435,10 +3435,10 @@ static void ParseArgs(int* argc, } else if (strncmp(arg, "--tls-cipher-list=", 18) == 0) { default_cipher_list = arg + 18; #if NODE_FIPS_MODE - } else if (strcmp(arg, "--disable-fips") == 0) { - disable_fips_crypto = true; } else if (strcmp(arg, "--enable-fips") == 0) { enable_fips_crypto = true; + } else if (strcmp(arg, "--force-fips") == 0) { + force_fips_crypto = true; #endif /* NODE_FIPS_MODE */ #endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) diff --git a/src/node.h b/src/node.h index 479f74fced7e1e..fcbb45085c5439 100644 --- a/src/node.h +++ b/src/node.h @@ -180,8 +180,8 @@ namespace node { NODE_EXTERN extern bool no_deprecation; #if HAVE_OPENSSL && NODE_FIPS_MODE -NODE_EXTERN extern bool disable_fips_crypto; NODE_EXTERN extern bool enable_fips_crypto; +NODE_EXTERN extern bool force_fips_crypto; #endif NODE_EXTERN int Start(int argc, char *argv[]); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c59befe2a6313e..0916d9016cf172 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5674,12 +5674,12 @@ void InitCryptoOnce() { CRYPTO_THREADID_set_callback(crypto_threadid_cb); #ifdef NODE_FIPS_MODE - /* Override FIPS settings in cnf file. */ - int err = 0; - if (disable_fips_crypto && !FIPS_mode_set(0)) { - err = ERR_get_error(); - } else if (!disable_fips_crypto && enable_fips_crypto && !FIPS_mode_set(1)) { - err = ERR_get_error(); + /* Override FIPS settings in cnf file, if needed. */ + unsigned long err = 0; + if (enable_fips_crypto || force_fips_crypto) { + if (0 == FIPS_mode() && !FIPS_mode_set(1)) { + err = ERR_get_error(); + } } if (0 != err) { fprintf(stderr, "openssl fips failed: %s\n", ERR_error_string(err, NULL)); @@ -5760,12 +5760,9 @@ void SetFipsCrypto(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); #ifdef NODE_FIPS_MODE bool mode = args[0]->BooleanValue(); - if (disable_fips_crypto) { - return env->ThrowError( - "Cannot set FIPS mode, it was forced with --disable-fips at startup."); - } else if (enable_fips_crypto) { + if (force_fips_crypto) { return env->ThrowError( - "Cannot set FIPS mode, it was forced with --enable-fips at startup."); + "Cannot set FIPS mode, it was forced with --force-fips at startup."); } else if (!FIPS_mode_set(mode)) { unsigned long err = ERR_get_error(); return ThrowCryptoError(env, err); diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index 6703ef397d28b7..feac9804df89ad 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -47,17 +47,15 @@ function testHelper(stream, args, expectedOutput, cmd, env) { function responseHandler(buffer, expectedOutput) { const response = buffer.toString(); - // We won't always have data on both stdout and stderr. - if (response.length > 0) { - if (FIPS_ENABLED !== expectedOutput && FIPS_DISABLED !== expectedOutput) { - // In the case of expected errors just look for a substring. - assert.notEqual(-1, response.indexOf(expectedOutput)); - } else { - // Normal path where we expect either FIPS enabled or disabled. - assert.equal(expectedOutput, response); - } - childOk(child); + assert.notEqual(0, response.length); + if (FIPS_ENABLED !== expectedOutput && FIPS_DISABLED !== expectedOutput) { + // In the case of expected errors just look for a substring. + assert.notEqual(-1, response.indexOf(expectedOutput)); + } else { + // Normal path where we expect either FIPS enabled or disabled. + assert.equal(expectedOutput, response); } + childOk(child); } responseHandler(child[stream], expectedOutput); @@ -71,7 +69,7 @@ testHelper( 'require("crypto").fips', process.env); -// --enable-fips should force FIPS mode on +// --enable-fips should turn FIPS mode on testHelper( compiledWithFips() ? 'stdout' : 'stderr', ['--enable-fips'], @@ -79,27 +77,11 @@ testHelper( 'require("crypto").fips', process.env); -//--disable-fips should force FIPS mode on +//--force-fips should turn FIPS mode on testHelper( compiledWithFips() ? 'stdout' : 'stderr', - ['--disable-fips'], - compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, - 'require("crypto").fips', - process.env); - -// --disable-fips should take precedence over --enable-fips -testHelper( - compiledWithFips() ? 'stdout' : 'stderr', - ['--disable-fips', '--enable-fips'], - compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, - 'require("crypto").fips', - process.env); - -// --disable-fips and --enable-fips order do not matter -testHelper( - compiledWithFips() ? 'stdout' : 'stderr', - ['--enable-fips', '--disable-fips'], - compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, + ['--force-fips'], + compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING, 'require("crypto").fips', process.env); @@ -111,18 +93,18 @@ testHelper( 'require("crypto").fips', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); -// --disable-fips should take precedence over OpenSSL config file +// --enable-fips should take precedence over OpenSSL config file testHelper( compiledWithFips() ? 'stdout' : 'stderr', - ['--disable-fips'], - compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, + ['--enable-fips'], + compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING, 'require("crypto").fips', - addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); + addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); -// --enable-fips should take precedence over OpenSSL config file +// --force-fips should take precedence over OpenSSL config file testHelper( compiledWithFips() ? 'stdout' : 'stderr', - ['--enable-fips'], + ['--force-fips'], compiledWithFips() ? FIPS_ENABLED : OPTION_ERROR_STRING, 'require("crypto").fips', addToEnv('OPENSSL_CONF', CNF_FIPS_OFF)); @@ -164,19 +146,35 @@ testHelper( 'require("crypto").fips)', addToEnv('OPENSSL_CONF', CNF_FIPS_ON)); -// --disable-fips prevents use of setFipsCrypto API +// --enable-fips does not prevent use of setFipsCrypto API +testHelper( + compiledWithFips() ? 'stdout' : 'stderr', + ['--enable-fips'], + compiledWithFips() ? FIPS_DISABLED : OPTION_ERROR_STRING, + '(require("crypto").fips = false,' + + 'require("crypto").fips)', + process.env); + +// --force-fips prevents use of setFipsCrypto API testHelper( 'stderr', - ['--disable-fips'], + ['--force-fips'], compiledWithFips() ? FIPS_ERROR_STRING : OPTION_ERROR_STRING, - 'require("crypto").fips = true', + 'require("crypto").fips = false', process.env); -// --enable-fips prevents use of setFipsCrypto API +// --force-fips and --enable-fips order does not matter testHelper( 'stderr', - ['--enable-fips'], + ['--force-fips', '--enable-fips'], compiledWithFips() ? FIPS_ERROR_STRING : OPTION_ERROR_STRING, 'require("crypto").fips = false', process.env); +//--enable-fips and --force-fips order does not matter +testHelper( + 'stderr', + ['--enable-fips', '--force-fips'], + compiledWithFips() ? FIPS_ERROR_STRING : OPTION_ERROR_STRING, + 'require("crypto").fips = false', + process.env); From 66cbd1a456b8dd916306029043184870f9dd2bec Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 17 Feb 2016 21:22:09 -0500 Subject: [PATCH 09/14] WIP: Call OpenSSL_init before RAND_* --- src/node.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/node.cc b/src/node.cc index 655e2d28a7608e..a9569c782f9d88 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4239,6 +4239,11 @@ int Start(int argc, char** argv) { Init(&argc, const_cast(argv), &exec_argc, &exec_argv); #if HAVE_OPENSSL +#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 // V8 on Windows doesn't have a good source of entropy. Seed it from // OpenSSL's pool. V8::SetEntropySource(crypto::EntropySource); From 48b05fd8d805a840e0c09712a4fdcc2d6b990465 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 17 Feb 2016 13:48:27 -0500 Subject: [PATCH 10/14] WIP: Fix spurious parens & stale docs --- src/node.cc | 4 ++-- src/node_crypto.cc | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node.cc b/src/node.cc index a9569c782f9d88..564a7a5723f701 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3289,8 +3289,8 @@ static void PrintHelp() { #if HAVE_OPENSSL " --tls-cipher-list=val use an alternative default TLS cipher list\n" #if NODE_FIPS_MODE - " --disable-fips force use of crypto without FIPS compliance" - " --enable-fips force use of crypto with FIPS compliance" + " --enable-fips enable FIPS compliant crypto on startup" + " --force-fips force FIPS compliant crypto (cannot disable)" #endif /* NODE_FIPS_MODE */ #endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 0916d9016cf172..873a39c09dc130 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3860,7 +3860,7 @@ SignBase::Error Sign::SignFinal(const char* key_pem, #ifdef NODE_FIPS_MODE /* Validate DSA2 parameters from FIPS 186-4 */ - if (FIPS_mode() && (EVP_PKEY_DSA == pkey->type)) { + if (FIPS_mode() && EVP_PKEY_DSA == pkey->type) { size_t L = BN_num_bits(pkey->pkey.dsa->p); size_t N = BN_num_bits(pkey->pkey.dsa->q); bool result = false; From b9647c3e0b1071f2955ffc164f509bd46f3066e3 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 17 Feb 2016 13:57:20 -0500 Subject: [PATCH 11/14] WIP: Rename hasFipsCrypto to getFipsCrypto --- lib/crypto.js | 4 ++-- src/node_crypto.cc | 4 ++-- test/common.js | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/crypto.js b/lib/crypto.js index 664d88397a36aa..cf92eff1eec0bc 100644 --- a/lib/crypto.js +++ b/lib/crypto.js @@ -11,7 +11,7 @@ try { var getCiphers = binding.getCiphers; var getHashes = binding.getHashes; var getCurves = binding.getCurves; - var hasFipsCrypto = binding.hasFipsCrypto; + var getFipsCrypto = binding.getFipsCrypto; var setFipsCrypto = binding.setFipsCrypto; } catch (e) { throw new Error('Node.js is not compiled with openssl crypto support'); @@ -648,7 +648,7 @@ exports.getCurves = function() { }; Object.defineProperty(exports, 'fips', { - get: hasFipsCrypto, + get: getFipsCrypto, set: setFipsCrypto }); diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 873a39c09dc130..85fefe0bdf2ba8 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5748,7 +5748,7 @@ void SetEngine(const FunctionCallbackInfo& args) { } #endif // !OPENSSL_NO_ENGINE -void HasFipsCrypto(const FunctionCallbackInfo& args) { +void GetFipsCrypto(const FunctionCallbackInfo& args) { if (FIPS_mode()) { args.GetReturnValue().Set(1); } else { @@ -5795,7 +5795,7 @@ void InitCrypto(Local target, #ifndef OPENSSL_NO_ENGINE env->SetMethod(target, "setEngine", SetEngine); #endif // !OPENSSL_NO_ENGINE - env->SetMethod(target, "hasFipsCrypto", HasFipsCrypto); + env->SetMethod(target, "getFipsCrypto", GetFipsCrypto); env->SetMethod(target, "setFipsCrypto", SetFipsCrypto); env->SetMethod(target, "PBKDF2", PBKDF2); env->SetMethod(target, "randomBytes", RandomBytes); diff --git a/test/common.js b/test/common.js index 9562b2859ad28a..a64b6d6bc91a5d 100644 --- a/test/common.js +++ b/test/common.js @@ -161,7 +161,7 @@ Object.defineProperty(exports, 'hasCrypto', { Object.defineProperty(exports, 'hasFipsCrypto', { get: function() { - return exports.hasCrypto && require('crypto').hasFipsCrypto; + return exports.hasCrypto && require('crypto').getFipsCrypto; } }); From a8cb1ade9ed68c68da6c3506a3b684d13948acd4 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Wed, 17 Feb 2016 14:03:32 -0500 Subject: [PATCH 12/14] WIP: Fix line endings --- src/node.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node.cc b/src/node.cc index 564a7a5723f701..c3445e28217bf1 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3289,8 +3289,8 @@ static void PrintHelp() { #if HAVE_OPENSSL " --tls-cipher-list=val use an alternative default TLS cipher list\n" #if NODE_FIPS_MODE - " --enable-fips enable FIPS compliant crypto on startup" - " --force-fips force FIPS compliant crypto (cannot disable)" + " --enable-fips enable FIPS crypto at startup\n" + " --force-fips force FIPS crypto (cannot be disabled)\n" #endif /* NODE_FIPS_MODE */ #endif /* HAVE_OPENSSL */ #if defined(NODE_HAVE_I18N_SUPPORT) From a23f5e3556195375cc19593991d96ee69fba4a86 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Mon, 22 Feb 2016 17:37:00 -0500 Subject: [PATCH 13/14] WIP: Update stale API in test --- test/common.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common.js b/test/common.js index a64b6d6bc91a5d..cc54c65c7b2892 100644 --- a/test/common.js +++ b/test/common.js @@ -161,7 +161,7 @@ Object.defineProperty(exports, 'hasCrypto', { Object.defineProperty(exports, 'hasFipsCrypto', { get: function() { - return exports.hasCrypto && require('crypto').getFipsCrypto; + return exports.hasCrypto && require('crypto').fips; } }); From 1d2806413632826331a6d67becb53cf51c25e0b3 Mon Sep 17 00:00:00 2001 From: Stefan Budeanu Date: Mon, 22 Feb 2016 18:31:32 -0500 Subject: [PATCH 14/14] WIP: Allow tests to survive polluted env --- test/parallel/test-crypto-fips.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/parallel/test-crypto-fips.js b/test/parallel/test-crypto-fips.js index feac9804df89ad..abccb450ad8681 100644 --- a/test/parallel/test-crypto-fips.js +++ b/test/parallel/test-crypto-fips.js @@ -67,7 +67,7 @@ testHelper( [], FIPS_DISABLED, 'require("crypto").fips', - process.env); + addToEnv('OPENSSL_CONF', '')); // --enable-fips should turn FIPS mode on testHelper( @@ -116,7 +116,7 @@ testHelper( compiledWithFips() ? FIPS_ENABLED : FIPS_ERROR_STRING, '(require("crypto").fips = true,' + 'require("crypto").fips)', - process.env); + addToEnv('OPENSSL_CONF', '')); // setFipsCrypto should be able to turn FIPS mode on and off testHelper( @@ -126,7 +126,7 @@ testHelper( '(require("crypto").fips = true,' + 'require("crypto").fips = false,' + 'require("crypto").fips)', - process.env); + addToEnv('OPENSSL_CONF', '')); // setFipsCrypto takes precedence over OpenSSL config file, FIPS on testHelper(