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

crypto: support FIPS mode of OpenSSL #1890

Closed
wants to merge 7 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Jun 3, 2015

Support building and running with FIPS-compliant OpenSSL. The process is
following:

  1. Download and verify openssl-fips-x.x.x.tar.gz from
    https://www.openssl.org/source/
  2. Extract source to openssl-fips folder
  3. cd openssl-fips && ./config fipscanisterbuild --prefix=pwd/out
  4. make -j && make install
  5. Get into io.js checkout folder
  6. ./configure --openssl-fips=/path/to/openssl-fips/out
  7. Build io.js with make -j

Fix: nodejs/node-v0.x-archive#25463

@indutny
Copy link
Member Author

indutny commented Jun 3, 2015

R= @nodejs/crypto

@indutny
Copy link
Member Author

indutny commented Jun 3, 2015

Aye, made a mistake in instructions. Force pushed!

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Jun 3, 2015
@@ -720,6 +725,14 @@ def configure_openssl(o):
o['variables']['node_use_openssl'] = b(not options.without_ssl)
o['variables']['node_shared_openssl'] = b(options.shared_openssl)
o['variables']['openssl_no_asm'] = 1 if options.openssl_no_asm else 0
if options.openssl_fips:
o['variables']['openssl_fips'] = options.openssl_fips
o['make_global_settings'] = [
Copy link
Member

Choose a reason for hiding this comment

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

Using make_global_settings like that is a horrible and not very portable hack.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, what would be your suggestion to override the linker command?

@rvagg
Copy link
Member

rvagg commented Jun 3, 2015

@indutny I reckon those instructions should go in to the README rather than being lost in this issue, unless you can think of a better place for it?

@indutny
Copy link
Member Author

indutny commented Jun 4, 2015

Just figured out I could do it better and remove the need to patch fipsld. Will push an update soon.

@indutny
Copy link
Member Author

indutny commented Jun 4, 2015

@rvagg agree!

@shigeki
Copy link
Contributor

shigeki commented Jun 4, 2015

The sources of openssl-fips-2.0.9 are about 8.4M bytes.
If it is really required, is it better to include all sources into the repo and build them within gyp?

}
if make_global_settings != False:
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not very pythonic. Recommended way is if not make_global_settings:

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@jbergstroem
Copy link
Member

I'm not very familiar to fips, but if it ends up being built as a library we should support building against a shared .so as well.

@indutny
Copy link
Member Author

indutny commented Jun 4, 2015

@shigeki sorry, but there are some complications with this FIPS thing. I'm not sure if it can be included in our source tree. Unless someone knows for sure.

@shigeki
Copy link
Contributor

shigeki commented Jun 4, 2015

@indutny No, problem, you can take your choice.

@indutny indutny force-pushed the feature/fips-mode branch 4 times, most recently from 9338069 to c9802dc Compare June 4, 2015 20:35
@indutny
Copy link
Member Author

indutny commented Jun 4, 2015

Everything addressed, removed the need to patch OpenSSL FIPS checkout. PTAL (cc @bnoordhuis @rvagg @shigeki )

Support building and running with FIPS-compliant OpenSSL. The process is
following:

1. Download and verify `openssl-fips-x.x.x.tar.gz` from
   https://www.openssl.org/source/
2. Extract source to `openssl-fips` folder
3. `cd openssl-fips && ./config fipscanisterbuild --prefix=`pwd`/out`
4. `make -j && make install`
5. Get into io.js checkout folder
6. `./configure --openssl-fips=/path/to/openssl-fips/out`
7. Build io.js with `make -j`

Fix: nodejs/node-v0.x-archive#25463
if (!FIPS_mode_set(1)) {
int r;
r = ERR_get_error();
fprintf(stderr, "openssl fips failed: %s\n", ERR_error_string(r, NULL));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we directly do ERR_error_string(ERR_get_error(), NULL)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't fit into 80 columns. Anyway, merged two previous lines into one. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, okay. I would have done

fprintf(stderr, "openssl fips failed: %s\n",
    ERR_error_string(ERR_get_error(), NULL));

Can we use that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but what is the point? It is harder to read, and takes the same two lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

True, I was thinking along the lines of saving some memory. But, as Donald Knuth said, "Premature optimization is the root of all evil". So, this should be fine I guess :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't save anything, in both JS and C/C++. Compiler are smart enough to figure it out ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@indutny Oh, I didn't know that even JS Engine could do that. Thanks :-)

