Update openssl1.1.1d #29550

sam-github · 2019-09-13T20:20:14Z

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

sam-github · 2019-09-13T20:37:15Z

@mscdex @bnoordhuis Note last commit, all the checks for DH_NOT_SUITABLE_GENERATOR in test/parallel/test-crypto-dh.js fail with openssl 1.1.1d, because of the removal in openssl/openssl#9363 of https://github.com/openssl/openssl/blob/2500c093aa1e9c90c11c415053c0a27a00661d0d/crypto/dh/dh_check.c#L143-L154

https://github.com/bernd-edlinger/openssl/blob/9087476b536dd13fe99fdd36773234923cea78bb/CHANGES#L12-L16 describes this a security improvement, to avoid leaking data, as well as

The main motivation for this was that RFC 7919 parameters use an order q subgroup as well,
but those did not pass the DH_check sanity test, however those parameters are perfectly sane.

Change DH parameters to generate the order q subgroup instead of 2q openssl/openssl#9363 (comment)

In Node.js, the tests have been refactored multiple times, but the original code was introduced by @mscdex in nodejs/node-v0.x-archive#7086 in response to a feature request, nodejs/node-v0.x-archive#2338

@bnoordhuis and others had concerns that simply allowing these params was risky, thus the exposure of the verification codes for (elective) sanity checks. Those checks don't appear possible anymore.

I find literally zero use of this error code, or even the property, in the rest of the Node.js code base, so we have no internal dependency on it. I don't pretend to knowing enough group theory to understand what's at issue here.

Should I simply remove the tests, as in the last WIP commit, or should I report this upstream to OpenSSL, or something else?

/to @nodejs/crypto

nodejs-github-bot · 2019-09-13T20:42:40Z

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

mscdex · 2019-09-13T22:43:17Z

in response to a feature request, nodejs/node-v0.x-archive#2338

My PR was a response to my own feature request here. My reference to that other issue was in discussion about handling of the 'error' or OpenSSL errors in general.

mscdex · 2019-09-13T22:52:31Z

I don't personally use verifyError, but as long as the accepting of custom generators is still allowed like before, I'm happy.

sam-github · 2019-09-14T01:37:45Z

@mscdex thanks for the references.

Are you OK with the changes in 9607205, to just ignore the code since it never is set?

mscdex · 2019-09-14T03:09:57Z

I don't know enough about it to be sure, it could be that the generator values in the tests have to be updated in order to trigger DH_NOT_SUITABLE_GENERATOR?

bnoordhuis · 2019-09-15T07:36:56Z

This avoids leaking bit 0 of the private key.

I'm not sure I understand openssl/openssl#9363: bit 0 is always 1 because it's a prime > 2, isn't it?

Apropos the DH_NOT_SUITABLE_GENERATOR tests, we can either:

remove them
use different values (i.e., g=2 and g=5 don't work anymore but g=1 or g>p still should)
do the modulo checks ourselves (in C++ or in BigInt JS land), at least for now

This PR is supposed to eventually end up in the LTS branch, right? That would make (3) the most attractive for BC reasons but I'm willing to be convinced I'm wrong. :-)

cc @tniessen - perhaps you have an opinion?

sam-github · 2019-09-16T17:36:39Z

I polled OpenSSL for a comment: openssl/openssl#9363 (comment)

I'm averse to indefinitely floating changes on OpenSSL that reverse their API decisions without a very, very compelling reason (which would beg the question of why the change isn't pushed back to OpenSSL).

So, there is a 4th/5th option:

4.:

use different generators in master so code paths are tested
use a compatibility patch on 10.x and 12.x so they have no change in behaviour, but the patch will EOL when the respective LTS release lines EOL

Do 4, minus the patch, and only add the patch if someone actually notices and cares.

My impression from reading the code and discussion is that the feature allows uses of DH generators in a way that that the code isn't meaningful to, and that for the unsafe params the API isn't used, so doesn't help much. In other words -- anyone using custom DH params probably knows what they are doing already.

kroeckx · 2019-09-16T21:01:54Z

I would argue that this is not an API change, but a bug fix in OpenSSL. It used to return that it's not a suitable generator while it was.

As I understand this, the feature request was to make it possible to load keys that OpenSSL used to reject, but now accepts.

nodejs-github-bot · 2019-09-17T20:53:47Z

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

nodejs-github-bot · 2019-09-17T23:27:49Z

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

bnoordhuis

LGTM but note that this breaks the ubuntu1604_sharedlibs_openssl111_x64 buildbot for obvious reasons.

