Update openssl 1.1.1c by sam-github · Pull Request #28211 · nodejs/node

sam-github · 2019-06-13T17:24:04Z

See:

Note openssl 1.1.1c fixes CVE https://www.openssl.org/news/vulnerabilities.html#2019-1543, but I believe this does not affect node since #26537 protects it. Arguably, we could back out #26537, except that it can creep back in if an external OpenSSL 1.1.1a or b is used. Best to leave, I think.

/to @nodejs/crypto

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

Patching the s390 asm rules is no longer required. See: openssl/openssl#8351

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

nodejs-github-bot · 2019-06-13T17:24:08Z

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23868/
CI: https://ci.nodejs.org/job/node-test-pull-request/23873/

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

sam-github · 2019-06-13T20:10:57Z

backport: #28212

sam-github · 2019-06-13T20:17:08Z

@nodejs/releasers @nodejs/lts This cherry-picks clean onto 12.x-staging, but it does not onto v10.x-staging, so I backported. Possibly it needs to "bake" by being in a 12.x release before getting released on 10.x.

richardlau · 2019-06-13T20:23:17Z

@nodejs/releasers @nodejs/lts This cherry-picks clean onto 12.x-staging, but it does not onto v10.x-staging, so I backported. Possibly it needs to "bake" by being in a 12.x release before getting released on 10.x.

We did reserve a date (June 25th) for a security release across all currently supported versions of Node.js. This OpenSSL update seems like the sort of thing the reserved date was intended for (non-critical security updates).

ryzokuken

RSLGTM. I think you'd need to run license-builder.sh?

shigeki

I agree that #26537 fixed CVE-2019-1543 and leave it for the older version is used in the shared library.

sam-github · 2019-06-14T16:04:36Z

@ryzokuken I've never run license-builder.sh, and its not part of the OpenSSL update instructions (see deps/openssl/config/README.md). Should it be?

OpenSSL doesn't change its license in patches, but when I tried running it, I noticed the valgrind license seems out of date.

tniessen

RSLGTM, thanks Sam.

Patching the s390 asm rules is no longer required. See: openssl/openssl#8351 PR-URL: nodejs#28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.1c.tar.gz $ mv openssl-1.1.1c openssl $ git add --all openssl $ git commit openssl PR-URL: nodejs#28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

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: nodejs#28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

BridgeAR · 2019-06-17T09:58:00Z

Landed in b6326ce...7cb8981 🎉

Patching the s390 asm rules is no longer required. See: openssl/openssl#8351 PR-URL: #28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.1c.tar.gz $ mv openssl-1.1.1c openssl $ git add --all openssl $ git commit openssl PR-URL: #28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

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: #28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Patching the s390 asm rules is no longer required. See: openssl/openssl#8351 PR-URL: #28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.1c.tar.gz $ mv openssl-1.1.1c openssl $ git add --all openssl $ git commit openssl PR-URL: #28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

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: #28211 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Trott · 2019-06-22T19:52:39Z

Looks like this update causes test/pummel/test-crypto-dh.js to never finish. (Previously, it took about 12 seconds to run on CI.) Investigating, but if anyone has any immediate ideas, that would be helpful.

Trott · 2019-06-22T20:19:23Z

The test still passes given enough time but performance of getDiffieHellman() is much much worse than before. Might be not-a-bug if it’s due to a security fix or something?

Trott · 2019-06-23T01:40:32Z

Pummel test issues workarounds/fixes in #28390

sam-github · 2019-06-24T02:22:26Z

@Trott re:

Unfortunately, 4c8fe4a doesn't build, which is currently messing up my bisect to find where a crypto dh problem was introduced. Fortunately, it's probably this PR, so...I think I can work around it. But 66b4930 probably should have been squashed into that commit.

I wouldn't expect bisect to work across any openssl updates, because upstream sources are updated in a seperate commit from the config files generated by node's build system, according to our openssl update process, see https://github.com/nodejs/node/blob/master/deps/openssl/config/README.md#4-commit-and-make-test.

