Add a check of the OPENSSL_FIPS environment variable around each call…

[lordjabez]

As currently implemented, when Node is compiled with FIPS support
(./configure fips), there is no way to disable engaging FIPS mode
during execution (Issue #3819). This means that several functions that rely on
non-FIPS approved algorithms (e.g. md5 hashing) will fail, as will
any code that depends on them (most obviously, npm).

What seems needed to me is a way to explicitly enable or disable
FIPS operation each time node is invoked. The way this is done
with the openssl CLI is via the OPENSSL_FIPS environment variable.

This change adds a check to OPENSSL_FIPS at every place where
FIPS_mode_set(1) is called (which enables FIPS mode). If Node
is not compiled in FIPS mode these calls will not even be
compiled since they're all wrapped with IFDEFs.

Those who are trying to run Node.js in FIPS mode should be
familiar with this variable and using it will be natural.

[jasnell]

@nodejs/crypto @shigeki @mhdawson @indutny

[bnoordhuis]

We have an 'upstream first' policy for our dependencies. This PR as-is is not something we can accept.

[jasnell]

Curious... are the changes in the openssl src necessary for this?

[lordjabez]

In order to build Node.js with FIPS support, the OpenSSL FIPS module and the OpenSSL source must also be recompiled first, though no source changes are required.

Details on OpenSSL and FIPS can be found here: https://www.openssl.org/docs/fipsnotes.html

[lordjabez]

@bnoordhuis I'm a bit new to OSS contributing, can you elaborate on what I did wrong and how to fix the PR?

[mhdawson]

We'll have to look more closely but I thought the change would be more along the lines of just calling FIPS_mode_set(0) as opossed to FIPS_mode_set(1) which would be handled through a Node option.

https://wiki.openssl.org/index.php/FIPS_mode_set%28%29

[indutny]

I would rather make it --no-fips, though. FIPS-enabled node.js binaries should run in this mode by default.

[lordjabez]

@indutny On the contrary, the typical behavior of FIPS capable applications is to not activate FIPS mode unless explicitly commanded. For example, this is how the FIPS validated openssl command line utility works. I cribbed the environment variable from the openssl implementation, in fact. So it'd be quite natural for folks who do FIPS to already have that variable set in their operating environments.

Besides, having FIPS mode active by default at runtime makes using a FIPS capable Node build rather difficult, esp. when it's being used "under the hood" by other utilities. This whole issue came up for me because npm wouldn't run due to its use of non-FIPS hashing algorithms. If FIPS is on by default, do I now need to explicitly disable it every time I run npm or any other of a hundred different CLI tools that use FIPS under the hood? That becomes even more painful if this switch is via a switch (vs environment variable) because now I have to hack around on the npm scripts themselves.

[lordjabez]

There is actually one check for OPENSSL_FIPS already in the codebase (

node/deps/openssl/openssl/apps/openssl.c

Line 311 in 6fff47f

if (getenv("OPENSSL_FIPS")) {

) so my suggestion is not unprecedented.

[stefanmb]

@lordjabez

When compiling with FIPS you're supposed to grab the OpenSSL source from a secure path (i.e. snail mail) and not from Node's copy of it. If you modify that source you are outside the scope of OpenSSL's certificate and can no longer claim you use a FIPS verified cryptographic provider. You'll have to submit (and pay) a request for your own validation on this modified source code.

Edit: This doesn't actually apply to the PR. Please ignore!

[lordjabez]

@stefanmb My understanding is that's not entirely accurate. The only part of OpenSSL that must be acquired via snail mail and built unmodified is the FIPS object module (a process I've already gone through), which is not the same as the OpenSSL source itself.

Then when I build Node.js, I point it at that object code with ./configure --openssl-fips=/usr/local/ssl/fips-2.0 per the readme, which I presume then links against the validated module. Hence the FIPS "secret sauce" is used unmolested, thus is validated.

Unless there's something I'm missing?

[stefanmb]

@lordjabez You are absolutely correct! My comment does not apply to what you're doing, sorry for the confusion.

[stefanmb]

@lordjabez I think what @bnoordhuis was saying is that they won't accept changes to dependencies, unless they flow back from the original projects, i.e. this change would have to flow back from OpenSSL itself.

[shigeki]

Looking at openssl sources, I still do not figure out why patches to openssl sources are needed. There are two files in this PR.

• evp_cnf.c This is used for parsing openssl.conf in apps/openssl command. This command is only used for testing in node. Even if you want to use it, fips would be disabled by writing fips_mode = off in the configration though I've not tested it yet.
• ssltest.c This is not included in building node and not used. See deps/openssl/openssl.gypi.

I'm +1 for adding --no-fips or --disable-fips option rather than environment. While it needs to change global installed commands like npm to add the option explicitly in hash bang, this helps to avoid the risk to run node in disabled fips accidentally.

[jasnell]

@lordjabez ... yes, @stefanmb is correct. The main issue with this PR is the fact that it touches two files in the /deps/openssl directory. We can only accept such changes if they are first landed in OpenSSL itself. If/when that happens, we can either (a) update the entire openssl source or (b) float the patch that landed in OpenSSL back here. We try to do (b) very rarely and prefer (a).

If there's a way to resolve the issue strictly through changes in the nodejs src, then awesome, let's get this PR updated and reviewed. Otherwise, if the changes to OpenSSL are required for this to be resolved, you'll need to first submit the changes to OpenSSL and get them landed there.

[lordjabez]

@jasnell That makes sense. Let me do some further investigation to see if there might be another solution.

[lordjabez]

Good news: only one file really needs to change. I'll update the pull request.

[lordjabez]

For those who still don't agree with using the environment variable, do a search on "OPENSSL_FIPS variable" and you'll see it's a common pattern for applications compiled with FIPS support to default to "FIPS mode off" at runtime, with it only being enabled when this variable is set.

My apologies for belaboring the point, but I do believe it's the right approach.

[chrisamoore]

👍

[jasnell]

Thank you for updating! @nodejs/crypto ... thoughts?

[indutny]

@jasnell My opinion on this is the same. It should be enabled by default, and there should be a way to turn it off. Considering that this is a security thing, does it makes sense to move it to command-line arguments instead of ENV?

[jasnell]

@indutny +1... @lordjabez, I recognize that the environment OPENSSL_FIPS environment variable may be used in other locations as a common pattern but in node core the attempt (at least) is to have the more secure option enabled by default and only optionally switched off -- not the other way around. Also, there's been a preference towards using command line switches as opposed to using environment variables for security related switches (see #2412 for another example of this). Given that precedence, I have to agree with @indutny

I appreciate your patience on this and definitely welcome the contribution. Just please bear with us a bit longer as we just want to make sure that the change is the correct one to make. ;-)

[shigeki]

I think that we need to have process.fips or whatever to know wheather fips mode is enabled or not and change common.hasFipsCrypto when we have a command line option.

[jasnell]

@shigeki ... sounds reasonable :-)

[lordjabez]

The desire for command line switch makes sense, but I'm still stuck on the insistence that FIPS mode be enabled by default, because of how it will likely have unintended side effects in use cases where strict crypto is not a concern, and because all the other software I know of that is FIPS capable defaults it to off (e.g. openssl itself, Apache, Tomcat, Firefox, Windows, even RHEL).

But if the most egregious of those cases can be handled reasonably (I'd think at minimum npm needs to work), then it's probably fine.

[indutny]

@lordjabez we default to FIPS off as well. It should be explicitly enabled during the build phase.

However, considering that everyone does it, perhaps there may be a reason to follow the de-facto standard here. Still I would like it to be a command-line option, not an environment variable.

[mhdawson]

I'm +1 for following a commonly accepted approach. @lordjabez I searched on "OPENSSL_FIPS variable" but did not get a good set of results. Any chance you can include the links that shows that off by default and then an environment variable is used for Apache, Tomcat, Firefox, Windows,etc. In particular if there are examples of other languages (ex ruby, python, etc.) doing that it would be particularly useful.

[ScarletTanager]

I also agree with pursuing a commonly accepted approach. Having said that, I am unaware of the OPENSSL_FIPS variable being a common mechanism for controlling OpenSSL's behavior within an application (it does seem to have been a supported mechanism in 1.x releases of the OpenSSL FIPS module, but I would definitely not use those releases anymore). If you wish to have a particular OpenSSL installation run in FIPS mode by default, the usual mechanism is to enable this in the OpenSSL configuration file - see here for more details. You would use fips_mode = yes|no to control OpenSSL's behavior. Individual applications can actually load their own OpenSSL configurations as well, thereby overriding defaults. The use of the config is how you enable FIPS mode for e.g. using OpenSSL in kernel modules.

For applications (including other languages), the most common approach I've seen is to expose the FIPS_mode_set() call directly and let the application choose whether or not to run in FIPS mode. In ruby you can call this directly via OpenSSL.fips_mode = true|false, but it does not seem to be so straightforward in Python, judging from what I could find on the subject. In the wrapper API we're developing for go we do expose this function. The OpenSSL FIPS user's guide says explicitly, "Applications that utilize FIPS mode must call the FIPS_mode_set() function." (See p.22 of the user guide.)

[ScarletTanager]

So following on my previous comment, I would ask if we could add a wrapper for FIPS_mode_set() to the Node API?

[lordjabez]

@ScarletTanager that sounds like the best suggestion so far. I'll look into adjusting the implementation to do just that.

[mhdawson]

I think that we'd want both a command line option as well as allowing the application to make the call. That way FIPs aware applications can do the right thing but at the same time existing applications can be run in FIPs mode by using the command line option.

[indutny]

Makes sense!

[lordjabez]

I'm happy to take a stab at the modifications. Might need a couple days though.

[mhdawson]

@lordjabez sounds good, let me know if you want any pointers as to where in the code command line processing is located. I just created a PR that needed to add one for different reason so its fresh in my mind.

[mhdawson]

@lordjabez any update on this ? If you are not going to get to it @stefanmb may take a crack at it.

[mhdawson]

I'm also wondering if we should close this PR and continue the discussion in #3819

[lordjabez]

I apologize, I've been away due to the holiday. Will dive back in tonight.

[stefanmb]

Hi folks, so this PR and issue fell #3819 fell off the map for a bit but I think we should finish it up.

I've prototyped most of everyone's suggestions, and it wasn't very difficult, but I have some issues regarding the actual requirements.

Here is what has been suggested so far:

(1) When compiling with FIPS support, start without FIPS enabled (Suggested by @ScarletTanager and @lordjabez).
(2) Introduce a --disable-fips command line switch (Suggested by @shigeki and @jasnell).
(3) Turn on FIPS by using the OpenSSL configuration file (Suggested by @ScarletTanager).

So what I have right now works as follows: New API has been introduced, hasFipsCrypto is now part of the crypto API and is a thin wrapper around FIPS_mode(). We also have the possibility of adding a wrapper around FIPS_mode_set(), as suggested by ScarletTanager). InitCyrptoOnce now calls OPENSSL_config. Note that reading the config file not only affects FIPS, but other OpenSSL settings.

To concretely see what I've been doing, look in this branch: https://github.com/stefanmb/node/tree/fips-switch

Outstanding Issues:

(a) Do we want a FIPS build to start by default in non-FIPS mode? Doing so changes behaviour vis-a-vis existing releases.
(b) Do we want to override the OpenSSL config file with --disable-fips?
(c) Do we want to provide a command line switch to give Node.js the path to the OpenSSL config file? e.g. --openssl-cnf=/foo/bar.cnf If we use the global OPENSSL_CONF environment variable, all apps using OpenSSL will be affected.
(d) Do we actually want a Node.js JavaScript wrapper to allow an application to turn FIPS mode on/off? Note that a native module could do that anyway.
(e) Note that this all complicates testing, the FIPS builds now have to be tested in both FIPS and non-FIPS mode.

Thanks, once I have a clearer view of these requirements I'll add tests and open a new PR to supersede this one.

[lordjabez]

Here's my thoughts on the matter, from someone actually using Node in FIPS
mode today:

a) I prefer it start in non-FIPS mode by default, since 95% of my node apps
don't require it
b) I'd prefer not to use the global OpenSSL config file at all, but if we
do, I definitely want the ability to override it
c) Not the way I'd expect to use it; this seems to be a bit kludgey to me
d) I think that'd be useful

