Distrust certs issued after 00:00:00 Oct. 21, 2016 by StartCom and WoSign #9469
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
shigeki added tls 
nodejs-github-bot added the c++ 
ContributorAuthor
shigeki commented Nov 4, 2016
CI is running on https://ci.nodejs.org/job/node-test-pull-request/4785/. |
bnoordhuis requested changes Nov 4, 2016
Member
bnoordhuis left a comment
bnoordhuis left a comment There was a problem hiding this comment.
Mostly LGTM. There is a typo in the second commit log: s/udpate/update/
src/node_crypto.cc Outdated
Member
There was a problem hiding this comment.
You could write this as for (auto it : StartComAndWoSignDNs){
Contributor
There was a problem hiding this comment.
for (auto& startcom_wosign : StartComAndWoSignDNs){ to avoid a needless copy.
ContributorAuthor
There was a problem hiding this comment.
Done. I used const auto& since it is just a reference.
test/fixtures/keys/Makefile Outdated
ContributorAuthor
There was a problem hiding this comment.
Done. I missed Makefile is out of lint checks.
src/StartComAndWoSignData.inc Outdated
Contributor
There was a problem hiding this comment.
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.)
ContributorAuthor
There was a problem hiding this comment.
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"intmain(){char buf[1024]; constunsignedchar* startcom_wosign_data; X509_NAME* startcom_wosign_name; BIO *bio = BIO_new_fp(stdout, BIO_NOCLOSE); for(constauto& 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 reviewed Nov 7, 2016
src/node_crypto.cc Outdated
Member
There was a problem hiding this comment.
Hmm... would there possibly be a more generic way of doing this? Adding functions that are specific to StartCom and WoSign seems... troubling.
ContributorAuthor
There was a problem hiding this comment.
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 reviewed Nov 7, 2016
shigekiforce-pushed the WoSign_StartCom_check branch from e8ae182 to 20750b6CompareContributorAuthor
shigeki commented Nov 7, 2016
indutny approved these changes Nov 7, 2016
src/node_crypto.cc Outdated
ContributorAuthor
There was a problem hiding this comment.
It's not. I'm not sure why.
intX509_cmp_time(constASN1_TIME*s, time_t*t);ContributorAuthor
shigeki commented Nov 7, 2016
CI is again running on https://ci.nodejs.org/job/node-test-pull-request/4795/ . |
seishun reviewed Nov 7, 2016
src/node_crypto.cc Outdated
Contributor
There was a problem hiding this comment.
Minor, but calling it it is confusing, as it's not an iterator.
ContributorAuthor
There was a problem hiding this comment.
@seishun Yes, I rename it to dn after 'distinguished name`. Thanks.
Member
ChALkeR commented Dec 24, 2016
@bnoordhuis, does that look good to you now? |
jasnell approved these changes Dec 24, 2016
shigekiforce-pushed the WoSign_StartCom_check branch from f67befd to e11d2abCompareContributorAuthor
shigeki commented Dec 26, 2016
I've just resolved the conflict of 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. |
ContributorAuthor
shigeki commented Jan 26, 2017
Chrome 56 is released today. It is ready to land this. The approval from @bnoordhuis is needed. How is it? |
bnoordhuis approved these changes Jan 26, 2017
Member
bnoordhuis left a comment
bnoordhuis left a comment There was a problem hiding this comment.
LGTM. s/certifiate/certificate/ in the first commit log and I'd say "Remove expired certificates from CNNIC whitelist." in the second one.
shigekiforce-pushed the WoSign_StartCom_check branch from e11d2ab to 4e5bc71CompareContributorAuthor
shigeki commented Jan 26, 2017
@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. |
Member
jasnell commented Jan 27, 2017
I'm +1 on backporting this to 6 and 4. @nodejs/lts |
shigekiforce-pushed the WoSign_StartCom_check branch from 4e5bc71 to 9363bd0Compareimyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
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 <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
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/HT204132Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 7, 2017
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/HT204132Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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/HT204132Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
CNNIC Whitelist was updated with removing expired certificates. Fixes: #1895 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
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/HT204132Fixes: #9434 PR-URL: #9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Merged
shigeki pushed a commit that referenced this pull request Mar 28, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 28, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Mar 29, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: #9469Fixes: #12033 PR-URL: #12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
kevinsawicki pushed a commit to electron/node that referenced this pull request May 16, 2017
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/HT204132Fixes: nodejs/node#9434 PR-URL: nodejs/node#9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
kevinsawicki pushed a commit to electron/node that referenced this pull request May 16, 2017
CNNIC Whitelist was updated with removing expired certificates. Fixes: nodejs/node#1895 PR-URL: nodejs/node#9469 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
kevinsawicki pushed a commit to electron/node that referenced this pull request May 16, 2017
The additional validity checks applied to StartCom and WoSign certificates failed to free memory before returning. Refs: nodejs/node#9469Fixes: nodejs/node#12033 PR-URL: nodejs/node#12089 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Shigeki Ohtsu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected 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.jswhich came from 2bc7841.R: @bnoordhuis