Upgrade openssl102d#2141

shigeki · 2015-07-09T13:09:37Z

TLS client of iojs prior this release has a vulnerability of Alternative chains certificate forgery (CVE-2015-1793) . See https://www.openssl.org/news/secadv_20150709.txt .

CI is running in https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/131/

shigeki · 2015-07-09T13:14:52Z

This needs additional tests of make internet to confirm alt cert chain. Now building on my machine.

shigeki · 2015-07-09T13:20:01Z

Internet test is fine.

$ make test-internet make -C out BUILDTYPE=Release V=1 make[1]: Entering directory `/home/ohtsu/github/io.js/out' make[1]: Nothing to be done for `all'. make[1]: Leaving directory `/home/ohtsu/github/io.js/out' ln -fs out/Release/iojs iojs /usr/bin/python tools/test.py internet [00:48|% 100|+ 11|- 0]: Done

thefourtheye · 2015-07-09T13:42:02Z

@shigeki But we don't do internet tests in CI, right? How can we check this then?

shigeki · 2015-07-09T13:44:43Z

@thefourtheyemake test-internet does not depend on platforms. It is enough to test-internet on my machine. I also working on reproducing of CVE-2015-1793 if we can include a new test.

shigeki · 2015-07-09T13:56:08Z

@thefourtheye Please do not review the original openssl sources in deps/openssl/openssl/ here. I'd like to have a review only my modification.

thefourtheye · 2015-07-09T13:58:43Z

@shigeki Oops, really sorry. My bad. I was just glancing through the source code and started leaving comments.

shigeki · 2015-07-09T13:59:51Z

@thefourtheye The original sources are in the commit of 254f85590bc8747534431d97b43a1e5d119deca0. Please review my commits out of it.

thefourtheye · 2015-07-09T14:01:21Z

I think it would have been better if there is one separate commit with your changes alone.

shigeki · 2015-07-09T14:10:34Z

67f1bc14d4baef58c6f367993dac071fa4380bbd, 2778cbbdf3d75dd3d8803aacd104f73b3ffae413, 4bebd596848e5873d9907e2633454857c18d605a and 4d83dd9d816a3a450a8c2762f1b0134ec921df3a are floating patches to openssl. You can see they were already reviewed in the previous update. They should be maintained separately because they have different purpose to fix. 9e122dbf2af07d8b47ffbe390e6029f8a86044a9 and 76b61c89476ec06b3d9a0e18f8e97fab6ce7342a are the new ones for upgrading this version.

bnoordhuis · 2015-07-09T14:11:09Z

@shigeki Would it be possible to do it as one commit that applies the delta between 1.0.2c and 1.0.2d and one commit that updates the configurations? There is a lot of churn when looking at individual commits, while the real delta is only about 1 kLoC.

shigeki · 2015-07-09T14:24:19Z

@bnoordhuis Let me confirm that you mean that the following 3 are into one commit?

diffs of original sources from 1.0.2c to 1.0.2d (254f855)
header replacement 76b61c89476ec06b3d9a0e18f8e97fab6ce7342a
floating patches (4commits: 67f1bc1, 2778cbb, 4bebd59, 4d83dd9)

And this is left as a separated commit.

config update 9e122dbf2af07d8b47ffbe390e6029f8a86044a9

shigeki · 2015-07-09T14:42:30Z

CI of https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/131/ was finished except pi1. The results are

ubuntu1504-64
test-cluster-master-error.js: AssertionError: The workers did not die after an error in the master
centos7-64 fedora21 fedora22
Jenkins error
armv7-wheezy
test-fs-readfile-error.js and test-http-curl-chunk-problem.js: no error outputs
pi2-raspbian-wheezy
test-fs-readfile-pipe-large.js and test-fs-readfilesync-pipe-large.js: errors on tmp directory

I'm not sure they are existed before but I think they are not related to this upgrade.

bnoordhuis · 2015-07-09T15:27:52Z

@shigeki Correct. If you want to go to bed, I can take care of it.

