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.

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

Allow enabling FIPS mode from an environment variable #10938

Closed
gibfahn opened this issue Jan 21, 2017 · 11 comments · Fixed by #11006
Closed

Allow enabling FIPS mode from an environment variable #10938

gibfahn opened this issue Jan 21, 2017 · 11 comments · Fixed by #11006
Labels
crypto Issues and PRs related to the crypto subsystem.

Comments

@gibfahn
Copy link
Member

gibfahn commented Jan 21, 2017

Problem

As of cae9eb35f0, it is no longer possible to enable FIPS mode with an environment variable. This change was to prevent security issues caused by the misuse of the $OPENSSL_CONF variable. However this means it is no longer possible to test that the FIPS binary actually fails as expected in CitGM.

We can expect (for example) citgm ws to fail with:

error: | 4) WebSocketServer hixie mode connection establishing handles messages passed along with the upg
error: | Uncaught Error: error:060A80A3:digital envelope routines:FIPS_DIGESTINIT:disabled for fips      
error: | at Error (native)                                                                               
error: | at new Hash (crypto.js:56:18)                                                                   
error: | at Object.Hash (crypto.js:55:12)                                                                
error: | at WebSocketServer.completeHandshake (lib/WebSocketServer.js:411:24)                            
error: | at onClientVerified (lib/WebSocketServer.js:471:25)                                             
error: | at WebSocketServer.handleHixieUpgrade (lib/WebSocketServer.js:526:3)                            
error: | at WebSocketServer.handleUpgrade (lib/WebSocketServer.js:172:61)                                
error: | at Server.WebSocketServer._onServerUpgrade (lib/WebSocketServer.js:89:12)                       
error: | at onParserExecuteCommon (_http_server.js:409:14)                                               
error: | at HTTPParser.onParserExecute (_http_server.js:377:5)                         
Node Version How to enable FIPS Command to test
v4 On by default citgm ws
v6.0.0..v6.8.1 Requires env var OPENSSL_CONF=/path/to/openssl_fips_enabled.cnf citgm ws
v6.9.0.. Command line flag only mkdir bin; printf '#!/bin/sh'"\n/path/to/node --force-fips "'$*'>bin/node; export PATH=$PWD/bin:$PATH; citgm ws (doesn't work as child processes are spawned).

Possible solution

Allow OPENSSL_FIPS=enable to enable FIPS mode, but don't provide an equivalent to disable it, I don't think this causes any security issues.

Refs:

Turn off FIPS by default: #5181
Discussion of OPENSSL_FIPS: #3820
PR to ignore OPENSSL_CONF: https://github.com/nodejs/node-private/pull/82

cc/ @rvagg @bnoordhuis @shigeki @mhdawson @gdams @sxa555

@gibfahn gibfahn added the crypto Issues and PRs related to the crypto subsystem. label Jan 21, 2017
@gdams
Copy link
Member

gdams commented Jan 22, 2017

I am personally slightly confused as to what security difference there would be between using an environment variable to set the config file rather than passing it as a flag. Looking at the PR it seems to be implying that the issue was with attackers being able to swap out the default config file from a known location. So my question is what is the difference between the two commands below?

node --force-fips /path/to/openssl_fips_enabled.cnf
export OPENSSL_CONF=/path/to/openssl_fips_enabled.cnf

We are still using the same path so surely an attacker could still modify/change the config file?
@rvagg perhaps you can explain this to me?

@bnoordhuis
Copy link
Member

The security issue was that node unconditionally loaded a config file. The OPENSSL_CONF variable only influenced where it looked, not if. On Windows, it was in a location that is usually writable by other users.

Support for OPENSSL_CONF could be brought back. I'm not a huge fan of the side channel nature of environment variables, though.

@gdams
Copy link
Member

gdams commented Jan 23, 2017

Ahh okay, thanks for clearing that up, The only issue with using a flag is that we are seriously reducing the usability of fips. Not only are we unable to spawn child processes of node (such as in citgm) but I would also imagine that this prevents us from using clusters too?

@sam-github
Copy link
Contributor

Setting OPENSSL_CONF=/dev/null would cause node to not use a conf file.

Command line flags are difficult to control compared to env vars, the node invocation is often hidden (such as inside a batch or shell script).

Since it it was windows in particular that doesn't store OpenSSL's conf file in a secure location by default, how about we bring back the default loading of the conf file on non-Windows, and the env var that controls the location?

@bnoordhuis
Copy link
Member

The environment variable might be acceptable but I don't like the idea of a default config file, it's very implicit and un-node-y.

Meta: I don't understand why FIPS is configurable at runtime in the first place. In order to tick those government contract checkboxes, crypto needs to be locked down with no way to disable or override it. I would have expected separate FIPS-only binaries.

@gibfahn
Copy link
Member Author

gibfahn commented Jan 24, 2017

@bnoordhuis Separate FIPS-only binaries is how it worked in v4, it was changed for v6 as a result of #3819. It might be a discussion we should reconsider, but I guess people want to use their FIPS node binaries to npm install things.

So are you saying that you're fine with loading an OpenSSL config file if OPENSSL_CONF=/path/to/file is set, but not ok with having a default location that always gets loaded if it exists (like /usr/local/ssl/openssl.cnf).

@bnoordhuis
Copy link
Member

@gibfahn Correct.

@mhdawson @stefanmb See #10938 (comment) - I have no love for FIPS and it's not my department but doesn't a runtime knob weaken its security guarantees? Perhaps something to reconsider if you agree.

@mhdawson
Copy link
Member

@bnoordhuis I started at the same point thinking that you'd just want to know it was on. However, there was strong push from community members who wanted/needed the runtime switch for their use cases and so it was added in 6.x.

@mhdawson
Copy link
Member

From the discussion sounds like we have consensus that adding back the option to set the config file with OPENSSL_CONF without any fallback default (ie unless you specify it through env or command line no default file will be opened) . Seems like the next step is to submit a PR for that.

@sam-github
Copy link
Contributor

I'll do that tomorrow.

@sam-github
Copy link
Contributor

#11006

sam-github added a commit to sam-github/node that referenced this issue Feb 9, 2017
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: nodejs#10938
PR-URL: nodejs#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
sam-github added a commit to sam-github/node that referenced this issue Feb 13, 2017
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: nodejs#10938
PR-URL: nodejs#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
sam-github added a commit to sam-github/node that referenced this issue Feb 16, 2017
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: nodejs#10938
PR-URL: nodejs#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit that referenced this issue Feb 16, 2017
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: #10938
PR-URL: #11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit that referenced this issue Feb 22, 2017
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: #10938
PR-URL: #11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
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: nodejs#10938
PR-URL: nodejs#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
sam-github added a commit to sam-github/node that referenced this issue Apr 17, 2017
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: nodejs#10938
PR-URL: nodejs#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this issue May 16, 2017
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: #10938
PR-URL: #11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this issue May 18, 2017
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: #10938
PR-URL: #11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
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: nodejs/node#10938
PR-URL: nodejs/node#11006
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants