Adding intermediate ca to https server causes error in certain https requests

[coolaj86]

Here's the error message that comes from the native binding to openssl:

140735241175808:error:0B07C065:x509 certificate routines:X509_STORE_add_cert:cert already in hash table:../deps/openssl/openssl/crypto/x509/x509_lu.c:357

Here's what is causing it:

options = {
  key: fs.readFileSync(path.join(certsPath, 'my-server.key.pem'))
, cert: fs.readFileSync(path.join(certsPath, 'my-server.crt.pem'))
, ca: [
    // here be the problem
    // I can't know whether a particular version of node.js / io.js has this certificate already installed
    // so naturally, I add it manually as instructed by my SSL provider
    // At first blush, everything seems to work, but... (see below)
    fs.readFileSync(path.join(caCertsPath, 'intermediate.crt.pem'))
  ]
, requestCert: false
, rejectUnauthorized: false
};

server = https.createServer(options);

Adding an Intermediate CA to the configuration of creating an HTTPS server for a specific domain's cert has the unintended (and unpredictable) side effect of causing some HTTPS requests to other servers to fail with a duplicate certificate errors (but only on the first try).

// the error will only happen with certain combos of domains and ssl certs
// and only on the first request to a failing domain
// loading RapidSSL's GeoTrust intermediary in the step above
// and requesting against https on graph.facebook.com is known to cause a failure
https.request(options, function (response) {
  response.on('error', function (err) {
    console.error('https request error:', err);
  });
});

Since it's not possible to programmatically determine whether or not a certain Intermediary CA already exists in node.js/io.js' chain, the responsibility should fall to the internals to check for duplicates before adding them.

For example, a new intermediate certificate may not yet be bundled with a certain version of node.js/io.js, but it may be added in a later version. My app that was working will then inexplicably cease to function.

Tested broken in node v0.11.16 and io.js v1.1.0

Test case with real SSL cert:

1. Install the server at https://github.com/ldsorg/passport-lds-connect-example/tree/break-https-requests
2. Click to login, click to login via Facebook (it's a dummy app that can only be used by localhost, the email scope is only requested for the purpose of demonstrating scope)
3. experience the SSL request failure
4. Now comment out the ca array in serve.js
5. Repeat and note that there is no failure.

Why this is difficult to debug:

• only some requests are affected (try the example above, but use the ldsconnect login with test:test, it works)
• some requests will succeed after they have failed once (try the facebook example twice by refreshing the page)
• no known workaround (manually overriding the global CA list https.globalAgent.options.ca normally fixes similar issues, but not in this case)

The test case above is a good test case and very reproducible, but the problem would be difficult to isolate with arbitrary ssl certificates, arbitrary intermediates, and arbitrary urls which is why I haven't reduced it down further. Although there are a number of libraries in use in the example, the critical path is very straightforward and easy to trace back to the built-in https module.

If desired, I can create a screencast showing the issue and where commenting out the intermediate ca in the server setup changes the outcome (success or failure) of https requests to facebook.

[coolaj86]

It looks like this bug was introduced in v0.9 or v0.10, but perhaps so few people are using SSL directly from node with the affected bundled certs while also interacting with other services that is just hasn't been noticed.

http://stackoverflow.com/questions/16187178/nodejs-v0-10-x-freebsd-x509-store-add-certcert-already-in-hash-table/28314646

[Fishrock123]

cc @indutny?

[coolaj86]

Actually, this may be a race-condition.

Here's a screencast of me showing the problem exists in v1.1.0 but doesn't exist in v0.10.35: http://youtu.be/-_kwZLdgKjM

I changed my monitor resolution and moved a console.log() to do the screencast I was originally intending to do when I discovered the issue and then I would hit the error with v1.1.0 almost every single time - with or without the ca array in the https options config.

Then I switched to v0.10.35 and I don't hit the error at all.

Then I switched to v0.11.16 and I hit it almost every time. If I keep trying it will eventually work.

Then I switched back to v1.1.0 as a sanity check and again, hits it almost every time.

[indutny]

@coolaj86 is there any way to reproduce it with a simple test case without dependencies? This would help a lot.

[coolaj86]

Working on it, but having a hard time. It's definitely a timing bug.

So far I've found that if I use setTimeout(fn, 10) or even setImmediate(fn) to wrap the second call to https.request, I don't get the error. However, wrapping in process.nextTick(fn) still yields an error.

I'm gonna try to just use the oauth2 library directly (and skip a few passthru callbacks in the passport layer).

It's tricky because I'm fairly certain I can't reproduce the error without making multiple requests back to back (and it only errors against on the cert associated with graph.facebook.com thus far), so I have to do the whole oauth2 loop and I can't hard-code any values (because the first request to facebook will fail with a used token error and never get to the second - which is where the ssl dup cert error is happening).

[indutny]

Gosh, I know why it happens :)

