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

src: set CONF_MFLAGS_DEFAULT_SECTION for OpenSSL 3 #38732

Closed
wants to merge 3 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented May 19, 2021

This commit adds a call to OPENSSL_init_crypto to initialize
OPENSSL_INIT_LOAD_CONFIG to avoid the default behavior where errors
raised during the parsing of the OpenSSL configuration file are not
propagated and cannot be detected.

The motivation for this is that if FIPS is configured the OpenSSL
configuration file will have an .include pointing to the fipsmodule.cnf
file generated by the openssl fipsinstall command. If the path to this
file is incorrect no error will be reported. For Node.js this will mean
that EntropySource will be called by V8 as part of its initalization
process, and EntropySource will in turn call CheckEntropy. CheckEntropy
will call RAND_status which will now always return 0 leading to an
endless loop and the node process will appear to hang/freeze.

I'll continue investigating the cause of this and see if this is
expected behavior or not, but in the mean time it would be good to be
able to workaround this issue with this commit.

@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 19, 2021
@waelsy123
Copy link
Contributor

@danbev is there any opened issue to link this pull request?

@danbev
Copy link
Contributor Author

danbev commented May 19, 2021

@danbev is there any opened issue to link this pull request?

The is no issue for this at the moment, but I can create one if needed. This issue discovered in #38633 (review) which I should have added as a Refs in the commit message. I'll to that.

@nodejs-github-bot
Copy link
Collaborator

This commit adds a call to OPENSSL_init_crypto to initialize
OPENSSL_INIT_LOAD_CONFIG to avoid the default behavior where errors
raised during the parsing of the OpenSSL configuration file are not
propagated and cannot be detected.

The motivation for this is that if FIPS is configured the OpenSSL
configuration file will have an .include pointing to the fipsmodule.cnf
file generated by the openssl fipsinstall command. If the path to this
file is incorrect no error will be reported. For Node.js this will mean
that EntropySource will be called by V8 as part of its initalization
process, and EntropySource will in turn call CheckEntropy. CheckEntropy
will call RAND_status which will now always return 0 leading to an
endless loop and the node process will appear to hang/freeze.

I'll continue investigating the cause of this and see if this is
expected behavior or not, but in the mean time it would be good to be
able to workaround this issue with this commit.

Refs: nodejs#38633 (review)
@danbev danbev force-pushed the openssl-config-include-error branch from ddd7847 to 86fa07c Compare May 19, 2021 13:46
Remove setting of config_filename when OPENSSL_CONF environment variable
is specified (only set it for --openssl-config Node.js option.
Only skip the settting of the config_filename for OpenSSL 3 and not the
OPENSSL_init_ssl.
@nodejs-github-bot
Copy link
Collaborator

@danbev
Copy link
Contributor Author

danbev commented May 24, 2021

Landed in e9e851f.

@danbev danbev closed this May 24, 2021
danbev added a commit that referenced this pull request May 24, 2021
This commit adds a call to OPENSSL_init_crypto to initialize
OPENSSL_INIT_LOAD_CONFIG to avoid the default behavior where errors
raised during the parsing of the OpenSSL configuration file are not
propagated and cannot be detected.

The motivation for this is that if FIPS is configured the OpenSSL
configuration file will have an .include pointing to the fipsmodule.cnf
file generated by the openssl fipsinstall command. If the path to this
file is incorrect no error will be reported. For Node.js this will mean
that EntropySource will be called by V8 as part of its initalization
process, and EntropySource will in turn call CheckEntropy. CheckEntropy
will call RAND_status which will now always return 0 leading to an
endless loop and the node process will appear to hang/freeze.

I'll continue investigating the cause of this and see if this is
expected behavior or not, but in the mean time it would be good to be
able to workaround this issue with this commit.

PR-URL: #38732
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Refs: #38633 (review)
@danbev danbev deleted the openssl-config-include-error branch May 24, 2021 06:21
danielleadams pushed a commit that referenced this pull request May 31, 2021
This commit adds a call to OPENSSL_init_crypto to initialize
OPENSSL_INIT_LOAD_CONFIG to avoid the default behavior where errors
raised during the parsing of the OpenSSL configuration file are not
propagated and cannot be detected.

The motivation for this is that if FIPS is configured the OpenSSL
configuration file will have an .include pointing to the fipsmodule.cnf
file generated by the openssl fipsinstall command. If the path to this
file is incorrect no error will be reported. For Node.js this will mean
that EntropySource will be called by V8 as part of its initalization
process, and EntropySource will in turn call CheckEntropy. CheckEntropy
will call RAND_status which will now always return 0 leading to an
endless loop and the node process will appear to hang/freeze.

I'll continue investigating the cause of this and see if this is
expected behavior or not, but in the mean time it would be good to be
able to workaround this issue with this commit.

PR-URL: #38732
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Refs: #38633 (review)
@danielleadams danielleadams mentioned this pull request May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants