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

deps: define OPENSSLDIR and ENGINESDIR explicitly #29455

Closed
wants to merge 1 commit into from
Closed

deps: define OPENSSLDIR and ENGINESDIR explicitly #29455

wants to merge 1 commit into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Sep 5, 2019 •
edited by sam-github
Loading

According to CVE-2019-1552(*), it is encouraged to change OPENSSLDIR from the default of /usr/local/ssl to a privileged directory on Windows. "C:\Program Files\Common Files\SSL" is set as it is the default path in OpenSSL-1.1.1.

This is also described in openssl/openssl@d333eba for the forthcoming release of OpenSSL-1.0.2t.

It breaks the compatibility of the OPENSSLDIR path with the previous v8 LTS releases. For v8 LTS will be ended after 4 months and its severity is LOW, I do not mind if this is not fixed.

(*) https://www.openssl.org/news/secadv/20190730.txt

Fixes: #29445

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@shigeki
deps: define OPENSSLDIR and ENGINESDIR explicitly
d768b62 
According to CVE-2019-1552(*), it is encouraged to change OPENSSLDIR
from the default of /usr/local/ssl to a privileged directory on
Windows. "C:\Program Files\Common Files\SSL" is set as it is the
default path in OpenSSL-1.1.1.

(*) https://www.openssl.org/news/secadv/20190730.txt

Fixes: nodejs#29445
@shigeki shigeki added openssl Issues and PRs related to the OpenSSL dependency. land-on-v8.x labels Sep 5, 2019
This was referenced Sep 5, 2019
Closed
Closed
@sam-github
Copy link
Contributor

@nodejs/tsc opinions? @nodejs/lts ?

I'm OK with following @shigeki's advice: don't fix, its low priority and on an almost EOL release line.

@nodejs/platform-windows , you should take careful note -- this is specific to Windows, do you have an opinion?

@sam-github sam-github added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 5, 2019
@mhdawson mhdawson mentioned this pull request Sep 9, 2019
Closed
@mhdawson
Copy link
Member

mhdawson commented Sep 9, 2019

I'm OK with following @shigeki's advice as well.

Once concern is what happens if you upgrade from an earlier version and had config files in /usr/local/ssl. Would you run without the configuration you expected and not know it?

Maybe we should look to see if there is config in /usr/local/ssl and warn that it's not going to be used in the current version?

@mhdawson
Copy link
Member

mhdawson commented Sep 9, 2019

I'm OK with following @shigeki's advice: don't fix, its low priority and on an almost EOL release line.

@shigeki
Copy link
Contributor Author

shigeki commented Sep 11, 2019

Once concern is what happens if you upgrade from an earlier version and had config files in /usr/local/ssl. Would you run without the configuration you expected and not know it?

It depends. openssl.cnf can change various default values such as TLS versions and ciphers. Some changes might be known to a user but some might not.

Maybe we should look to see if there is config in /usr/local/ssl and warn that it's not going to be used in the current version?

/usr/local/ssl has no problems if it is protected by the privileged user. Node-v10 and later had a bug of the default path setting on Windows. No one would use it.

@Trott Trott removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 11, 2019
@BethGriggs
Copy link
Member

It looks like we're agreeing to not land this change in v8.x?
Just as an FYI, the final scheduled release of v8.x is due to go out on 8th Oct (#29617).

@shigeki
Copy link
Contributor Author

shigeki commented Sep 27, 2019

Close this for we agreed not to land this.

@shigeki shigeki closed this Sep 27, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 6, 2019
@shigeki @Trott
deps: fix OPENSSLDIR on Windows
eea5bb4 
Backslashes and spaces are need to be escaped to define OPENSSLDIR to
"C:\Program Files\Common Files\SSL".

PR-URL: nodejs#29456
Refs: nodejs#29455
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Dec 9, 2019
@shigeki @targos
deps: fix OPENSSLDIR on Windows
4588606 
Backslashes and spaces are need to be escaped to define OPENSSLDIR to
"C:\Program Files\Common Files\SSL".

PR-URL: #29456
Refs: #29455
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jan 13, 2020
@shigeki @targos
deps: fix OPENSSLDIR on Windows
b27b420 
Backslashes and spaces are need to be escaped to define OPENSSLDIR to
"C:\Program Files\Common Files\SSL".

PR-URL: #29456
Refs: #29455
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@shigeki @BethGriggs
deps: fix OPENSSLDIR on Windows
ff82ccb 
Backslashes and spaces are need to be escaped to define OPENSSLDIR to
"C:\Program Files\Common Files\SSL".

PR-URL: #29456
Refs: #29455
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.
6 participants
@shigeki @sam-github @mhdawson @BethGriggs @Trott @nodejs-github-bot