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

Upgrade to OpenSSL-1.0.2m #16691

Closed
wants to merge 7 commits into from
Closed

Upgrade to OpenSSL-1.0.2m #16691

wants to merge 7 commits into from

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

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

bnoordhuis
bnoordhuis approved these changes Nov 2, 2017
View reviewed changes
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.)

@richardlau richardlau mentioned this pull request Nov 2, 2017
Closed
tniessen
tniessen approved these changes Nov 2, 2017
View reviewed changes
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

mhdawson
mhdawson approved these changes Nov 2, 2017
View reviewed changes
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

MylesBorins
MylesBorins approved these changes Nov 2, 2017
View reviewed changes
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

rvagg added a commit to nodejs/nodejs.org that referenced this pull request Nov 2, 2017
@rvagg
Blog: update to OpenSSL 1.0.2m announcement
0f1cd94 
Ref: nodejs/Release#271
Ref: nodejs-private/security#155
Ref: nodejs/node#16691
@rvagg rvagg mentioned this pull request Nov 2, 2017
Merged
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2017
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 3, 2017 •
edited
Loading

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
@shigeki
deps: upgrade openssl sources to 1.0.2m
ff1b8de 
This replaces all sources of openssl-1.0.2m.tar.gz into
deps/openssl/openssl
@shigeki
deps: copy all openssl header files to include dir
7f94976 
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.
@indutny @shigeki
deps: fix openssl assembly error on ia32 win32
b7c3b98 
`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>
@shigeki
deps: fix asm build error of openssl in x86_win32
a99627e 
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>
@shigeki
openssl: fix keypress requirement in apps on win32
a476006 
Reapply b910613 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@shigeki
deps: add -no_rand_screen to openssl s_client
34e7adf 
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>
@shigeki
deps: update openssl asm and asm_obsolete files
7506453 
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
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
Blog: update to OpenSSL 1.0.2m announcement (#1443)
c92475b 
Ref: nodejs/Release#271
Ref: nodejs-private/security#155
Ref: nodejs/node#16691
@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
Merged
Merged
@gibfahn gibfahn mentioned this pull request Nov 5, 2017
Merged
@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
@MylesBorins
2017-11-07, Version 6.12.0 'Boron' (LTS)
c6a2ec5 
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
@MylesBorins
2017-11-07, Version 4.8.6 'Argon' (Maintenance)
449d60d 
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
Merged
cjihrig added a commit to cjihrig/node that referenced this pull request Nov 7, 2017
@cjihrig
2017-11-07, Version 9.1.0 (Current)
9de633d 
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
@MylesBorins
2017-11-07, Version 4.8.6 'Argon' (Maintenance)
16b1faa 
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
@MylesBorins
2017-11-07, Version 6.12.0 'Boron' (LTS)
70dc6d5 
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
@cjihrig
2017-11-07, Version 9.1.0 (Current)
a77bfca 
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
@gibfahn
2017-11-07, Version 8.9.1 'Carbon' (LTS)
48813de 
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
@abernix abernix mentioned this pull request Nov 7, 2017
Merged
gibfahn added a commit that referenced this pull request Nov 7, 2017
@gibfahn
2017-11-07, Version 8.9.1 'Carbon' (LTS)
f5a7a9e 
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
@adisbladis adisbladis mentioned this pull request Nov 10, 2017
Closed
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@rvagg rvagg rvagg approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@mhdawson mhdawson mhdawson approved these changes

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.
10 participants
@shigeki @rvagg @MylesBorins @bnoordhuis @jasnell @cjihrig @tniessen @gireeshpunathil @mhdawson @indutny