crypto: add cert check to CNNIC Whitelist

[shigeki]

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]

Doesn't make cpplint complain about the long lines?

[shigeki]

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

[shigeki]

I had typos. s/CNNNIC/CNNIC /

[bnoordhuis]

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

[bnoordhuis]

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

[shigeki]

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

[shigeki]

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]

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]

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

[shigeki]

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]

@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]

Should be static.

[shigeki]

Fixed.

[bnoordhuis]

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.

[shigeki]

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?

[bnoordhuis]

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.

[shigeki]

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

[bnoordhuis]

LGTM if the CI is happy.

[shigeki]

@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.

[bnoordhuis]

@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?

[shigeki]

@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.