Skip to content

Commit

Permalink
crypto: fix VerifyCallback in case of verify error
Browse files Browse the repository at this point in the history
3beb880 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: #2061
PR-URL: #2064
Reviewed-By: Ben Noordhuis <[email protected]>
  • Loading branch information
Shigeki Ohtsu committed Jun 27, 2015
1 parent 8cee8f5 commit 9e890fe
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 25 deletions.
2 changes: 1 addition & 1 deletion src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
89 changes: 65 additions & 24 deletions test/parallel/test-tls-cnnic-whitelist.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

0 comments on commit 9e890fe

Please sign in to comment.