Update openssl to 1.0.1o

[mhdawson]

List of changes:
https://github.com/openssl/openssl/blob/OpenSSL_1_0_1-stable/CHANGES

Skimming through them the one most likely to affect end users is:

*) Reject DH handshakes with parameters shorter than 768 bits.
[Kurt Roeckx and Emilia Kasper]

There are also some related changes we may want to make at the same time:

From io.js:

nodejs/node#1739

• Is this still needed given the openssl update, possibly as it could provide better error info.

nodejs/node#1831

• Do we still need this one given the limitation in the new version of open ssl
• patch likely applies ok to 0.12.X but 0.10.X would require refactoring

Joyent/node issue

Drop at DH group modp1 from as suggsted in -#25366

Some related docs:

• http://www.internet-computer-security.com/VPN-Guide/Diffie-Hellman.html
• https://weakdh.org/imperfect-forward-secrecy.pdf (From 25366)

[mhdawson]

@shigeki and @indutny comments or suggestions ?

[indutny]

It is not only this, but ECParameters thing too. It mainly affects the clients and node.js has tls.connect() that is vulnerable to this.

Overall, I'm +1 on upgrading it in v0.12

[mhdawson]

I was assuming but probably should have stated that we need to update both 0.10.X and 0.12.X for openssl.

@indutny, any comments on whether we want/need to also try to pull in the changes in the other PR/issues mentioned ?

[mhdawson]

Looking more closely at #25366. This test case using modp1 passes with the openssl fix to limit the size to be >768 (openssl/openssl@6383038) so its seems changing getDiffieHellman() is separate from any impact from openssl changes. This would mean we could chose to do this independently, and if we do in 0.10.X and 0.12X guard with an option to revert to the original behavior.

var crypto = require('crypto');
var alice = crypto.getDiffieHellman('modp1');
var bob = crypto.getDiffieHellman('modp1');
alice.generateKeys();
bob.generateKeys();
var alice_secret = alice.computeSecret(bob.getPublicKey(), null, 'hex');
var bob_secret = bob.computeSecret(alice.getPublicKey(), null, 'hex');
/* alice_secret and bob_secret should be the same */
console.log(alice_secret == bob_secret);

[shigeki]

@mhdawson I will submit a PR to upgrade openssl-1.0.1n today. Please wait for a moment.

[misterdjules]

@joyent/node-tsc @shigeki Moved into the 0.10.39 milestone as the OpenSSL upgrade will need to happen there too.

[sxa]

Hi, I had a play with the upgrade today before I saw @shigeki's comment in https://github.com/sxa555/node/tree/v0.10-ssl101n and https://github.com/sxa555/node/tree/v0.12-ssl101n

I'm getting a TIMEOUT failure in simple/test-tls-dhe which I haven't gone into yet but otherwise it looks ok (I also got a failure in the SSL test 707 on 0.10 but not in 0.12, although that seems reproducible with a plain 0.10.38_101m bulid on the same RHEL machine) I've tested that the branch builds cleanly on Windows too.

@shigeki If you've tried it do you get the same failure on test-tls-dhe?

[sxa]

Actually it may not matter since I've just seen that 1.0.1o has just been released to fix an ABI breakage:
https://twitter.com/mancha140/status/609386942489178112

[shigeki]

@sxa555 You need #25514 for the test-tls-dhe on v0.12. I'm going to work on upgrading to openssl-1.0.1o

[sxa]

@shigeki Cheers - I've got branches for them both at https://github.com/sxa555/node/tree/v0.12-ssl101n and https://github.com/sxa555/node/tree/v0.10-ssl101o but I'm running out of time now so all yours :)

[sxa]

(First of those should have been 101o instead of 101n obviously :) )

[mhdawson]

PR from shigeki for nodejs/node#1739 on Node 0.12.x - https://github.com/joyent/node/pull/25514/commits

@shigeki are you also working on a equivalent for 0.10.x ?

[shigeki]

DHE is not supported on v0.10.x. So we need not that PR.

