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
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@
'OBJ_DIR': '<(PRODUCT_DIR)/obj.target',
'V8_BASE': '<(PRODUCT_DIR)/obj.target/deps/v8/tools/gyp/libv8_base.a',
}],
['openssl_fips != ""', {
'OPENSSL_PRODUCT': 'libcrypto.a',
}, {
'OPENSSL_PRODUCT': 'libopenssl.a',
}],
['OS=="mac"', {
'clang%': 1,
}, {
Expand Down
28 changes: 27 additions & 1 deletion configure
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,11 @@ parser.add_option("--openssl-no-asm",
dest="openssl_no_asm",
help="Do not build optimized assembly for OpenSSL")

parser.add_option("--openssl-fips",
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor. Everywhere else in the PR, ' is used and only here " is used. Is there any convention followed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied and changed previous line :)

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, even that is not consistent with the previous add_option calls. This shouldn't matter anyway.

action="store",
dest="openssl_fips",
help="Build OpenSSL using FIPS canister .o file in supplied folder")

shared_optgroup.add_option('--shared-http-parser',
action='store_true',
dest='shared_http_parser',
Expand Down Expand Up @@ -720,6 +725,16 @@ 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
fips_dir = os.path.join(root_dir, 'deps', 'openssl', 'fips')
fips_ld = os.path.abspath(os.path.join(fips_dir, 'fipsld'))
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?

['LINK', fips_ld + ' <(openssl_fips)/bin/fipsld'],
]
else:
o['variables']['openssl_fips'] = ''


if options.without_ssl:
return
Expand Down Expand Up @@ -1025,10 +1040,21 @@ configure_fullystatic(output)
# move everything else to target_defaults
variables = output['variables']
del output['variables']

# make_global_settings should be a root level element too
if 'make_global_settings' in output:
make_global_settings = output['make_global_settings']
del output['make_global_settings']
else:
make_global_settings = False

output = {
'variables': variables,
'target_defaults': output
'target_defaults': output,
}
if make_global_settings:
output['make_global_settings'] = make_global_settings

pprint.pprint(output, indent=2)

write('config.gypi', do_not_edit +
Expand Down
19 changes: 19 additions & 0 deletions deps/openssl/doc/FIPS.md
Original file line number Diff line number Diff line change
@@ -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)


It is possible to build io.js with [OpenSSL FIPS module][0].

Instructions:

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`

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

[0]: https://www.openssl.org/docs/fips/fipsnotes.html
14 changes: 14 additions & 0 deletions deps/openssl/fips/fipscc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/sh
ARGS=

while [ "x$1" != "x" ]; do
ARG=$1
shift
case $ARG in
*fips_premain.c) ARGS="$ARGS -x c $ARG -x none";;
*) ARGS="$ARGS $ARG";;
esac
done

echo $CXX $ARGS
${CXX} $ARGS
8 changes: 8 additions & 0 deletions deps/openssl/fips/fipsld
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#!/bin/sh

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

DIR=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
FIPSLD_CC=$DIR/fipscc $FIPSLD $@
23 changes: 23 additions & 0 deletions deps/openssl/openssl.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
'openssl_no_asm%': 0,
'llvm_version%': 0,
'gas_version%': 0,
'openssl_fips%': 'false',
},
'targets': [
{
Expand All @@ -21,6 +22,28 @@
['exclude', 'store/.*$']
],
'conditions': [
# FIPS
['openssl_fips != ""', {
'defines': [
'OPENSSL_FIPS',
],
'include_dirs': [
'<(openssl_fips)/include',
],

# Trick fipsld, it expects to see libcrypto.a
'product_name': 'crypto',

'direct_dependent_settings': {
'defines': [
'OPENSSL_FIPS',
],
'include_dirs': [
'<(openssl_fips)/include',
],
},
}],

['openssl_no_asm!=0', {
# Disable asm
'defines': [
Expand Down
4 changes: 2 additions & 2 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -233,13 +233,13 @@
[ 'node_target_type!="static_library"', {
'xcode_settings': {
'OTHER_LDFLAGS': [
'-Wl,-force_load,<(PRODUCT_DIR)/libopenssl.a',
'-Wl,-force_load,<(PRODUCT_DIR)/<(OPENSSL_PRODUCT)',
],
},
'conditions': [
['OS in "linux freebsd"', {
'ldflags': [
'-Wl,--whole-archive <(PRODUCT_DIR)/libopenssl.a',
'-Wl,--whole-archive <(PRODUCT_DIR)/<(OPENSSL_PRODUCT)',
'-Wl,--no-whole-archive',
],
}],
Expand Down
9 changes: 9 additions & 0 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5071,6 +5071,15 @@ void InitCryptoOnce() {
CRYPTO_set_locking_callback(crypto_lock_cb);
CRYPTO_THREADID_set_callback(crypto_threadid_cb);

#ifdef OPENSSL_FIPS
if (!FIPS_mode_set(1)) {
int err = ERR_get_error();
fprintf(stderr, "openssl fips failed: %s\n", ERR_error_string(err, NULL));
UNREACHABLE();
}
#endif // OPENSSL_FIPS


// Turn off compression. Saves memory and protects against CRIME attacks.
#if !defined(OPENSSL_NO_COMP)
#if OPENSSL_VERSION_NUMBER < 0x00908000L
Expand Down