@@ -0,0 +1,19 @@
# Building io.js with FIPS-compliant OpenSSL
Copy link
Member

Choose a reason for hiding this comment

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

could we have a link to this from the README? Perhaps at the bottom of the build section:

See also: [Building io.js with FIPS-compliant OpenSSL](./deps/openssl/doc/FIPS.md)

@shigeki
Copy link
Contributor

shigeki commented Jun 5, 2015

There seems a fix needed to build on my Ubuntu because /bin/sh is not bash but dash.

diff --git a/deps/openssl/fips/fipsld b/deps/openssl/fips/fipsld
index 513982a..4c345cd 100755
--- a/deps/openssl/fips/fipsld
+++ b/deps/openssl/fips/fipsld
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/usr/bin/env bash

 # NOTE: Just a wrapper around normal fipsld
 FIPSLD=$1

And without CXX env, build was faild , so I set it manually.

I can't say whether this is really FIPS compliance or not because I've not read all of contents in OpenSSL handbook. I wonder if patched openssl can be allowed. Tests also show a lot of fips errors for their forbidden rules.

@indutny
Copy link
Member Author

indutny commented Jun 5, 2015

@shigeki I think this is quite unusual. Is there any reason to replace /bin/sh with different terminal, instead of using chsh?

@indutny
Copy link
Member Author

indutny commented Jun 5, 2015

@shigeki thanks for checking it out!

@indutny
Copy link
Member Author

indutny commented Jun 5, 2015

I'll read the FIPS guide to be sure, but here is an excerpt that I think should answer the question of compliance:

OpenSSL itself is not validated,and never will be. Instead a carefully defined software component
called the OpenSSL FIPS Object Module has been created. 

@indutny
Copy link
Member Author

indutny commented Jun 5, 2015

I think it should be pretty much correct to use FIPS module with patched OpenSSL:

 The new
product may reference the FIPS 140-1 or FIPS 140-2 validated cryptographic module so
long as the new product does not alter the original validated cryptographic module. 

and

 A
product which uses an embedded validated cryptographic module cannot claim itself to be
validated; only that it utilizes an embedded validated cryptographic module. There is no
assurance that a product is correctly utilizing an embedded validated cryptographic module
- this is outside the scope of the FIPS 140-1 or FIPS 140-2 validation.

IANAL, but OpenSSL was never FIPS compliant, and regardless of any patches it won't became one :)

@shigeki
Copy link
Contributor

shigeki commented Jun 6, 2015

In Ubuntu, /bin/sh is linked to dash by default. https://wiki.ubuntu.com/DashAsBinSh

I understand OpenSSL was never FIPS compliant. It needs so much requirements. Thanks for clarification.

@indutny
Copy link
Member Author

indutny commented Jun 6, 2015

Gosh. Please forgive my ignorance, @shigeki . I was completely unaware of dash.

Maybe we can fix the script to make it POSIX compliant, instead of forcing the bash? I'll look into it tomorrow.

@indutny
Copy link
Member Author

indutny commented Jun 9, 2015

@shigeki please take another look, it still requires CXX, but should work with dash.

5. Get into io.js checkout folder
6. `./configure --openssl-fips=/path/to/openssl-fips/out`
7. Build io.js with `make -j`

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better to add some descriptions to the doc as

  • supported platform: Windows is not supported. Only build on MacOS and Linux are confirmed.
  • CXX env is needed.
  • confirmation of result of fips build as
$ ./iojs -e "console.log(process.versions.openssl)"
1.0.2a-fips

@shigeki
Copy link
Contributor

shigeki commented Jun 10, 2015

Builds on my Ubuntu and MacOS are fine. Add some comments on the doc. LGTM.