I don't have good suggestions on what to do here. 16.04 ships openssl 1.02 so that means the shared library build is one we provide ourselves? Can we upgrade it when this PR is merged?

bnoordhuis · 2019-09-18T18:47:38Z

test/parallel/test-crypto-dh.js


+// Confirm DH_check() results are exposed for optional examination.
+const bad_dh = crypto.createDiffieHellman('02', 'hex');
+assert.strictEqual(bad_dh.verifyError, DH_CHECK_P_NOT_PRIME |


Millions of mathematicians weep tonight....

richardlau · 2019-09-18T19:46:51Z

LGTM but note that this breaks the ubuntu1604_sharedlibs_openssl111_x64 buildbot for obvious reasons.

I don't have good suggestions on what to do here. 16.04 ships openssl 1.02 so that means the shared library build is one we provide ourselves? Can we upgrade it when this PR is merged?

The sharedlibs builds run in a docker container defined in https://github.com/nodejs/build/blob/master/ansible/roles/docker/templates/ubuntu1604_sharedlibs.Dockerfile.j2. Currently we're using 1.1.1b (for openssl 1.1.1). If we update it to 1.1.1d will things break before this PR lands on all release lines that currently run in that container?

sam-github · 2019-09-18T22:18:17Z

Yes, I noticed the breakage. I'm not exactly sure what to do, its kindof tough when openssl changes behaviour mid-release line. My temptation would be to rewrite the tests so they pass against openssl 1.1.1c and before, as well as openssl 1.1.1d and later. The docker sharedlib test failure is valid, IMO, we should either fix it, or remove that test from CI if we don't care.

bnoordhuis · 2019-09-19T07:14:19Z

Version-sniffing process.versions.openssl is an option. Another is to take the buildbot offline until all release lines have upgraded.

(I'm not saying that's the best option, mind you, but it's an option.)

This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.0h.tar.gz $ mv openssl-1.1.0h openssl $ git add --all openssl $ git commit openssl

After an OpenSSL source update, all the config files need to be regenerated and comitted by: $ cd deps/openssl/config $ make $ git add deps/openssl/config/archs $ git add deps/openssl/openssl/crypto/include/internal/bn_conf.h $ git add deps/openssl/openssl/crypto/include/internal/dso_conf.h $ git add deps/openssl/openssl/include/openssl/opensslconf.h $ git commit

OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment)

nodejs-github-bot · 2019-09-19T20:11:55Z

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

nodejs-github-bot · 2019-09-20T19:57:50Z

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

nodejs-github-bot · 2019-09-23T17:59:26Z

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

sam-github · 2019-09-23T20:51:20Z

@nodejs/crypto needs another reviewer

sam-github · 2019-10-01T17:47:18Z

@nodejs/crypto -- needs one more review, please

sam-github · 2019-10-01T18:03:56Z

cc: @nodejs/tsc

sam-github · 2019-10-01T18:11:51Z

Thanks @addaleax

Landed in 7ce316e...3473e58

This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.0h.tar.gz $ mv openssl-1.1.0h openssl $ git add --all openssl $ git commit openssl PR-URL: #29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

After an OpenSSL source update, all the config files need to be regenerated and comitted by: $ cd deps/openssl/config $ make $ git add deps/openssl/config/archs $ git add deps/openssl/openssl/crypto/include/internal/bn_conf.h $ git add deps/openssl/openssl/crypto/include/internal/dso_conf.h $ git add deps/openssl/openssl/include/openssl/opensslconf.h $ git commit PR-URL: #29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: #29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.0h.tar.gz $ mv openssl-1.1.0h openssl $ git add --all openssl $ git commit openssl PR-URL: #29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

After an OpenSSL source update, all the config files need to be regenerated and comitted by: $ cd deps/openssl/config $ make $ git add deps/openssl/config/archs $ git add deps/openssl/openssl/crypto/include/internal/bn_conf.h $ git add deps/openssl/openssl/crypto/include/internal/dso_conf.h $ git add deps/openssl/openssl/include/openssl/opensslconf.h $ git commit PR-URL: #29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: #29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: nodejs#29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment) PR-URL: #29550 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added the openssl Issues and PRs related to the OpenSSL dependency. label Sep 13, 2019

sam-github force-pushed the update-openssl1.1.1d branch from 29792ed to 01fd06d Compare September 13, 2019 20:41

sam-github force-pushed the update-openssl1.1.1d branch from 01fd06d to 9607205 Compare September 13, 2019 20:57

sam-github requested review from bnoordhuis, shigeki and mscdex September 14, 2019 01:37

sam-github mentioned this pull request Sep 16, 2019

Change DH parameters to generate the order q subgroup instead of 2q openssl/openssl#9363

Closed

2 tasks

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:50

sam-github marked this pull request as ready for review September 17, 2019 20:52

sam-github force-pushed the update-openssl1.1.1d branch from 9607205 to 7dc56e0 Compare September 17, 2019 20:53

bnoordhuis approved these changes Sep 18, 2019

View reviewed changes

sam-github added 3 commits September 19, 2019 12:50

deps: upgrade openssl sources to 1.1.1d

6007d28

This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.0h.tar.gz $ mv openssl-1.1.0h openssl $ git add --all openssl $ git commit openssl

test: well-defined DH groups now verify clean

13f3159

OpenSSL 1.1.1d no longer generates warnings for some DH groups that used to be considered unsafe. See below for discussion. This is considered a bug fix. See: - openssl/openssl#9363 - openssl/openssl#9363 (comment)

sam-github force-pushed the update-openssl1.1.1d branch from 7dc56e0 to e78c120 Compare September 19, 2019 20:07

fixup! test: well-defined DH groups now verify clean

94c599e

sam-github force-pushed the update-openssl1.1.1d branch from e78c120 to 94c599e Compare September 19, 2019 20:11

addaleax approved these changes Oct 1, 2019

View reviewed changes

sam-github closed this Oct 1, 2019

richardlau mentioned this pull request Oct 7, 2019

crypto: remove guard against fixed OpenSSL bug #29854

Closed

3 tasks

BridgeAR mentioned this pull request Oct 10, 2019

v12.12.0 proposal #29919

Merged

sam-github mentioned this pull request Oct 10, 2019

[v10.x backport] Update openssl 1.1.1d #29921

Closed

4 tasks

BethGriggs mentioned this pull request Oct 18, 2019

v10.17.0 proposal #29875

Merged

sam-github deleted the update-openssl1.1.1d branch November 28, 2019 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update openssl1.1.1d #29550

Update openssl1.1.1d #29550

sam-github commented Sep 13, 2019 •

edited

Loading

sam-github commented Sep 13, 2019

nodejs-github-bot commented Sep 13, 2019

mscdex commented Sep 13, 2019

mscdex commented Sep 13, 2019

sam-github commented Sep 14, 2019

mscdex commented Sep 14, 2019

bnoordhuis commented Sep 15, 2019

sam-github commented Sep 16, 2019 •

edited

Loading

kroeckx commented Sep 16, 2019

nodejs-github-bot commented Sep 17, 2019

nodejs-github-bot commented Sep 17, 2019

bnoordhuis left a comment

bnoordhuis Sep 18, 2019

richardlau commented Sep 18, 2019 •

edited

Loading

sam-github commented Sep 18, 2019

bnoordhuis commented Sep 19, 2019

nodejs-github-bot commented Sep 19, 2019

nodejs-github-bot commented Sep 20, 2019

nodejs-github-bot commented Sep 23, 2019

sam-github commented Sep 23, 2019

sam-github commented Oct 1, 2019

sam-github commented Oct 1, 2019

sam-github commented Oct 1, 2019

Update openssl1.1.1d #29550

Update openssl1.1.1d #29550

Conversation

sam-github commented Sep 13, 2019 • edited Loading

Checklist

sam-github commented Sep 13, 2019

nodejs-github-bot commented Sep 13, 2019

mscdex commented Sep 13, 2019

mscdex commented Sep 13, 2019

sam-github commented Sep 14, 2019

mscdex commented Sep 14, 2019

bnoordhuis commented Sep 15, 2019

sam-github commented Sep 16, 2019 • edited Loading

kroeckx commented Sep 16, 2019

nodejs-github-bot commented Sep 17, 2019

nodejs-github-bot commented Sep 17, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Sep 18, 2019

Choose a reason for hiding this comment

richardlau commented Sep 18, 2019 • edited Loading

sam-github commented Sep 18, 2019

bnoordhuis commented Sep 19, 2019

nodejs-github-bot commented Sep 19, 2019

nodejs-github-bot commented Sep 20, 2019

nodejs-github-bot commented Sep 23, 2019

sam-github commented Sep 23, 2019

sam-github commented Oct 1, 2019

sam-github commented Oct 1, 2019

sam-github commented Oct 1, 2019

sam-github commented Sep 13, 2019 •

edited

Loading

sam-github commented Sep 16, 2019 •

edited

Loading

richardlau commented Sep 18, 2019 •

edited

Loading