[mhdawson]

I looked at resolve test-tls-dhe3.js https://github.com/joyent/node/pull/25514/commits does resolve it but I think its because it prevents a server from being created with a short key as opposed to fixing the problem we see without it which is a hang in the test case because the client (the opensslCli) rejects due to small key and we hang on the server waiting for a connection). I think it would break existing code though so we might want to consider an option to revert to original behavior as I think with a non- patched client it would still work even with the openssl update.

[mhdawson]

Looking at nodejs/node#1831 with respect to protecting the tls client. Without 1831 but with the specific ssl patch to address logjam (openssl/openssl@6383038) the following test case throws this error:

mhdawson@duv-aurora:~/node-pull/repos/rawnode/node$ ./node test/parallel/test-tls-client-shortkey.js
events.js:85
      throw er; // Unhandled 'error' event
            ^
Error: 139832391821184:error:14082174:SSL routines:SSL3_CHECK_CERT_AND_ALGORITHM:dh key too small:../deps/openssl/openssl/ssl/s3_clnt.c:3257:
    at Error (native)

testcase:

'use strict';
var common = require('../common');
var assert = require('assert');
var tls = require('tls');
var fs = require('fs');
var key =  fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem');
var cert = fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem');
function loadDHParam(n) {
  var path = common.fixturesDir;
  if (n !== 'error') path += '/keys';
  return fs.readFileSync(path + '/dh' + n + '.pem');
}
var options = {
  key: key,
  cert: cert,
  ciphers: 'DHE-RSA-AES128-GCM-SHA256',
  dhparam: loadDHParam(512)
};
var connected = false;
var server = tls.createServer(options, function(conn) {
  conn.end();
});
server.on('close', function(err) {
  assert(!err);
  assert(connected);
});
server.listen(common.PORT, '127.0.0.1', function() {
  var client = tls.connect({
    port: common.PORT,
     rejectUnauthorized: false
  }, function() {
     connected = true;
     server.close();
  });
});

Want to look at bit more closely at the changes in 1831, but I think that this means:

1. If we included 1831, an option to revert back to original behaviour would not make sense as with the openssl change we'd not be able to revert back to the original behaviour anyway
2. We many not need 1831 for the 0.10.X and 0.12.X lines since the openssl fix seems to cover the issue.

[mhdawson]

Looking at nodejs/node#1831 further it seems like it depends on functionality only in openssl-1.0.2 while the 0.10.X and 0.12.X branches are on openssl-1.0.1. Specifically I can only find SSL_CTRL_GET_SERVER_TMP_KEY in openssl-1.0.2

Based on my research so far I'm thinking we should do the following for 0.10.X and 0.12.X :

• update to openssl-1.0.1o, this will protect the tls client, unfortunately there will be no way to revert to previous behavior and will prevent users who have upgraded from connecting to servers using a smaller key than 768
• To add protection on the server side, pull in tls: make server not use DHE in less than 1024bits for fix agaist Logjam Attack node#1739 but add an option/env variable to revert to previous behavior. This is secure by default but minimizes breakage in the LTS streams but providing a way out if necessary. From earlier comments seems like this is only needed for 0.12.X
• Leave out Add a new option to limit DH key size in tls connect node#1831 as we already have protection for the client from the openssl upgrade and it depends on some functionality not available in openssl-1.0.1
• drop modp1 (option 1 in Remove (or discourage) small DH groups in crypto.getDiffieHellman #25366, limited to modp1) and use the same option/env variable to allow a revert to previous behavior
• update the docs to suggest avoiding sizes less than 2048 as suggested in Remove (or discourage) small DH groups in crypto.getDiffieHellman #25366

So far looks like we have the following pull requests:
0.10.x

• Upgrade to openssl-1.0.1o to v0.10 #25523 (upgrade openssl)

0.12.X

• tls: make server not use DHE in less than 1024bits #25514 (prevent server from using keys < 768)

So to close I think we'd need

• upgrade open ssl for 0.12.X @shigeki are you already working on that ?
• update tls: make server not use DHE in less than 1024bits #25514 to include a command line/env variable to fall back to original behaviour
• Updates to drop modp1 with option to fall back to original behaviour and doc updates for 0.10.X and 0.12.X. I believe @thinred had offered to do that in Remove (or discourage) small DH groups in crypto.getDiffieHellman #25366

The main thing to discuss might be what the command line option/environment variable might be named. As a starting suggestion how about

Command line: --enable-small-dh-groups
Env Variable: ENABLE_SMALL_DH_GROUPS

My first take is that the next major LTS version would remove this option and we should document that along with the new option.

@shigeki and @indutny does this summary of how to proceed sound reasonable to you ?

[mhdawson]

@misterdjules any comments/suggestions ?

[mhdawson]

Patch on 0.12.X for adding command line options. Want feedback on whether we are agreed on naming and if this should be committed with the other patches or on its own before I create PR.

diff --git a/src/node.cc b/src/node.cc
index e669706..4463f74 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -2936,6 +2936,7 @@ static void PrintHelp() {
 #endif
          "  --enable-ssl2        enable ssl2\n"
          "  --enable-ssl3        enable ssl3\n"
+         "  --enable-small-dh-groups  enable small dh groups (not recommended)\n"
          "\n"
          "Environment variables:\n"
 #ifdef _WIN32
@@ -2953,6 +2954,7 @@ static void PrintHelp() {
          "                       (will extend linked-in data)\n"
 #endif
 #endif
+         "ENABLE_SMALL_DH_GROUPS enable small dh groups (not recommended)\n"
          "\n"
          "Documentation can be found at http://nodejs.org/\n");
 }
@@ -3051,6 +3053,8 @@ static void ParseArgs(int* argc,
     } else if (strncmp(arg, "--icu-data-dir=", 15) == 0) {
       icu_data_dir = arg + 15;
 #endif
+    } else if (strcmp(arg, "--enable-small-dh-groups") == 0) {
+       SMALL_DH_GROUPS_ENABLE = true;
     } else {
       // V8 option.  Pass through as-is.
       new_v8_argv[new_v8_argc] = arg;
@@ -3426,6 +3430,12 @@ void Init(int* argc,
                      "(check NODE_ICU_DATA or --icu-data-dir parameters)");
   }
 #endif
+
+  const char *enableSmallDHGroups  = getenv("ENABLE_SMALL_DH_GROUPS");
+  if (enableSmallDHGroups != NULL) {
+    SMALL_DH_GROUPS_ENABLE = true;
+  }
+
   // The const_cast doesn't violate conceptual const-ness.  V8 doesn't modify
   // the argv array or the elements it points to.
   V8::SetFlagsFromCommandLine(&v8_argc, const_cast(v8_argv), true);
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 03650a9..60b0e7f 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -77,6 +77,7 @@ namespace node {
 bool SSL2_ENABLE = false;
 bool SSL3_ENABLE = false;
+bool SMALL_DH_GROUPS_ENABLE = false;
 namespace crypto {
@@ -5171,6 +5172,7 @@ void InitCrypto(Handle target,
   NODE_DEFINE_CONSTANT(target, SSL3_ENABLE);
   NODE_DEFINE_CONSTANT(target, SSL2_ENABLE);
+  NODE_DEFINE_CONSTANT(target, SMALL_DH_GROUPS_ENABLE);
 }
 }  // namespace crypto
diff --git a/src/node_crypto.h b/src/node_crypto.h
index 0a4c34a..3e62a08 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -63,6 +63,7 @@ namespace node {
 extern bool SSL2_ENABLE;
 extern bool SSL3_ENABLE;
+extern bool SMALL_DH_GROUPS_ENABLE;
 namespace crypto {

The flag should be usable from either c or javascript (var binding = process.binding('crypto'); if (binding.SMALL_DH_GROUPS_ENABLE){// do whatever})

[misterdjules]

@mhdawson Can we consider the OpenSSL upgrade separately and as higher priority than changes to prevent using smaller DHE keys and DH groups?

That would mean landing #25523 in v0.10 and porting it to v0.12, and releasing new node v0.10.39 and v0.12.5 with these changes (and other changes that might have made it into these versions in the meantime).

Then we could land changes to prevent users from using smaller DHE keys and DH groups in node v0.10.40 and node v0.12.6.

Otherwise I'm concerned that all of these changes will take a long while to get shipped in actual node releases. Doing it incrementally seems like we would be able to quickly ship v0.10.x and v0.12.x versions of node with the most severe vulnerabilities fixed, while giving us some room to make the other changes without rushing them.

@joyent/node-collaborators @mhdawson @shigeki @indutny Thoughts?

[shigeki]

I agree that upgrading 1.0.1o at first because I thin that CVE-2015-1788 is more serious than logjam. Please take #25533 with it.

@mhdawson For the command line option of #25514, we need not it if we change the limit of size check to 768 because openssl-1.0.1o has already prohibit to use less than 768 bits key and it already breaks backward compatibility. I think accepting between 768 and 1024 does not matter much. #25514 is only for notifying its limit by throwing an error and showing its warning in case between 768 and 2048 bits.

[mhdawson]

@misterdjules I'm ok with the incremental approach

@shigeki, in respect to #25514 from what I could see openssl/openssl@6383038 only prevents the client from using small keys not the server. Is there another commit in the changeset that limits the server side ?

[shigeki]

@mhdawson Oh sorry, I was confused with master(1.1.0). Yes, 1.0.2 and 1.0.1 have the limit on only client. I was wrong. Explicit limit is needed on both 1.0.2 and 1.0.1.

[thinred]

I have a comment to the ENABLE_SMALL_DH_GROUPS patch: why is the global variable called SMALL_DH_GROUPS_ENABLE and not just ENABLE_SMALL_DH_GROUPS? I find it confusing.

[mhdawson]

Agreed that it is inconsistent. I started by suggesting the names for the command line/env variable but when I put the patch together I found that to be consistent with earlier options the ENABLE made morse send at the end for the global variable.

So from an external viewpoint I like the ENABLE at the front for the external options, but for internal consistency in the code the ENABLE is at the end. This seems to be the same for ssl. @misterjules and comment on wether its better for the global variable to be consistent with the external names or with the other globals that have already been defined ?

[misterdjules]

@mhdawson I would favor consistency.

[mhdawson]

@thinred thanks for putting together the PRs, I'll try to review on Monday. The current plan is that these changes should go in for the release after the one with the openssl upgrade

[mhdawson]

@thinred, I just realized you just had changes that referenced this issue and not a PR, so I may have been jumping the gun. In any cases one comment:

instead of this: SMALL_DH_GROUPS_ENABLE = (getenv("ENABLE_SMALL_DH_GROUPS") != NULL);
you should be including node_crypto.h and then using the SMALL_DH_GROUPS_ENABLE value that it defines.

Could you update that and then create a Pull request in which we can do the review. It would probably also make sense if you pulled in the patch in my earlier comment since there is a dependency and then we can review the change as a whole. All of the changes should be squashed to 1 commit for the PR

[thinred]

@mhdawson Yeah, sorry for this, I didn't know that it will generate such a mess...
Concerning your 2nd paragraph: of course, I assumed that somebody will merge my PR and your patch, since the latter is not yet in the main tree (right?)
Ok, so I'll take your patch, merge everything and then squash it (later this evening).

[misterdjules]

@mhdawson @thinred Closing as this issue's title explicitly mentions the upgrade to OpenSSL 1.0.1o, which was done in both v0.10 and v0.12 releases today with node v0.10.39 and node v0.12.5. I would suggest creating any additional issue/pull request you may need to continue the discussions started in the comments that are not about upgrading OpenSSL.

If that's a problem please let us know and we'll reopen the issue.

Thank you!

[thinred]

The issue indeed started and continues in #25366.