This just replaces all sources of openssl-1.0.2d.tar.gz into deps/openssl/openssl deps: copy all openssl header files to include dir All symlink files in `deps/openssl/openssl/include/openssl/` are removed and replaced with real header files to avoid issues on Windows. deps: fix openssl assembly error on ia32 win32 `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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> deps: fix asm build error of openssl in x86_win32 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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> openssl: fix keypress requirement in apps on win32 Reapply b910613 . Fixes: nodejs#589 PR-URL: nodejs#1389 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> deps: add -no_rand_screen to openssl s_client 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 <[email protected]>

They should be updated accroding to the fix of openssl/openssl@b4f0d1a

shigeki · 2015-07-09T15:32:38Z

@bnoordhuis Okay, I just squashed them. Thanks for taking care of me. I'm still okay. I will write you if I can't stay up no more.

bnoordhuis · 2015-07-09T16:24:48Z

Thanks Shigeki, LGTM. There's a minor typo in the second commit log: s/accroding/according/

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/132/

I'll land it for you if you want.

shigeki · 2015-07-09T16:27:37Z

@bnoordhuis Thanks for reviewing. I'd like to ask you to land this because I'm going to work this for node-v0.10.x from now. I appreciate your help very much.

shigeki · 2015-07-09T16:33:35Z

Note that I could not reproduce CVE-2015-1793 on iojs with the same test of openssl/openssl@593e9c6 by using same certs. The purpose of CA:false can be checked even in old iojs. Probably another conditions are needed in my test.

ohtsu@ubuntu:~/tmp/CVE-2015-1793$ ~/tmp/oldiojs/iojs-v2.3.1/iojs tls_client.js events.js:141 throw er; // Unhandled 'error' event ^ Error: unsupported certificate purpose at Error (native) at TLSSocket.<anonymous> (_tls_wrap.js:965:38) at emitNone (events.js:67:13) at TLSSocket.emit (events.js:166:7) at TLSSocket._finishInit (_tls_wrap.js:546:8)

This just replaces all sources of openssl-1.0.2d.tar.gz into deps/openssl/openssl deps: copy all openssl header files to include dir All symlink files in `deps/openssl/openssl/include/openssl/` are removed and replaced with real header files to avoid issues on Windows. deps: fix openssl assembly error on ia32 win32 `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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> deps: fix asm build error of openssl in x86_win32 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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> openssl: fix keypress requirement in apps on win32 Reapply b910613 . Fixes: nodejs#589 PR-URL: nodejs#1389 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> deps: add -no_rand_screen to openssl s_client 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 <[email protected]> PR-URL: nodejs#2141 Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis · 2015-07-09T18:05:59Z

I'm doing one more test run because Jenkins crapped out on the last one: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/133/

They should be updated according to the fix at openssl/openssl@b4f0d1a PR-URL: #2141 Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis · 2015-07-09T18:24:44Z

Jenkins is having a bad day but on the bots that work, it's solid enough that I have no qualms landing it. Merged in ca93f7f and c70e68f. Thanks again, Shigeki!

Fishrock123 · 2015-07-09T18:53:00Z

I'm really not confident about this given the results for win2008r2: https://jenkins-iojs.nodesource.com/job/iojs+pr+win/119/nodes=win2008r2/tapTestReport/

Are we sure those are Jenkins failures? ..because I'm not.

This just replaces all sources of openssl-1.0.2d.tar.gz into deps/openssl/openssl deps: copy all openssl header files to include dir All symlink files in `deps/openssl/openssl/include/openssl/` are removed and replaced with real header files to avoid issues on Windows. deps: fix openssl assembly error on ia32 win32 `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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> deps: fix asm build error of openssl in x86_win32 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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> openssl: fix keypress requirement in apps on win32 Reapply b910613 . Fixes: nodejs#589 PR-URL: nodejs#1389 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> deps: add -no_rand_screen to openssl s_client 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 <[email protected]> PR-URL: nodejs#2141 Reviewed-By: Ben Noordhuis <[email protected]>

They should be updated according to the fix at openssl/openssl@b4f0d1a PR-URL: nodejs#2141 Reviewed-By: Ben Noordhuis <[email protected]>

Fishrock123 · 2015-07-09T19:15:55Z

Hmm a CI aganist master seems to not have those failures so maybe it's fine. https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/134/

indutny · 2015-07-09T19:16:33Z

Good job everyone!

shigeki · 2015-07-09T20:14:03Z

Thanks for everyone.

I can confirm that iojs was really vulnerable to CVE-2015-1793 with the following test.

// Test for CVE-2015-1793 (Alternate Chains Certificate Forgery)//// RootCA(missing)// |// interCA// |// subinterCA subinterCA (self-signed)// | |// leaf(CA:false)----------------// |// bad(CA:false)vartls=require('tls');varfs=require('fs');varbad=fs.readFileSync('./bad.pem');varbad_key=fs.readFileSync('./bad.key');varinterCA=fs.readFileSync('./interCA.pem');varsubinterCA=fs.readFileSync('./subinterCA.pem');varsubinterCA_ss=fs.readFileSync('./subinterCA-ss.pem');varleaf=fs.readFileSync('./leaf.pem');varopts={cert: bad,key: bad_key,ca: [leaf,subinterCA]};varserver=tls.createServer(opts);server.listen(8443,function(){varopts={host: 'localhost',port: 8443,ca: [interCA,subinterCA_ss]};tls.connect(opts);});

Old iojs-v.2.3.1 passes the openssl cert verification even if leaf cert is CA:false but it is checked by iojs with verifying the common name in tls.js.

ohtsu@ubuntu:~/tmp/CVE-2015-1793$ iojs alt-cert-test.js events.js:141 throw er; // Unhandled 'error' event ^ Error: Hostname/IP doesn't match certificate's altnames: "Host: localhost. is not cert's CN: bad" at Object.checkServerIdentity (tls.js:210:15) at TLSSocket.<anonymous> (_tls_wrap.js:970:29) at emitNone (events.js:67:13) at TLSSocket.emit (events.js:166:7) at TLSSocket._finishInit (_tls_wrap.js:546:8)

On iojs with openssl-1.0.2d, it checks the leaf cert purpose. The vulnerability is surely fixed.

ohtsu@ubuntu:~/tmp/CVE-2015-1793$ ~/github/io.js/iojs alt-cert-test.js events.js:141 throw er; // Unhandled 'error' event ^ Error: unsupported certificate purpose at Error (native) at TLSSocket.<anonymous> (_tls_wrap.js:989:38) at emitNone (events.js:67:13) at TLSSocket.emit (events.js:166:7) at TLSSocket._finishInit (_tls_wrap.js:566:8)

This just replaces all sources of openssl-1.0.2d.tar.gz into deps/openssl/openssl deps: copy all openssl header files to include dir All symlink files in `deps/openssl/openssl/include/openssl/` are removed and replaced with real header files to avoid issues on Windows. deps: fix openssl assembly error on ia32 win32 `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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> deps: fix asm build error of openssl in x86_win32 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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> openssl: fix keypress requirement in apps on win32 Reapply b910613 . Fixes: nodejs#589 PR-URL: nodejs#1389 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> deps: add -no_rand_screen to openssl s_client 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 <[email protected]> PR-URL: nodejs#2141 Reviewed-By: Ben Noordhuis <[email protected]> PORT-PR-URL: nodejs#2146 PORT-FROM: ca93f7f

