Distrust certs issued after 00:00:00 Oct. 21, 2016 by StartCom and WoSign #9469

shigeki · 2016-11-04T15:58:05Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

crypto, tls and test.

Description of change

Per discussion of #9434, this PR checks the certs issued by StartCom and WoSign and if notBefore is after 00:00:00 Oct. 21 2016, the tls client connection is failed with CERT_REVOKED error.

This also includes CNNIC whitelist update since #1895 that expired certs were removed and a minor bug fix of test/parallel/test-tls-cnnic-whitelist.js which came from 2bc7841.

R: @bnoordhuis

shigeki · 2016-11-04T15:59:20Z

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

bnoordhuis

Mostly LGTM. There is a typo in the second commit log: s/udpate/update/

bnoordhuis · 2016-11-04T17:46:45Z

src/node_crypto.cc

+  const unsigned char* startcom_wosign_data;
+  X509_NAME* startcom_wosign_name;
+
+  for (size_t i = 0; i < arraysize(StartComAndWoSignDNs); i++) {


You could write this as for (auto it : StartComAndWoSignDNs) {

for (auto& startcom_wosign : StartComAndWoSignDNs) { to avoid a needless copy.

Done. I used const auto& since it is just a reference.

bnoordhuis · 2016-11-04T17:52:57Z

test/fixtures/keys/Makefile

+	openssl genrsa -out fake-startcom-root-key.pem 2048
+
+fake-startcom-root-cert.pem: fake-startcom-root.cnf fake-startcom-root-key.pem
+	openssl req -new -x509 -days 9999 -config fake-startcom-root.cnf -key fake-startcom-root-key.pem -out fake-startcom-root-cert.pem


Can you wrap this at 80 columns?

Done. I missed Makefile is out of lint checks.

not-an-aardvark · 2016-11-04T22:25:04Z

src/StartComAndWoSignData.inc

+  0x31, 0x2C, 0x30, 0x2A, 0x06, 0x03, 0x55, 0x04, 0x03, 0x13, 0x23, 0x53, 0x74,
+  0x61, 0x72, 0x74, 0x43, 0x6F, 0x6D, 0x20, 0x43, 0x65, 0x72, 0x74, 0x69, 0x66,
+  0x69, 0x63, 0x61, 0x74, 0x69, 0x6F, 0x6E, 0x20, 0x41, 0x75, 0x74, 0x68, 0x6F,
+  0x72, 0x69, 0x74, 0x79, 0x20, 0x47, 0x32,


Just as a precaution: How did you create these arrays? We might want to double-check that they are accurate. (I'm hoping to avoid an issue like this.)

The data is ASN.1 encoded with Name type. Here is a list of subject names included in StartComAndWoSignData.inc and they all matches the names listed in https://bugzilla.mozilla.org/show_bug.cgi?id=1309707#c16 and certdata.txt in Node.

$ cat printStartComWoSign.cc #include <cstdint> #include <cstdio> #include <x509.h> #include <bio.h> #include "StartComAndWoSignData.inc" int main() { char buf[1024]; const unsigned char* startcom_wosign_data; X509_NAME* startcom_wosign_name; BIO *bio = BIO_new_fp(stdout, BIO_NOCLOSE); for(const auto& it : StartComAndWoSignDNs) { startcom_wosign_data = it.data; startcom_wosign_name = d2i_X509_NAME(NULL, &startcom_wosign_data, it.len); X509_NAME_oneline(startcom_wosign_name, buf, sizeof buf); BIO_printf(bio, "%s\n", buf); } } $ ./printStartComWoSign /C=CN/O=WoSign CA Limited/CN=CA \xE6\xB2\x83\xE9\x80\x9A\xE6\xA0\xB9\xE8\xAF\x81\xE4\xB9\xA6 /C=CN/O=WoSign CA Limited/CN=CA WoSign ECC Root /C=CN/O=WoSign CA Limited/CN=Certification Authority of WoSign /C=CN/O=WoSign CA Limited/CN=Certification Authority of WoSign G2 /C=IL/O=StartCom Ltd./OU=Secure Digital Certificate Signing/CN=StartCom Certification Authority /C=IL/O=StartCom Ltd./CN=StartCom Certification Authority G2

jasnell · 2016-11-07T00:00:58Z

src/node_crypto.cc

@@ -2678,9 +2682,40 @@ inline X509* FindRoot(STACK_OF(X509)* sk) {
 }


-// Whitelist check for certs issued by CNNIC. See
+inline bool CertIsStartComOrWoSign(X509_NAME* name) {


Hmm... would there possibly be a more generic way of doing this? Adding functions that are specific to StartCom and WoSign seems... troubling.

I don't think it leads troubling because this function checks the cert subject name with a specific type of structure provided by Mozilla only for StartCom and WoSign distrusting.
This function (and this features) will be removed after deprecating their root cert when all issued certs are expired. So I don't think it need to be more generic.

jasnell · 2016-11-07T00:01:31Z

test/parallel/test-tls-startcom-wosign-whitelist.js

@@ -0,0 +1,91 @@
+'use strict';
+var common = require('../common');


shigeki · 2016-11-07T01:24:20Z

Fixed this according to the comments except the one from @jasnell .

@indutny Sorry for missing you to mention. Please look at this if you have time.

indutny

LGTM, if CI is green.

indutny · 2016-11-07T03:37:11Z

src/node_crypto.cc

+  if (!CertIsStartComOrWoSign(root_name))
+    return true;
+
+  time_t october_21_2016 = static_cast<time_t>(1477008000);


could it be const?

It's not. I'm not sure why.

int X509_cmp_time(const ASN1_TIME *s, time_t *t);

shigeki · 2016-11-07T04:30:18Z

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

seishun · 2016-11-07T07:30:48Z

src/node_crypto.cc

+  const unsigned char* startcom_wosign_data;
+  X509_NAME* startcom_wosign_name;
+
+  for (const auto& it : StartComAndWoSignDNs) {


Minor, but calling it it is confusing, as it's not an iterator.

@seishun Yes, I rename it to dn after 'distinguished name`. Thanks.

ChALkeR · 2016-12-24T17:51:15Z

@bnoordhuis, does that look good to you now?

shigeki · 2016-12-26T03:08:10Z

I've just resolved the conflict of test/parallel/test-tls-cnnic-whitelist.js and rebased against the latest master.

Firefox51 is to be stable on Jan 24th and Chrome56 is on Jan 31st, 2017. They will begin to have WoSing/StartCom checks in their stable release.

I think it is better to land this in the end of Jan, 2017 to align the browser release.

shigeki · 2017-01-26T14:49:59Z

Chrome 56 is released today. It is ready to land this. The approval from @bnoordhuis is needed. How is it?

bnoordhuis

LGTM. s/certifiate/certificate/ in the first commit log and I'd say "Remove expired certificates from CNNIC whitelist." in the second one.

shigeki · 2017-01-26T16:33:36Z

@bnoordhuis Oh, typo. Thanks. Fixed.

I also added an announcement link from apple https://support.apple.com/en-us/HT204132 in the commit log. I will land this tomorrow after rebasing and running CI.

After that, it should be discussed if it should be backported to LTS.

jasnell · 2017-01-27T21:58:08Z

I'm +1 on backporting this to 6 and 4. @nodejs/lts

Notable changes: * deps: * update V8 to 5.5 (Michaël Zasso) [#11029](nodejs/node#11029) * upgrade libuv to 1.11.0 (cjihrig) [#11094](nodejs/node#11094) * add node-inspect 1.10.4 (Jan Krems) [#10187](nodejs/node#10187) * upgrade zlib to 1.2.11 (Sam Roberts) [#10980](nodejs/node#10980) * lib: build `node inspect` into `node` (Anna Henningsen) [#10187](nodejs/node#10187) * crypto: Remove expired certs from CNNIC whitelist (Shigeki Ohtsu) [#9469](nodejs/node#9469) * inspector: add --inspect-brk (Josh Gavant) [#11149](nodejs/node#11149) * fs: allow WHATWG URL objects as paths (James M Snell) [#10739](nodejs/node#10739) * src: support UTF-8 in compiled-in JS source files (Ben Noordhuis) [#11129](nodejs/node#11129) * url: extend url.format to support WHATWG URL (James M Snell) [#10857](nodejs/node#10857) PR-URL: nodejs/node#11185 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

When tls client connects to the server with certification issued by either StartCom or WoSign listed in StartComAndWoSignData.inc, check notBefore of the server certificate and CERT_REVOKED error returns if it is after 00:00:00 on October 21, 2016. See for details in https://blog.mozilla.org/security/2016/10/24/distrusting-new-wosign-and-startcom-certificates/, https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html and https://support.apple.com/en-us/HT204132 Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

When tls client connects to the server with certification issued by either StartCom or WoSign listed in StartComAndWoSignData.inc, check notBefore of the server certificate and CERT_REVOKED error returns if it is after 00:00:00 on October 21, 2016. See for details in https://blog.mozilla.org/security/2016/10/24/distrusting-new-wosign-and-startcom-certificates/, https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html and https://support.apple.com/en-us/HT204132 Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

When tls client connects to the server with certification issued by either StartCom or WoSign listed in StartComAndWoSignData.inc, check notBefore of the server certificate and CERT_REVOKED error returns if it is after 00:00:00 on October 21, 2016. See for details in https://blog.mozilla.org/security/2016/10/24/distrusting-new-wosign-and-startcom-certificates/, https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html and https://support.apple.com/en-us/HT204132 Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

When tls client connects to the server with certification issued by either StartCom or WoSign listed in StartComAndWoSignData.inc, check notBefore of the server certificate and CERT_REVOKED error returns if it is after 00:00:00 on October 21, 2016. See for details in https://blog.mozilla.org/security/2016/10/24/distrusting-new-wosign-and-startcom-certificates/, https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html and https://support.apple.com/en-us/HT204132 Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469 Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

When tls client connects to the server with certification issued by either StartCom or WoSign listed in StartComAndWoSignData.inc, check notBefore of the server certificate and CERT_REVOKED error returns if it is after 00:00:00 on October 21, 2016. See for details in https://blog.mozilla.org/security/2016/10/24/distrusting-new-wosign-and-startcom-certificates/, https://security.googleblog.com/2016/10/distrusting-wosign-and-startcom.html and https://support.apple.com/en-us/HT204132 Fixes: nodejs/node#9434 PR-URL: nodejs/node#9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

CNNIC Whitelist was updated with removing expired certificates. Fixes: nodejs/node#1895 PR-URL: nodejs/node#9469 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: nodejs/node#9469 Fixes: nodejs/node#12033 PR-URL: nodejs/node#12089 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

shigeki added tls Issues and PRs related to the tls subsystem. crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests. labels Nov 4, 2016

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 4, 2016

bnoordhuis requested changes Nov 4, 2016

View reviewed changes

ChALkeR mentioned this pull request Nov 4, 2016

Drop WoSign/StartCom root certs in 8.0 #9434

Closed

not-an-aardvark reviewed Nov 4, 2016

View reviewed changes

jasnell reviewed Nov 7, 2016

View reviewed changes

shigeki force-pushed the WoSign_StartCom_check branch from e8ae182 to 20750b6 Compare November 7, 2016 01:11

indutny approved these changes Nov 7, 2016

View reviewed changes

seishun reviewed Nov 7, 2016

View reviewed changes

ChALkeR added the security Issues and PRs related to security. label Nov 7, 2016

jasnell approved these changes Dec 24, 2016

View reviewed changes

shigeki force-pushed the WoSign_StartCom_check branch from f67befd to e11d2ab Compare December 26, 2016 02:57

nodejs-github-bot added lts-watch-v6.x labels Dec 26, 2016

bnoordhuis approved these changes Jan 26, 2017

View reviewed changes

shigeki force-pushed the WoSign_StartCom_check branch from e11d2ab to 4e5bc71 Compare January 26, 2017 16:29

nodejs-github-bot added lts-watch-v6.x labels Jan 26, 2017

shigeki force-pushed the WoSign_StartCom_check branch from 4e5bc71 to 9363bd0 Compare January 31, 2017 11:06

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

MylesBorins mentioned this pull request Mar 9, 2017

v4.8.1 proposal #11760

Merged

joyeecheung mentioned this pull request Mar 28, 2017

Suspect memory leak in v6.10.1 #12033

Closed

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Mar 28, 2017

ikkisoft mentioned this pull request May 13, 2017

StartCom and WoSign certs still trusted by Electron (node update required for Electron 1.6.x) electron/electron#9457

Closed

kevinsawicki mentioned this pull request May 16, 2017

Upgrade node for StartCom/WoSign cert rejection electron/electron#9499

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Distrust certs issued after 00:00:00 Oct. 21, 2016 by StartCom and WoSign #9469

Distrust certs issued after 00:00:00 Oct. 21, 2016 by StartCom and WoSign #9469

shigeki commented Nov 4, 2016

shigeki commented Nov 4, 2016

bnoordhuis left a comment

bnoordhuis Nov 4, 2016

seishun Nov 4, 2016 •

edited

Loading

shigeki Nov 7, 2016

bnoordhuis Nov 4, 2016

shigeki Nov 7, 2016

not-an-aardvark Nov 4, 2016 •

edited

Loading

shigeki Nov 7, 2016 •

edited by TimothyGu

Loading

jasnell Nov 7, 2016

shigeki Nov 7, 2016

jasnell Nov 7, 2016

shigeki Nov 7, 2016

shigeki commented Nov 7, 2016

indutny left a comment

indutny Nov 7, 2016

shigeki Nov 7, 2016

shigeki commented Nov 7, 2016

seishun Nov 7, 2016

shigeki Nov 7, 2016

ChALkeR commented Dec 24, 2016

shigeki commented Dec 26, 2016

shigeki commented Jan 26, 2017

bnoordhuis left a comment

shigeki commented Jan 26, 2017

jasnell commented Jan 27, 2017

		@@ -0,0 +1,91 @@
		'use strict';
		var common = require('../common');

Distrust certs issued after 00:00:00 Oct. 21, 2016 by StartCom and WoSign #9469

Distrust certs issued after 00:00:00 Oct. 21, 2016 by StartCom and WoSign #9469

Conversation

shigeki commented Nov 4, 2016

Checklist

Affected core subsystem(s)

Description of change

shigeki commented Nov 4, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seishun Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

shigeki Nov 7, 2016 • edited by TimothyGu Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shigeki commented Nov 7, 2016

indutny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shigeki commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChALkeR commented Dec 24, 2016

shigeki commented Dec 26, 2016

shigeki commented Jan 26, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

shigeki commented Jan 26, 2017

jasnell commented Jan 27, 2017

seishun Nov 4, 2016 •

edited

Loading

not-an-aardvark Nov 4, 2016 •

edited

Loading

shigeki Nov 7, 2016 •

edited by TimothyGu

Loading