★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/28211Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Update openssl 1.1.1c #28211

Closed
wants to merge 3 commits into from
+63,239 −27,143

Conversation

Reviewers

@jasnell jasnell

@shigeki shigeki

@tniessen tniessen

@ryzokuken ryzokuken

Assignees
No one assigned
Labels
author ready backport-requested-v10.x baking-for-lts openssl
Projects
None yet
Milestone
No milestone
10 participants
@sam-github @nodejs-github-bot @richardlau @BridgeAR @Trott @jasnell @shigeki @tniessen @ryzokuken @danbev
@sam-github
Copy link
Member

commented Jun 13, 2019

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

sam-github added some commits Jun 12, 2019

@sam-github
deps: updated openssl upgrade instructions 
Patching the s390 asm rules is no longer required.

See: openssl/openssl#8351
a15b6fa
@sam-github
deps: upgrade openssl sources to 1.1.1c 
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
d44118c
@nodejs-github-bot

This comment has been minimized.

Sign in to view
Copy link

commented Jun 13, 2019 •
edited

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/

@nodejs-github-bot nodejs-github-bot added the openssl label Jun 13, 2019

@sam-github
deps: update archs files for OpenSSL-1.1.1c 
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
Loading status checks…
66b4930

@sam-github sam-github force-pushed the sam-github:update-openssl-1.1.1c branch from 2a10271 to 66b4930 Jun 13, 2019

@sam-github sam-github referenced this pull request Jun 13, 2019

Open

[v10.x] update openssl to 1.1.1c #28212

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

@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jun 13, 2019

backport: #28212
@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jun 13, 2019

@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.

@sam-github sam-github added the baking-for-lts label Jun 13, 2019

@richardlau

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 13, 2019 •
edited

@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).

@sam-github sam-github referenced this pull request Jun 13, 2019

Closed

Bot does not post or edit comment on new Jenkins builds #239

@jasnell

jasnell approved these changes Jun 14, 2019
View changes

@ryzokuken

ryzokuken approved these changes Jun 14, 2019
View changes

Copy link
Member

left a comment

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

shigeki approved these changes Jun 14, 2019
View changes

Copy link
Contributor

left a comment

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jun 14, 2019

@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

tniessen approved these changes Jun 16, 2019
View changes

Copy link
Member

left a comment

RSLGTM, thanks Sam.

@danbev danbev added the author ready label Jun 17, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 17, 2019

@sam-github @BridgeAR
deps: updated openssl upgrade instructions 
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>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
5990c4d

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 17, 2019

@sam-github @BridgeAR
deps: upgrade openssl sources to 1.1.1c 
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>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
4c8fe4a

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 17, 2019

@sam-github @BridgeAR
deps: update archs files for OpenSSL-1.1.1c 
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>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
7cb8981
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 17, 2019

Landed in b6326ce...7cb8981 🎉

@BridgeAR BridgeAR closed this Jun 17, 2019

BridgeAR added a commit that referenced this pull request Jun 17, 2019

@sam-github @BridgeAR
deps: updated openssl upgrade instructions 
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>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
e46d9e9

BridgeAR added a commit that referenced this pull request Jun 17, 2019

@sam-github @BridgeAR
deps: upgrade openssl sources to 1.1.1c 
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>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
0a2353e

BridgeAR added a commit that referenced this pull request Jun 17, 2019

@sam-github @BridgeAR
deps: update archs files for OpenSSL-1.1.1c 
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>
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
5fddde6

@BridgeAR BridgeAR referenced this pull request Jun 17, 2019

Merged

v12.5.0 proposal #28268

targos added a commit that referenced this pull request Jun 18, 2019

@sam-github @targos
deps: updated openssl upgrade instructions 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
9419daf

targos added a commit that referenced this pull request Jun 18, 2019

@sam-github @targos
deps: upgrade openssl sources to 1.1.1c 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
9c7ea2c

targos added a commit that referenced this pull request Jun 18, 2019

@sam-github @targos
deps: update archs files for OpenSSL-1.1.1c 
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>
Verified
This commit was signed with a verified signature.
targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
9ea74b7
@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 22, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 22, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 23, 2019

Pummel test issues workarounds/fixes in #28390

@sam-github sam-github deleted the sam-github:update-openssl-1.1.1c branch Jun 24, 2019

@sam-github

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Jun 24, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jun 24, 2019

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

BridgeAR added a commit that referenced this pull request Jun 26, 2019

@BridgeAR
2019-06-18, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
4360273

BridgeAR added a commit that referenced this pull request Jun 26, 2019

@BridgeAR
2019-06-18, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
65829a2

BridgeAR added a commit that referenced this pull request Jun 26, 2019

@BridgeAR
2019-06-26, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
f5e0b17

BridgeAR added a commit that referenced this pull request Jun 27, 2019

@BridgeAR
2019-06-27, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
01b404f

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jun 27, 2019

@BridgeAR
2019-06-27, Version 12.5.0 (Current) 
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
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
af3cc71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.