Jud

"The making of things is in my heart from my own making by Thee." - J.R.R.
Tolkien

On Tue, Jan 26, 2016 at 2:36 PM, Stefan Budeanu [email protected]
wrote:

Hi folks, so this PR and issue fell #3819
#3819 fell off the map for a bit
but I think we should finish it up.

I've prototyped most of everyone's suggestions, and it wasn't very
difficult, but I have some issues regarding the actual requirements.

Here is what has been suggested so far:

(1) When compiling with FIPS support, start without FIPS enabled
(Suggested by @ScarletTanager https://github.com/ScarletTanager and
@lordjabez https://github.com/lordjabez).
(2) Introduce a --disable-fips command line switch (Suggested by @shigeki
https://github.com/shigeki and @jasnell https://github.com/jasnell).
(3) Turn on FIPS by using the OpenSSL configuration file (Suggested by
@ScarletTanager https://github.com/ScarletTanager).

So what I have right now works as follows: New API has been introduced,
hasFipsCrypto is now part of the crypto API and is a thin wrapper around
FIPS_mode(). We also have the possibility of adding a wrapper around
FIPS_mode_set(), as suggested by ScarletTanager). InitCyrptoOnce now calls
OPENSSL_config. Note that reading the config file not only affects FIPS,
but other OpenSSL settings.

To concretely see what I've been doing, look in this branch:
https://github.com/stefanmb/node/tree/fips-switch

Outstanding Issues:

(a) Do we want a FIPS build to start by default in non-FIPS mode? Doing so
changes behaviour vis-a-vis existing releases.
(b) Do we want to override the OpenSSL config file with --disable-fips?
(c) Do we want to provide a command line switch to give Node.js the path
to the OpenSSL config file? e.g. --openssl-cnf=/foo/bar.cnf If we use the
global OPENSSL_CONF environment variable, all apps using OpenSSL will be
affected.
(d) Do we actually want a Node.js JavaScript wrapper to allow an
application to turn FIPS mode on/off? Note that a native module could do
that anyway.
(e) Note that this all complicates testing, the FIPS builds now have to be
tested in both FIPS and non-FIPS mode.

Thanks, once I have a clearer view of these requirements I'll add tests
and open a new PR to supersede this one.

—
Reply to this email directly or view it on GitHub
#3820 (comment).

[ScarletTanager]

My thoughts:

(a) Do we want a FIPS build to start by default in non-FIPS mode? Doing so changes behaviour vis-a-vis existing releases.

Yes.

(b) Do we want to override the OpenSSL config file with --disable-fips?

I'm fine with that, but if we expose FIPS_MODE_set() in the API, then I don't care as much, honestly. Exposing FIPS_MODE_set() is a higher priority to me.

(c) Do we want to provide a command line switch to give Node.js the path to the OpenSSL config file? e.g. --openssl-cnf=/foo/bar.cnf If we use the global OPENSSL_CONF environment variable, all apps using OpenSSL will be affected.

I'm somewhat agnostic on this, but I see this as a lower priority convenience feature, not something we need "right now." I will note that the OpenSSL API itself provides methods for configuring OpenSSL and for reading in a specific config file - if those are exposed at some point, I think that might obviate any necessity for this feature.

(d) Do we actually want a Node.js JavaScript wrapper to allow an application to turn FIPS mode on/off? Note that a native module could do that anyway.

YES.

(e) Note that this all complicates testing, the FIPS builds now have to be tested in both FIPS and non-FIPS mode.

Point taken, but I think that (d) is a pretty important feature to have/support, since it's the way most anyone familiar with OpenSSL will expect things to work. I think (a+d) is table stakes for me as a next logical step.

[stefanmb]

I have made PR #5181 to resolve outstanding issues here. I suggest closing this PR and continuing the discussion in #5181. Thanks.

[mhdawson]

Since it looks like #5181 is well on its way going to close this one