[coolaj86]

???

[indutny]

@coolaj86 does this patch fix the problem for you:

diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index d052661..634540e 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -587,6 +587,7 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);

   SecureContext* sc = Unwrap<SecureContext>(args.Holder());
+  ClearErrorOnReturn clear_error_on_return;

   if (args.Length() != 1) {
     return env->ThrowTypeError("Bad parameter");
@@ -647,6 +649,7 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {

 void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
   SecureContext* sc = Unwrap<SecureContext>(args.Holder());
+  ClearErrorOnReturn clear_error_on_return;

   CHECK_EQ(sc->ca_store_, nullptr);

@@ -682,6 +685,7 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {

 void SecureContext::SetCiphers(const FunctionCallbackInfo<Value>& args) {
   SecureContext* sc = Unwrap<SecureContext>(args.Holder());
+  ClearErrorOnReturn clear_error_on_return;

   if (args.Length() != 1 || !args[0]->IsString()) {
     return sc->env()->ThrowTypeError("Bad parameter");
@@ -721,6 +725,7 @@ void SecureContext::SetECDHCurve(const FunctionCallbackInfo<Value>& args) {
 void SecureContext::SetDHParam(const FunctionCallbackInfo<Value>& args) {
   SecureContext* sc = Unwrap<SecureContext>(args.This());
   Environment* env = sc->env();
+  ClearErrorOnReturn clear_error_on_return;

   // Auto DH is not supported in openssl 1.0.1, so dhparam needs
   // to be specifed explicitly
@@ -825,6 +830,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
   bool ret = false;

   SecureContext* sc = Unwrap<SecureContext>(args.Holder());
+  ClearErrorOnReturn clear_error_on_return;

   if (args.Length() < 1) {
     return env->ThrowTypeError("Bad parameter");

?

[indutny]

There was mistake in the patch, please reapply

[indutny]

Should be fixed by https://github.com/iojs/io.js/pull/719

[coolaj86]

make was taking too long... just switched to make -j 4

(it's gotta compile everything because I haven't done this in a while - like since v0.6.x)

[coolaj86]

Survey says: IT WORKS!!!!

I'll run through some more tests a little later, but so far I'm about 20 for 20 with 100% success.

Here's what I did (for any onlookers that need the fix today):

git clone git@github.com:iojs/io.js.git
pushd io.js
vim crypto.diff # copied text from above
git apply crypto.diff

./configure
make -j 4

sudo chown -R $(whoami):staff /usr/local/

make install

iojs /path/to/server.js

Had to wait quite a while for the compile to finish.... either v8 is growing or my macbook just isn't what it used to be.

[coolaj86]

Can I buy you a soda (we're not as much into alcohol in Utah) or gratipay you or something?

[indutny]

Hahah, no need in soda, thanks!

[indutny]

Fixed!

[pke]

in which nodejs release is this fixed? I am getting this still in v0.12.7

[indutny]

@pke it didn't land in v0.12 branch. May I ask you to open an issue with text like this:

Backport https://github.com/nodejs/io.js/commit/6f7a9784eaef82a1aa6cf53bbbd7224c446876a0 to v0.12

;) Thank you!

[pke]

@indutny In which repo? io or node?

[indutny]

@pke joyent/node

[pke]

Done.

[indutny]

Thank you, @pke!