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.

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

Upgrade to OpenSSL-1.0.2m #16691

Closed
wants to merge 7 commits into from
Closed

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Nov 2, 2017

This upgrades to OpenSSL-1.0.2m . It includes the fix of the moderate severity of CVE-2017-3736 that affects Node in RSA calculations of TLS and crypto modules but the attack is said to be very difficult.

This upgrades have no changes in opensslconf.h in the config dir.

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
Affected core subsystem(s)

deps/openssl

R @nodejs/crypto or others.

@shigeki shigeki added the openssl Issues and PRs related to the OpenSSL dependency. label Nov 2, 2017
@shigeki
Copy link
Contributor Author

shigeki commented Nov 2, 2017

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM (mostly rubber-stamp.)

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.

Rubberstamp LGTM

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

@rvagg
Copy link
Member

rvagg commented Nov 2, 2017

needing impact assessment of CVE-2017-3736 so we can decide on whether to rush releases out nodejs/Release#271, @nodejs/crypto please weigh in

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

CI is good... fails are all flakes or infra

My impression of the assessment was that we don't need to rush a release tomorrow but can bundle this with other fixes on tuesday.

@rvagg
Copy link
Member

rvagg commented Nov 2, 2017

running CI again @ rebuilding @ https://ci.nodejs.org/job/node-test-commit/13690/, some odd errors in there, and the flaky test-http2-server-rst-stream should be fixed by 3a977fc now

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 3, 2017

linux failures are a known async flake (there is an open Pr to set it to flaky)

arm-fanned failures appear infra related, opened [build issue]

windows failure is both the same async failure as above and another failure also marked flaky that appears unrelated

  • sequential/test-inspector-bindings # TODO : Fix flaky test

shigeki and others added 7 commits November 3, 2017 09:18
This replaces all sources of openssl-1.0.2m.tar.gz into
deps/openssl/openssl
All symlink files in `deps/openssl/openssl/include/openssl/`
are removed and replaced with real header files to avoid
issues on Windows. Two files of opensslconf.h in crypto and
include dir are replaced to refer config/opensslconf.h.
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reapply b910613 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
In openssl s_client on Windows, RAND_screen() is invoked to initialize
random state but it takes several seconds in each connection.
This added -no_rand_screen to openssl s_client on Windows to skip
RAND_screen() and gets a better performance in the unit test of
test-tls-server-verify.
Do not enable this except to use in the unit test.

Fixes: nodejs#1461
PR-URL: nodejs#1836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Regenerate asm files with Makefile and CC=gcc and ASM=nasm where gcc
version was 5.4.0 and nasm version was 2.11.08.

Also asm files in asm_obsolete dir to support old compiler and
assembler are regenerated without CC and ASM envs.
@shigeki shigeki force-pushed the upgrade_openssl102m branch from 6fa53c6 to 7506453 Compare November 3, 2017 00:19
@shigeki
Copy link
Contributor Author

shigeki commented Nov 3, 2017

I've just rebased this with the current master that has a fix of the flaky test.

@shigeki
Copy link
Contributor Author

shigeki commented Nov 3, 2017

A new CI is running on https://ci.nodejs.org/job/node-test-pull-request/11160/.

rvagg added a commit to nodejs/nodejs.org that referenced this pull request Nov 3, 2017
@rvagg rvagg self-requested a review November 3, 2017 03:34
@MylesBorins
Copy link
Contributor

One more CI set to rebase against master

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

@MylesBorins
Copy link
Contributor

CI is green aside from windows failures fixed by 3d4d5e0

If there are no complaints I'll land this on master in just over an hour

@MylesBorins
Copy link
Contributor

Landed on all relevant staging branches

This was referenced Nov 3, 2017
@gibfahn gibfahn mentioned this pull request Nov 5, 2017
@rvagg
Copy link
Member

rvagg commented Nov 6, 2017

Great job @shigeki! Thanks for jumping on this so promptly and giving the thorough impact assessment.

MylesBorins added a commit that referenced this pull request Nov 6, 2017
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
MylesBorins added a commit that referenced this pull request Nov 6, 2017
Notable Changes:

* **crypto**:
  - update root certificates (Ben Noordhuis)
    #13279
  - update root certificates (Ben Noordhuis)
    #12402
* **deps**:
  - add support for more modern versions of INTL (Bruno Pagani)
    #13040
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade openssl sources to 1.0.2l (Daniel Bevenius)
    #13233

PR-URL: #16500
@cjihrig cjihrig mentioned this pull request Nov 6, 2017
cjihrig added a commit to cjihrig/node that referenced this pull request Nov 7, 2017
Notable changes:

* CLI:
  - NODE_OPTIONS now supports the --stack-trace-limit option.
    nodejs#16495
* deps:
  - OpenSSL is upgraded to 1.0.2m
    nodejs#16691
* http:
  - A 'connect' event handler leak has been fixed.
    nodejs#16725
  - The 103 Early Hints status code is now supported.
    nodejs#16644

PR-URL: nodejs#16851
MylesBorins added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

* **crypto**:
  - update root certificates (Ben Noordhuis)
    #13279
  - update root certificates (Ben Noordhuis)
    #12402
* **deps**:
  - add support for more modern versions of INTL (Bruno Pagani)
    #13040
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade openssl sources to 1.0.2l (Daniel Bevenius)
    #13233

PR-URL: #16500
MylesBorins added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    #12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    #11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    #16691
  - upgrade libuv to 1.15.0 (cjihrig)
    #15745
  - upgrade libuv to 1.14.1 (cjihrig)
    #14866
  - upgrade libuv to 1.13.1 (cjihrig)
    #14117
  - upgrade libuv to 1.12.0 (cjihrig)
    #13306
* fs:
  - Add support for fs.write/fs.writeSync(fd, buffer, cb) and
    fs.write/fs.writeSync(fd, buffer, offset, cb) as documented
    (Andreas Lind) #7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    #12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    #10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    #12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    #13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    #13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    #12677
* test:
  - remove common.fail() (Rich Trott)
    #12293

PR-URL: #16263
cjihrig added a commit that referenced this pull request Nov 7, 2017
Notable changes:

* CLI:
  - NODE_OPTIONS now supports the --stack-trace-limit option.
    #16495
* deps:
  - OpenSSL is upgraded to 1.0.2m
    #16691
* http:
  - A 'connect' event handler leak has been fixed.
    #16725
  - The 103 Early Hints status code is now supported.
    #16644

PR-URL: #16851
gibfahn added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

- **openssl**:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu) [#16691](#16691)
- ***Revert*** "**https**:
  - refactor to use http internals" (Myles Borins) [#16660](#16660)

PR-URL: #16783
gibfahn added a commit that referenced this pull request Nov 7, 2017
Notable Changes:

- **openssl**:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu) [#16691](#16691)
- ***Revert*** "**https**:
  - refactor to use http internals" (Myles Borins) [#16660](#16660)

PR-URL: #16783
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants