crypto: add cert check to CNNIC Whitelist #1895

shigeki · 2015-06-04T16:06:53Z

When client connect to the server with certification issued by either CNNNIC Root CA or CNNNIC EV Root CA, check hash of server certification in the list of CNNICHashWhitelist.inc. If it's not, CERT_REVOKED error returns.

See for details in
https://blog.mozilla.org/security/2015/04/02/distrusting-new-cnnic-certificates/

Fixes: #1871

bnoordhuis · 2015-06-04T16:09:10Z

src/node_crypto.cc

@@ -13,6 +13,9 @@
 #include "util.h"
 #include "util-inl.h"
 #include "v8.h"
+// CNNIC Hash WhiteList is taken from
+// https://hg.mozilla.org/mozilla-central/raw-file/98820360ab66/security/certverifier/CNNICHashWhitelist.inc


Doesn't make cpplint complain about the long lines?

Woops, make test did not call make cpplint . I have to fix several styles.

shigeki · 2015-06-04T16:09:32Z

I had typos. s/CNNNIC/CNNIC /

bnoordhuis · 2015-06-04T16:11:38Z

src/node_crypto.cc

+    "\x03\x55\x04\x03\x13\x0A\x43\x4E\x4E\x49\x43\x20\x52\x4F\x4F\x54";
+const unsigned char* cnnic_p = CNNIC_ROOT_CA_SUBJECT_DATA;
+static X509_NAME *cnnic_name = d2i_X509_NAME(NULL, &cnnic_p,
+                                             sizeof(CNNIC_ROOT_CA_SUBJECT_DATA));


Style issues: star leans left and nullptr, not NULL. cnnic_p should be static.

Also, I think you're including the zero byte in the length now? Is that correct?

Styles are fixed. The length of sizeof(CNNIC_ROOT_CA_SUBJECT_DATA) is not zero. Did I read your comment correctly?

shigeki · 2015-06-04T17:43:12Z

The commit was updated according comments except the case of root_cert is nulland the duplicated call of SSL_get_verify_result(). I will take care of them tomorrow.

shigeki · 2015-06-04T17:53:26Z

The test of revoked case was made with something artificial change by removing one existing cert hash of www1.cnnic.cn from the whitelist as below. I'm not sure how to test this in general.

diff --git a/src/CNNICHashWhitelist.inc b/src/CNNICHashWhitelist.inc
index e3c6a61..652209e 100644
--- a/src/CNNICHashWhitelist.inc
+++ b/src/CNNICHashWhitelist.inc
@@ -636,10 +636,10 @@ static const struct WhitelistedCNNICHash WhitelistedCNNICHashes[] = {
     { 0x1B, 0xEC, 0xFE, 0x78, 0xCE, 0x5E, 0x77, 0xA9, 0x77, 0xBB, 0x5F, 0xE3, 0x49, 0x91, 0x06, 0xC6,
       0x4C, 0xF2, 0xB0, 0x76, 0x16, 0x59, 0x49, 0x04, 0x11, 0x17, 0xCD, 0x8A, 0xBC, 0xD9, 0x05, 0xD4 },
   },
-  {
-    { 0x1B, 0xF4, 0x8A, 0x83, 0x3C, 0xE4, 0x05, 0x64, 0x8C, 0xC0, 0xBD, 0xD3, 0xB5, 0xB8, 0xC1, 0x8E,
-      0xB5, 0x13, 0x15, 0x34, 0x29, 0x3A, 0xB2, 0x63, 0x44, 0xB5, 0x00, 0x76, 0x48, 0x11, 0x41, 0xED },
-  },
+//  {
+//    { 0x1B, 0xF4, 0x8A, 0x83, 0x3C, 0xE4, 0x05, 0x64, 0x8C, 0xC0, 0xBD, 0xD3, 0xB5, 0xB8, 0xC1, 0x8E,
+//      0xB5, 0x13, 0x15, 0x34, 0x29, 0x3A, 0xB2, 0x63, 0x44, 0xB5, 0x00, 0x76, 0x48, 0x11, 0x41, 0xED },
+//  },
   {
     { 0x1C, 0x04, 0x82, 0x0F, 0x7B, 0x4A, 0x2F, 0x1E, 0x38, 0x5D, 0xE1, 0xDE, 0x16, 0xB2, 0x22, 0x6E,
       0x88, 0x3D, 0x9C, 0x34, 0x66, 0x3E, 0x1B, 0x64, 0xE8, 0x5B, 0x98, 0x0E, 0xAF, 0xF0, 0xB9, 0xD3 },

var tls = require('tls');
var client = tls.connect(443, 'www1.cnnic.cn',{
},function() {
  client.end();
});
client.on('clientError', function(e) {
 console.log('clientError:', e);
});
client.on('error', function(e) {
 console.log('error:', e);
});

$ ./iojs ~/cnnic_test.js
error: { [Error: certificate revoked] code: 'CERT_REVOKED' }

bnoordhuis · 2015-06-04T18:27:51Z

src/node_crypto.cc

+    "\x03\x55\x04\x03\x13\x0A\x43\x4E\x4E\x49\x43\x20\x52\x4F\x4F\x54";
+const uint8_t* cnnic_p = CNNIC_ROOT_CA_SUBJECT_DATA;
+static X509_NAME* cnnic_name =
+    d2i_X509_NAME(nullptr, &cnnic_p, sizeof(CNNIC_ROOT_CA_SUBJECT_DATA));


What I mean with the zero byte is that sizeof("...") also includes the implicit '\0'. I speculate that's not the intent here.

Thanks, I missed it. It should be sizeof(CNNIC_ROOT_CA_SUBJECT_DATA)-1. Fortunately d2i API handles ASN.1 length properly so that the rest of unnecessary data are ignored. I will fix it.

shigeki · 2015-06-10T05:58:49Z

@bnoordhuis Sorry for my late response.
I found a serious bug in my previous commit. The chain obtained from SSL_get_peer_cert_chain() only contains certs sent from a server so that root certs added with SSL_CTX_set_cert_store() are not included. The completed cert chain can be obtained via the store context in VerifyCallback.

In the X509_verify_cert() where the verify callback is invoked, the cert chain is stacked from leaf to root in order during examining the issuer but I don't have a confident that the root is always stacked at the last in the cert chain so that FindRoot is still remained and add the check the root_cert never to be null.

I also added the tests to confirm this whitelist check. Please take an another look.

bnoordhuis · 2015-06-14T22:35:53Z

src/node_crypto.cc

+    "\x30\x32\x31\x0B\x30\x09\x06\x03\x55\x04\x06\x13\x02\x43\x4E\x31\x0E\x30"
+    "\x0C\x06\x03\x55\x04\x0A\x13\x05\x43\x4E\x4E\x49\x43\x31\x13\x30\x11\x06"
+    "\x03\x55\x04\x03\x13\x0A\x43\x4E\x4E\x49\x43\x20\x52\x4F\x4F\x54";
+const uint8_t* cnnic_p = CNNIC_ROOT_CA_SUBJECT_DATA;


Should be static.

bnoordhuis · 2015-06-14T22:42:38Z

src/node_crypto.cc

+                           ARRAY_SIZE(WhitelistedCNNICHashes),
+                           CNNIC_WHITELIST_HASH_LEN, compar);
+    if (result == nullptr) {
+      X509_STORE_CTX_set_error(ctx, X509_V_ERR_CERT_REVOKED);


Having this side effect here still doesn't sit right with me. I'd really prefer it if you (for example) changed it to return a ternary value.

Could you explain your worry about the side effects more clearly?

In X509_STORE_CTX_get_error(3SSL) ,

X509_STORE_CTX_set_error() sets the error code of ctx to s. For example it might be used in a verification callback to set an error based on additional checks.

it says that X509_STORE_CTX_set_error can be used in a verify callback.
In this case when the callback returns 0, it immediately goes to finish X509_verify_cert() as https://github.com/openssl/openssl/blob/master/crypto/x509/x509_vfy.c#L1785-L1787
and put the context error to verify_result as
https://github.com/openssl/openssl/blob/OpenSSL_1_0_2-stable/ssl/ssl_cert.c#L759-L768. I could not find any side effects here.

If this has the side effect, how can I set an error code?

One solution is to make this function return an enum { ALLOWED, DISALLOWED, REVOKED } instead of an int. Then, in VerifyCallback(), call X509_STORE_CTX_set_error() iff the return value is REVOKED.

If you change the return value, you should rename the function, because it's no longer the boolean function that the IsFoo() name implies.

I've just update this PR. Does 5023980183dfce012dc9d000d8f5853a739e14c1 meet your comment above?

bnoordhuis · 2015-06-15T18:21:36Z

LGTM if the CI is happy.

When client connect to the server with certification issued by either CNNIC Root CA or CNNIC EV Root CA, check hash of server certification in the list of CNNICHashWhitelist.inc. If it's not, CERT_REVOKED error returns. See for details in https://blog.mozilla.org/security/2015/04/02/distrusting-new-cnnic-certificates/

When client connect to the server with certification issued by either CNNIC Root CA or CNNIC EV Root CA, check hash of server certification in the list of CNNICHashWhitelist.inc. If it's not, CERT_REVOKED error returns. See for details in https://blog.mozilla.org/security/2015/04/02/distrusting-new-cnnic-certificates/ PR-URL: #1895 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

shigeki · 2015-06-16T14:33:21Z

@bnoordhuis Thanks for reviewing many times.

CI is https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/19/.
Most of tests are green except one test failure of test-util-debug.js in armv7-wheezy which is not related this PR.

Landed in 3beb880.

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>

CNNIC Whitelist was updated with removing expired certificates. Fixes: nodejs#1895 PR-URL: nodejs#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>

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>

bnoordhuis · 2019-03-08T14:09:51Z

src/node_crypto.cc

+    root_cert = FindRoot(chain);
+    CHECK_NE(root_cert, nullptr);
+    root_name = X509_get_subject_name(root_cert);
+  }


