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 openssl102d #2141

Closed
wants to merge 2 commits into from
Closed

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Jul 9, 2015

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
Copy link
Contributor Author

shigeki commented Jul 9, 2015

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

@shigeki
Copy link
Contributor Author

shigeki commented Jul 9, 2015

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

@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Jul 9, 2015
@thefourtheye
Copy link
Contributor

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

@shigeki
Copy link
Contributor Author

shigeki commented Jul 9, 2015

@thefourtheye make 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
Copy link
Contributor Author

shigeki commented Jul 9, 2015

@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
Copy link
Contributor

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

@shigeki
Copy link
Contributor Author

shigeki commented Jul 9, 2015

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

@thefourtheye
Copy link
Contributor

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

@shigeki
Copy link
Contributor Author

shigeki commented Jul 9, 2015

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
Copy link
Member

@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
Copy link
Contributor Author

shigeki commented Jul 9, 2015

@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
Copy link
Contributor Author

shigeki commented Jul 9, 2015

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
Copy link
Member

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

Shigeki Ohtsu added 2 commits July 10, 2015 00:30
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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

openssl: fix keypress requirement in apps on win32

Reapply b910613 .

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

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 <info@bnoordhuis.nl>
They should be updated accroding to the fix of
openssl/openssl@b4f0d1a
@shigeki shigeki force-pushed the upgrade_openssl102d branch from 67f1bc1 to 5891f12 Compare July 9, 2015 15:30
@shigeki
Copy link
Contributor Author

shigeki commented Jul 9, 2015

@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
Copy link
Member

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
Copy link
Contributor Author

shigeki commented Jul 9, 2015

@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
Copy link
Contributor Author

shigeki commented Jul 9, 2015

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)

bnoordhuis pushed a commit to bnoordhuis/io.js that referenced this pull request Jul 9, 2015
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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

openssl: fix keypress requirement in apps on win32

Reapply b910613 .

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

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 <info@bnoordhuis.nl>

PR-URL: nodejs#2141
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

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/

shigeki pushed a commit that referenced this pull request Jul 9, 2015
They should be updated according to the fix at
openssl/openssl@b4f0d1a

PR-URL: #2141
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

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!

@bnoordhuis bnoordhuis closed this Jul 9, 2015
@Fishrock123
Copy link
Contributor

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.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jul 9, 2015
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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

openssl: fix keypress requirement in apps on win32

Reapply b910613 .

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

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 <info@bnoordhuis.nl>

PR-URL: nodejs#2141
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jul 9, 2015
They should be updated according to the fix at
openssl/openssl@b4f0d1a

PR-URL: nodejs#2141
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@Fishrock123
Copy link
Contributor

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
Copy link
Member

indutny commented Jul 9, 2015

Good job everyone!

This was referenced Jul 9, 2015
@shigeki
Copy link
Contributor Author

shigeki commented Jul 9, 2015

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)

var tls = require('tls');
var fs = require('fs');
var bad = fs.readFileSync('./bad.pem');
var bad_key = fs.readFileSync('./bad.key');
var interCA = fs.readFileSync('./interCA.pem');
var subinterCA = fs.readFileSync('./subinterCA.pem');
var subinterCA_ss = fs.readFileSync('./subinterCA-ss.pem');
var leaf = fs.readFileSync('./leaf.pem');

var opts = {
  cert: bad,
  key: bad_key,
  ca: [leaf, subinterCA]
};
var server = tls.createServer(opts);
server.listen(8443, function() {
  var opts = {
    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)

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jul 9, 2015
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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>

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 <fedor@indutny.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

openssl: fix keypress requirement in apps on win32

Reapply b910613 .

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

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 <info@bnoordhuis.nl>

PR-URL: nodejs#2141
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PORT-PR-URL: nodejs#2146
PORT-FROM: ca93f7f
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jul 9, 2015
They should be updated according to the fix at
openssl/openssl@b4f0d1a

PR-URL: nodejs#2141
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

PORT-PR-URL: nodejs#2146
PORT-FROM: c70e68f
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 9, 2015
Notable changes

* openssl: Upgrade to 1.0.2d, fixes CVE-2015-1793 (Alternate Chains
Certificate Forgery) nodejs#2141.
rvagg pushed a commit to rvagg/io.js that referenced this pull request Sep 16, 2015
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
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
Notable changes

* openssl: Upgrade to 1.0.2d, fixes CVE-2015-1793 (Alternate Chains
Certificate Forgery) nodejs#2141.
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.

6 participants