They should be updated according to the fix at openssl/openssl@b4f0d1a PR-URL: nodejs#2141 Reviewed-By: Ben Noordhuis <[email protected]> PORT-PR-URL: nodejs#2146 PORT-FROM: c70e68f

Notable changes * openssl: Upgrade to 1.0.2d, fixes CVE-2015-1793 (Alternate Chains Certificate Forgery) nodejs#2141.

mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Jul 9, 2015

Shigeki Ohtsu added 2 commits July 10, 2015 00:30

deps: update deps/openssl/conf/arch/*/opensslconf.h
5891f12
They should be updated accroding to the fix of openssl/openssl@b4f0d1a

shigeki force-pushed the upgrade_openssl102d branch from 67f1bc1 to 5891f12Compare July 9, 2015 15:30

thefourtheye mentioned this pull request Jul 9, 2015
Release proposal: v2.3.4 #2115
Merged

shigeki pushed a commit that referenced this pull request Jul 9, 2015
deps: update deps/openssl/conf/arch/*/opensslconf.h
c70e68f
They should be updated according to the fix at openssl/openssl@b4f0d1a PR-URL: #2141 Reviewed-By: Ben Noordhuis <[email protected]>

bnoordhuis closed this Jul 9, 2015

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jul 9, 2015
deps: update deps/openssl/conf/arch/*/opensslconf.h
52b1230
They should be updated according to the fix at openssl/openssl@b4f0d1a PR-URL: nodejs#2141 Reviewed-By: Ben Noordhuis <[email protected]>

This was referenced Jul 9, 2015
Release 1.8.4 (wrong branch target) #2145
Closed
Release 1.8.4 #2146
Merged

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 9, 2015
2015-07-09 io.js v1.8.4 Release
c84a637
Notable changes * openssl: Upgrade to 1.0.2d, fixes CVE-2015-1793 (Alternate Chains Certificate Forgery) nodejs#2141.

hmalphettes mentioned this pull request Jul 10, 2015
Update v2.3.3 to v2.3.4 and v1.8.3 to v1.8.4 nodejs/docker-iojs#78
Merged

clintwood mentioned this pull request Jul 11, 2015
[0.13.x] io.js version? nwjs/nw.js#3655
Closed

rvagg pushed a commit to rvagg/io.js that referenced this pull request Sep 16, 2015
2015-07-09 io.js v1.8.4 Release
7111fde
Notable changes * openssl: Upgrade to 1.0.2d, fixes CVE-2015-1793 (Alternate Chains Certificate Forgery) nodejs#2141.

ChALkeR pushed a commit to ChALkeR/io.js that referenced this pull request Dec 20, 2015
2015-07-09 io.js v1.8.4 Release
14f81a0
Notable changes * openssl: Upgrade to 1.0.2d, fixes CVE-2015-1793 (Alternate Chains Certificate Forgery) nodejs#2141.

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
2015-07-09 io.js v1.8.4 Release
038e946
Notable changes * openssl: Upgrade to 1.0.2d, fixes CVE-2015-1793 (Alternate Chains Certificate Forgery) nodejs#2141.

snyk-bot mentioned this pull request Apr 1, 2020
[Snyk] Upgrade prismjs from 1.17.1 to 1.19.0 O330oei/node#14
Open

Uh oh!

Upgrade openssl102d#2141

Upgrade openssl102d #2141

Uh oh!

Conversation

shigeki commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

thefourtheye commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

thefourtheye commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

thefourtheye commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

bnoordhuis commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

bnoordhuis commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

bnoordhuis commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

bnoordhuis commented Jul 9, 2015

Uh oh!

bnoordhuis commented Jul 9, 2015

Uh oh!

Fishrock123 commented Jul 9, 2015

Uh oh!

Fishrock123 commented Jul 9, 2015

Uh oh!

indutny commented Jul 9, 2015

Uh oh!

shigeki commented Jul 9, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants