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

Drop #ifdef NODE_FIPS_MODE wherever possible #34903

Closed
voxik opened this issue Aug 24, 2020 · 11 comments
Closed

Drop #ifdef NODE_FIPS_MODE wherever possible #34903

voxik opened this issue Aug 24, 2020 · 11 comments
Labels
build Issues and PRs related to build files or the CI. crypto Issues and PRs related to the crypto subsystem.

Comments

@voxik
Copy link
Contributor

voxik commented Aug 24, 2020

Drop #ifdef NODE_FIPS_MODE wherever possible, because it does not make sense to guard FIPS code by #ifdef NODE_FIPS_MODE, when this ifdef is immediately followed by FIPS_mode(). It would make sense if the FIPS_mode() is not defined depending on OpenSSL settings, but that is not the case. I believe that removing the guards would help our users to get more precise information about FIPS settings from calls such as node -p 'crypto.getFips()'. This would also help to resolve this ticket.

@voxik
Copy link
Contributor Author

voxik commented Aug 24, 2020

@mhdawson @danbev

@voxik
Copy link
Contributor Author

voxik commented Aug 24, 2020

And since I am here, it would be also nice to revisit this code and get the information from FIPS_mode() instead of some command line option.

@voxik
Copy link
Contributor Author

voxik commented Aug 24, 2020

IOW, it would be nice if Node.js always relied solely on OpenSSL settings, no matter what build options or command line options are specified. The configuration options should only influence OpenSSL settings, not the Node.js code.

@bnoordhuis bnoordhuis added build Issues and PRs related to build files or the CI. crypto Issues and PRs related to the crypto subsystem. labels Aug 24, 2020
@bnoordhuis
Copy link
Member

Pull request welcome, I think.

voxik added a commit to voxik/node that referenced this issue Aug 25, 2020
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes nodejs#34903
@mhdawson
Copy link
Member

That seems reasonable to me and what you have in the branch which mentions this looks good so far.

@voxik
Copy link
Contributor Author

voxik commented Aug 26, 2020

Thx. I need to do some testing. I'll send PR once it is ready.

@mhdawson
Copy link
Member

@voxik thanks for the update.

@khardix
Copy link

khardix commented Sep 2, 2020

I did some testing around this on FIPS-enabled system. I built node with varying options and then ran ./node [--enable-fips] -p 'crypto.getFips()'.

  1. Node built with --shared-openssl --openssl-is-fips configure options, without this patch applied:

    • ./node --enable-fips -p 'crypto.getFips()' prints 1 ⇒ FIPS mode enabled.
    • ./node -p 'crypto.getFips()' prints 0 ⇒ FIPS mode is not enabled.
  2. Node built with --shared-openssl and with this patch applied:

    Same results as in point 1. Note that the configure option --openssl-is-fips seems to have no effect here, which is the whole point of this patch 😄.

  3. Node built with --shared-openssl without this patch:

    • ./node --enable-fips -p 'crypto.getFips()' reports bad option: --enable-fips.
    • ./node -p 'crypto.getFips()' prints 0 ⇒ FIPS mode not enabled.

    In this configuration, FIPS mode cannot be enabled at all.

  4. Node built with --openssl-is-fips or this patch applied (same results), but without --shared-openssl (and thus with the bundled one):

    • ./node --enable-fips -p 'crypto.getFips()' throws an error:

      openssl fips failed: error:0F06D065:common libcrypto routines:FIPS_mode_set:fips mode not supported
      ./node[1627]: ../src/node_crypto.cc:7028:void node::crypto::InitCryptoOnce(): Assertion `"Unreachable code reached"' failed.
      1: 0xad7950 node::Abort() [./node]
      2: 0xad79e2 node::Assert(node::AssertionInfo const&) [./node]
      3: 0xc02347 node::crypto::InitCryptoOnce() [./node]
      4: 0x7fe39fc9e20b  [/lib64/libpthread.so.0]
      5: 0x1458469 uv_once [./node]
      6: 0xc0b836 node::crypto::Initialize(v8::Local<v8::Object>, v8::Local<v8::Value>, v8::Local<v8::Context>, void*) [./node]
      7: 0xaaa73b node::binding::GetInternalBinding(v8::FunctionCallbackInfo<v8::Value> const&) [./node]
      8: 0xccd7a2 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [./node]
      9: 0xccdbf3  [./node]
      10: 0xcce496  [./node]
      11: 0xcce726 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [./node]
      12: 0x14cf119  [./node]
      
    • ./node -p crypto.getFips() prints 0 ⇒ FIPS mode not enabled.

    As expected, here the FIPS mode is reported as unsupported by the bundled OpenSSL version,
    although it is quite aggresive about it – the assertion seems quite strict.

  5. I did not really test the "vanilla" Node build, but from the above combinations
    I assume that the --enable-fips option would not exist and without it,
    the crypto.getFips() would still return 0, making this case equivalent to point 3.


@voxik Hope this helps, let me know if you need any other combination :)

@voxik
Copy link
Contributor Author

voxik commented Sep 2, 2020

@khardix Thx for testing. So if I understand correctly, you have tested on the system where FIPS was supported, but disabled by default. Do you think you could provide also test results on the system with OpenSSL configured in FIPS mode by default? I think that specifying fips=1 on kernel cmd line should be enough to enable the FIPS mode.

And I am going to take a look into the 4th bullet.

@voxik
Copy link
Contributor Author

voxik commented Sep 2, 2020

And I am going to take a look into the 4th bullet.

So I think the problem lies at this part of InitCryptoOnce():

  /* Override FIPS settings in cnf file, if needed. */
  unsigned long err = 0;  // NOLINT(runtime/int)
  if (per_process::cli_options->enable_fips_crypto ||
      per_process::cli_options->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, nullptr));
    UNREACHABLE();
  }

where the UNREACHABLE(); aborts the Node.js and actually the abortion is probably correct, because when OpenSSL does not support FIPS, it can't be enabled. But it should abort more gracefully. Not sure how to do that. May be something like return ThrowCryptoError(env, err);

@voxik
Copy link
Contributor Author

voxik commented Sep 2, 2020

I have opened PR #35019

khardix pushed a commit to khardix/node that referenced this issue Dec 15, 2020
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes nodejs#34903
khardix pushed a commit to khardix/node that referenced this issue Feb 9, 2021
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes nodejs#34903
targos pushed a commit that referenced this issue Feb 28, 2021
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes #34903

PR-URL: #36341
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
danbev pushed a commit to danbev/node that referenced this issue Sep 28, 2021
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes nodejs#34903

PR-URL: nodejs#36341
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
danbev pushed a commit to danbev/node that referenced this issue Sep 28, 2021
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes nodejs#34903

Backport-PR-URL: nodejs#40241
PR-URL: nodejs#36341
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
richardlau pushed a commit that referenced this issue Nov 25, 2021
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes: #34903
Backport-PR-URL: #40241
PR-URL: #36341
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
richardlau pushed a commit that referenced this issue Nov 30, 2021
There is no reason to hide FIPS functionality behind build flags.
OpenSSL always provide the information about FIPS availability via
`FIPS_mode()` function.

This makes the user experience more consistent, because the OpenSSL
library is always queried and the `crypto.getFips()` always returns
OpenSSL settings.

Fixes: #34903
Backport-PR-URL: #40241
PR-URL: #36341
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
4 participants