There are some pros and cons to changing the process we use to vendor in openssl updates. In general, every commit would pass make test, I think we can agree thats a big "pro". But if you look at the branches where we have floating patches on openssl (and possibly v8, or any dep, but I'm not familiar with them) that would require squashing the upstream vanilla sourc, with our floating patches and with the autogenerated config, so it would no longer be clear what in the commit came from upstream, and what came from us, which is something of a "con". I've no strong opinion either way, other than that we should be probably be consistent with how we manage all our deps, and I confess to not paying much attention to anything but the openssl updates and the mozilla certificate updates (https://github.com/nodejs/node/blob/master/doc/guides/updating-root-certs.md). The latter creates two commits in a row, too, but the first commit is just data, and doesn't break make all or make test.

Trott · 2019-06-24T03:19:14Z

@sam-github Ah, thanks for the explanation. That makes a lot of sense.

Notable changes: This release contains `semver-major` commits. These are in fact not `semver-major` due to follow-up commits that remove all breaking changes. * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default #28181 * deps: * Updated `V8` to 7.5.288.22 #27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c #28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure #27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant #27851 * report: * The cpu info got added to the report output #28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode #24260 * tools,gyp: * Introduce MSVS 2019 #27375 * util: * inspect: * Array grouping became more compact and uses more columns than before #28059 #28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines #28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated #28021 PR-URL: #28268

Notable changes: * build: * The startup time is reduced by enabling V8 snapshots by default nodejs#28181 * deps: * Updated `V8` to 7.5.288.22 nodejs#27375 * The numeric separator (v8.dev/features/numeric-separators) feature is now enabled by default * Updated `OpenSSL` to 1.1.1c nodejs#28211 * inspector: * The `--inspect-publish-uid` flag was added to specify ways of the inspector web socket url exposure nodejs#27741 * n-api: * Accessors on napi_define_* are now ECMAScript-compliant nodejs#27851 * report: * The cpu info got added to the report output nodejs#28188 * src: * Restore the original state of the stdio file descriptors on exit to prevent leaving stdio in raw or non-blocking mode nodejs#24260 * tools,gyp: * Introduce MSVS 2019 nodejs#27375 * util: * inspect: * Array grouping became more compact and uses more columns than before nodejs#28059 nodejs#28070 * Long strings will not be split at 80 characters anymore. Instead they will be split on new lines nodejs#28055 * worker: * `worker.terminate()` now returns a promise and using the callback is deprecated nodejs#28021 PR-URL: nodejs#28268

sam-github added 2 commits June 13, 2019 10:11

deps: updated openssl upgrade instructions
a15b6fa
Patching the s390 asm rules is no longer required. See: openssl/openssl#8351

deps: upgrade openssl sources to 1.1.1c
d44118c
This updates all sources in deps/openssl/openssl by: $ cd deps/openssl/ $ rm -rf openssl $ tar zxf ~/tmp/openssl-1.1.1c.tar.gz $ mv openssl-1.1.1c openssl $ git add --all openssl $ git commit openssl

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

sam-github force-pushed the update-openssl-1.1.1c branch from 2a10271 to 66b4930Compare June 13, 2019 19:20

sam-github mentioned this pull request Jun 13, 2019
[v10.x] update openssl to 1.1.1c #28212
Closed

sam-github added the backport-requested-v10.x label Jun 13, 2019

sam-github added the baking-for-lts PRs that need to wait before landing in a LTS release. label Jun 13, 2019

sam-github mentioned this pull request Jun 13, 2019
Bot does not post or edit comment on new Jenkins builds nodejs/github-bot#239
Closed

jasnell approved these changes Jun 14, 2019
View reviewed changes

ryzokuken approved these changes Jun 14, 2019
View reviewed changes

shigeki approved these changes Jun 14, 2019
View reviewed changes

tniessen approved these changes Jun 16, 2019
View reviewed changes

danbev added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 17, 2019

BridgeAR closed this Jun 17, 2019

BridgeAR mentioned this pull request Jun 17, 2019
v12.5.0 proposal #28268
Merged

sam-github deleted the update-openssl-1.1.1c branch June 24, 2019 02:15

richardlau removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Nov 26, 2021

Uh oh!

Uh oh!

Conversation

sam-github commented Jun 13, 2019

Checklist

Uh oh!

nodejs-github-bot commented Jun 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented Jun 13, 2019

Uh oh!

sam-github commented Jun 13, 2019

Uh oh!

richardlau commented Jun 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzokuken left a comment

Choose a reason for hiding this comment

Uh oh!

shigeki left a comment

Choose a reason for hiding this comment

Uh oh!

sam-github commented Jun 14, 2019

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Jun 17, 2019

Uh oh!

Trott commented Jun 22, 2019

Uh oh!

Trott commented Jun 22, 2019

Uh oh!

Trott commented Jun 23, 2019

Uh oh!

sam-github commented Jun 24, 2019

Uh oh!

Trott commented Jun 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

nodejs-github-bot commented Jun 13, 2019•
edited
Loading

richardlau commented Jun 13, 2019•
edited
Loading