@indutny
Copy link
Member Author

indutny commented Jun 10, 2015

@rvagg @shigeki all fixed, PTAL

@rvagg
Copy link
Member

rvagg commented Jun 10, 2015

docs lgtm

2. Extract source to `openssl-fips` folder
3. ``cd openssl-fips && ./config fipscanisterbuild --prefix=`pwd`/out``
(NOTE: On OS X, you may want to run
``./Configure darwin64-x86_64-cc --prefix=`pwd`/out`` if you are going to
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have OS X, so I couldn't confirm this. Is ./Configure with a capital C, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I have OS X. So I can confirm it ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, then :-) I just thought it was a typo.

@jbergstroem
Copy link
Member

Since no one else has mentioned shared library support I'll just have a look at what can be done after this lands.

@indutny
Copy link
Member Author

indutny commented Jun 10, 2015

You mean shared OpenSSL? This will certainly work, except that it won't have patches that we have applied to it. Namely:

@jbergstroem
Copy link
Member

@indutny great; I'll have a look at it later then (especially for the FIPS scenario).

@indutny
Copy link
Member Author

indutny commented Jun 10, 2015

Ok, cool. I tested it on OS X and Ubuntu, assuming that it is working and landing. Thanks everyone for the feedback!

@indutny
Copy link
Member Author

indutny commented Jun 10, 2015

Landed in 0f68377, thank you everyone!

indutny added a commit that referenced this pull request Jun 10, 2015
Support building and running with FIPS-compliant OpenSSL. The process is
following:

1. Download and verify `openssl-fips-x.x.x.tar.gz` from
   https://www.openssl.org/source/
2. Extract source to `openssl-fips` folder
3. ``cd openssl-fips && ./config fipscanisterbuild --prefix=`pwd`/out``
   (NOTE: On OS X, you may want to run
    ``./Configure darwin64-x86_64-cc --prefix=`pwd`/out`` if you are going to
    build x64-mode io.js)
4. `make -j && make install`
5. Get into io.js checkout folder
6. `./configure --openssl-fips=/path/to/openssl-fips/out`
7. Build io.js with `make -j`
8. Verify with `node -p "process.versions.openssl"` (`1.0.2a-fips`)

Fix: nodejs/node-v0.x-archive#25463
PR-URL: #1890
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
@indutny indutny closed this Jun 10, 2015
@indutny indutny deleted the feature/fips-mode branch June 10, 2015 23:51
@rvagg rvagg mentioned this pull request Jun 11, 2015
rvagg added a commit that referenced this pull request Jun 13, 2015
Notable Changes:

* libuv: Upgraded to 1.6.0 and 1.6.1, see full ChangeLog for details.
  (Saúl Ibarra Corretgé) #1905 #1889. Highlights include:
  - Fix TTY becoming blocked on OS X
  - Fix UDP send callbacks to not to be synchronous
  - Add uv_os_homedir() (exposed as os.homedir(), see below)
* npm: See full release notes for details. (Kat Marchán) #1899. Highlight:
  - Use GIT_SSH_COMMAND (available as of Git 2.3)
* openssl:
  - Upgrade to 1.0.2b and 1.0.2c, introduces DHE man-in-the-middle protection
    (Logjam) and fixes malformed ECParameters causing infinite loop
    (CVE-2015-1788). See the security advisory for full details.
    (Shigeki Ohtsu) #1950 #1958
  - Support FIPS mode of OpenSSL, see README for instructions.
    (Fedor Indutny) #1890
* os: Add os.homedir() method. (Colin Ihrig) #1791
* smalloc: Deprecate whole module. (Vladimir Kurchatkin) #1822
* Add new collaborators:
  - Alex Kocharin (@rlidwka)
  - Christopher Monsanto (@monsanto)
  - Ali Ijaz Sheikh (@ofrobots)
  - Oleg Elifantiev (@Olegas)
  - Domenic Denicola (@domenic)
  - Rich Trott (@Trott)
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 this pull request may close these issues.

Enabling FIPS-compliant encryption
7 participants