crypto: fix VerifyCallback in case of verify error

3beb880716654dbb2bbb9e333758825172951775 has a bug in VerifyCallback
when preverify is 1 and the cert chain has an verify error. If the
error is UNABLE_TO_GET_ISSUER_CERT_LOCALLY, it leads an assertion
error in finding rootCA.
The whitelist check should be made only when the cert chain has no
verify error with X509_V_OK.

Fixes: https://github.com/nodejs/io.js/issues/2061
PR-URL: https://github.com/nodejs/io.js/pull/2064
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 296ca0a..915ba05 100644 (file)
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -2310,7 +2310,7 @@ inline CheckResult CheckWhitelistedServerCert(X509_STORE_CTX* ctx) {
 inline int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) {
   // Failure on verification of the cert is handled in
   // Connection::VerifyError.
-  if (preverify_ok == 0)
+  if (preverify_ok == 0 || X509_STORE_CTX_get_error(ctx) != X509_V_OK)
     return 1;
 
   // Server does not need to check the whitelist.

diff --git a/test/parallel/test-tls-cnnic-whitelist.js b/test/parallel/test-tls-cnnic-whitelist.js
index ec1177d..759ce32 100644 (file)
--- a/test/parallel/test-tls-cnnic-whitelist.js
+++ b/test/parallel/test-tls-cnnic-whitelist.js
@@ -10,33 +10,74 @@ if (!common.hasCrypto) {
 var tls = require('tls');
 var fs = require('fs');
 var path = require('path');
+var finished = 0;
 
-var error = false;
-
-// agent7-cert.pem is issued by the fake CNNIC root CA so that its
-// hash is not listed in the whitelist.
-var options = {
-  key: fs.readFileSync(path.join(common.fixturesDir, 'keys/agent7-key.pem')),
-  cert: fs.readFileSync(path.join(common.fixturesDir, 'keys/agent7-cert.pem'))
-};
-
-var server = tls.createServer(options, function(s) {
-  s.resume();
-}).listen(common.PORT, function() {
-  var client = tls.connect({
-    port: common.PORT,
-    rejectUnauthorized: true,
+function filenamePEM(n) {
+  return path.join(common.fixturesDir, 'keys', n + '.pem');
+}
+
+function loadPEM(n) {
+  return fs.readFileSync(filenamePEM(n));
+}
+
+var testCases = [
+  { // Test 0: for the check of a cert not existed in the whitelist.
+    // agent7-cert.pem is issued by the fake CNNIC root CA so that its
+    // hash is not listed in the whitelist.
     // fake-cnnic-root-cert has the same subject name as the original
     // rootCA.
-    ca: [fs.readFileSync(path.join(common.fixturesDir,
-                                   'keys/fake-cnnic-root-cert.pem'))]
-  });
-  client.on('error', function(e) {
-    assert.strictEqual(e.code, 'CERT_REVOKED');
-    error = true;
-    server.close();
+    serverOpts: {
+      key: loadPEM('agent7-key'),
+      cert: loadPEM('agent7-cert')
+    },
+    clientOpts: {
+      port: common.PORT,
+      rejectUnauthorized: true,
+      ca: [loadPEM('fake-cnnic-root-cert')]
+    },
+    errorCode: 'CERT_REVOKED'
+  },
+  // Test 1: for the fix of iojs#2061
+  // agent6-cert.pem is signed by intermidate cert of ca3.
+  // The server has a cert chain of agent6->ca3->ca1(root) but
+  // tls.connect should be failed with an error of
+  // UNABLE_TO_GET_ISSUER_CERT_LOCALLY since the root CA of ca1 is not
+  // installed locally.
+  {
+    serverOpts: {
+      ca: loadPEM('ca3-key'),
+      key: loadPEM('agent6-key'),
+      cert: loadPEM('agent6-cert')
+    },
+    clientOpts: {
+      port: common.PORT,
+      rejectUnauthorized: true
+    },
+    errorCode: 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY'
+  }
+];
+
+function runTest(tindex) {
+  var tcase = testCases[tindex];
+
+  if (!tcase) return;
+
+  var server = tls.createServer(tcase.serverOpts, function(s) {
+    s.resume();
+  }).listen(common.PORT, function() {
+    var client = tls.connect(tcase.clientOpts);
+    client.on('error', function(e) {
+      assert.strictEqual(e.code, tcase.errorCode);
+      server.close(function() {
+        finished++;
+        runTest(tindex + 1);
+      });
+    });
   });
-});
+}
+
+runTest(0);
+
 process.on('exit', function() {
-  assert(error);
+  assert.equal(finished, testCases.length);
 });