@shigeki Do you remember why you wrote it like this? Unless I misunderstand something a valid chain contains only one self-signed certificate and it's always the last one, isn't it?

@bnoordhuis Honestly I did not remember it. But I guess I wrote it in order to care about the alt cert chain which has a cross cert in its chain as below. When the SHA-1 root was deleted in the root store, I was just afraid of the case when the last one in the cert chain stack was not SHA-2 root but its cross root to SHA-1.

shigeki mentioned this pull request Jun 4, 2015

tls: whitelist CNNIC certificates? #1871

Closed

bnoordhuis reviewed Jun 4, 2015
View reviewed changes

mscdex added the crypto Issues and PRs related to the crypto subsystem. label Jun 4, 2015

bnoordhuis reviewed Jun 4, 2015
View reviewed changes

shigeki force-pushed the CNNIC_Whitelist branch 2 times, most recently from bef171f to 062ac11 Compare June 4, 2015 17:40

bnoordhuis reviewed Jun 4, 2015
View reviewed changes

shigeki force-pushed the CNNIC_Whitelist branch from 062ac11 to ad61baf Compare June 10, 2015 05:38

bnoordhuis reviewed Jun 14, 2015
View reviewed changes

shigeki force-pushed the CNNIC_Whitelist branch from ad61baf to b57e36c Compare June 15, 2015 08:27

shigeki force-pushed the CNNIC_Whitelist branch from 5023980 to ee6334c Compare June 16, 2015 00:28

shigeki closed this Jun 16, 2015

rvagg mentioned this pull request Jun 16, 2015

Release proposal: v2.3.1 #1996

Closed

bnoordhuis mentioned this pull request Nov 3, 2016

Drop WoSign/StartCom root certs in 8.0 #9434

Closed

shigeki mentioned this pull request Nov 4, 2016

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

Closed

4 tasks

TimothyGu mentioned this pull request Aug 29, 2017

Why do you trust CNNIC? #15073

Closed

bnoordhuis reviewed Mar 8, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: add cert check to CNNIC Whitelist #1895

crypto: add cert check to CNNIC Whitelist #1895

shigeki commented Jun 4, 2015

bnoordhuis Jun 4, 2015

shigeki Jun 4, 2015

shigeki commented Jun 4, 2015

bnoordhuis Jun 4, 2015

bnoordhuis Jun 4, 2015

shigeki Jun 4, 2015

shigeki commented Jun 4, 2015

shigeki commented Jun 4, 2015

bnoordhuis Jun 4, 2015

shigeki Jun 8, 2015

shigeki commented Jun 10, 2015

bnoordhuis Jun 14, 2015

shigeki Jun 15, 2015

bnoordhuis Jun 14, 2015

shigeki Jun 15, 2015

bnoordhuis Jun 15, 2015

shigeki Jun 15, 2015

bnoordhuis commented Jun 15, 2015

shigeki commented Jun 16, 2015

bnoordhuis Mar 8, 2019

shigeki Mar 8, 2019

crypto: add cert check to CNNIC Whitelist #1895

crypto: add cert check to CNNIC Whitelist #1895

Conversation

shigeki commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shigeki commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shigeki commented Jun 4, 2015

shigeki commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shigeki commented Jun 10, 2015

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

Choose a reason for hiding this comment

bnoordhuis commented Jun 15, 2015

shigeki commented Jun 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment