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.

Sign up for GitHub

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

Jump to bottom

src: add proper mutexes for accessing FIPS state #42278

Merged
merged 2 commits into from
Apr 3, 2022
Merged

src: add proper mutexes for accessing FIPS state #42278

merged 2 commits into from
Apr 3, 2022

Conversation

addaleax
Copy link
Member

The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

@addaleax
src: add proper mutexes for accessing FIPS state
095c3e3 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 10, 2022
addaleax
addaleax commented Mar 10, 2022
View reviewed changes
src/crypto/crypto_util.cc Outdated
Comment on lines 222 to 223
// TODO: This should not be possible to set from worker threads.
// CHECK(env->owns_process_state());
Copy link
Member Author

Choose a reason for hiding this comment

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

Left out because this could be seen as a semver-major change. If we don’t think it’s semver-major, I’m happy to add this as well.

Copy link
Member

Choose a reason for hiding this comment

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

Is it worth doing this for Node.js 18 as a semver-major anyway?

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, sure. It’s just not quite as important and I didn’t want to make this PR one that wouldn’t be backported.

@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Mar 10, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 10, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
tniessen
tniessen reviewed Mar 10, 2022
View reviewed changes
src/crypto/crypto_util.cc
@@ -136,7 +136,13 @@ bool InitCryptoOnce(Isolate* isolate) {
return true;
}

// Protect accesses to FIPS state with a mutex. This should potentially
// be part of a larger mutex for global OpenSSL state.
static Mutex fips_mutex;
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense for semantic reasons, but is my understanding correct that this is technically not required since per_process::cli_options_mutex will already guarantee mutually exclusive access whenever fips_mutex is used?

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, that is correct as far as the current state of this PR is concerned. 👍

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for clarifying! 😃

tniessen
tniessen approved these changes Mar 10, 2022
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

LGTM with or without fips_mutex.

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen tniessen added the openssl Issues and PRs related to the OpenSSL dependency. label Mar 10, 2022
@github-actions github-actions bot mentioned this pull request Mar 11, 2022
Open
16 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Mar 12, 2022
Open
15 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
Copy link
Member

I have no idea what's going on with arm CI...

@github-actions github-actions bot mentioned this pull request Mar 13, 2022
Open
17 tasks
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Mar 14, 2022
Open
28 tasks
@github-actions github-actions bot mentioned this pull request Apr 3, 2022
Open
8 tasks
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2022
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/43273/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/43277/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/43285/

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit 1c69dfe into nodejs:master Apr 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 1c69dfe

@github-actions github-actions bot mentioned this pull request Apr 4, 2022
Open
9 tasks
@addaleax addaleax deleted the fips-mutexes branch April 4, 2022 09:18
@richardlau
Copy link
Member

src/crypto/crypto_util.cc was created as part of a semver-major refactor #35093 so this will need a manual backport to v14.x-staging if it is required there. It would also need a backport for 12.x but since we're not planning any further 12.x releases after tomorrow's release it's probably a bit late to land there.

@github-actions github-actions bot mentioned this pull request Apr 5, 2022
Open
13 tasks
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
@addaleax @juanarbol
src: add proper mutexes for accessing FIPS state
3084af3 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: nodejs#42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
This was referenced Apr 5, 2022
Closed
Merged
@github-actions github-actions bot mentioned this pull request Apr 6, 2022
Open
11 tasks
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
@addaleax @juanarbol
src: add proper mutexes for accessing FIPS state
e798e26 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@github-actions github-actions bot mentioned this pull request Apr 7, 2022
Open
17 tasks
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
@addaleax @xtx1130
src: add proper mutexes for accessing FIPS state
b51a7f7 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: nodejs#42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
@addaleax @juanarbol
src: add proper mutexes for accessing FIPS state
b3016c7 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@juanarbol juanarbol mentioned this pull request May 31, 2022
Closed
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
@addaleax @danielleadams
src: add proper mutexes for accessing FIPS state
456c186 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
@addaleax @targos
src: add proper mutexes for accessing FIPS state
4ffc088 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: #42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@targos targos mentioned this pull request Aug 3, 2022
Merged
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
@addaleax @guangwong
src: add proper mutexes for accessing FIPS state
758ab3f 
The FIPS state handling and OpenSSL initialization code makes
accesses to global OpenSSL state without any protection against
parallel modifications from multiple threads.

This commit adds such protections.

PR-URL: nodejs/node#42278
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@tniessen tniessen tniessen approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@addaleax @nodejs-github-bot @tniessen @richardlau @jasnell @RaisinTen @targos