tls: honorCipherOrder should not degrade defaults

Specifying honorCipherOrder should not change the SSLv2/SSLv3 defaults
for a TLS server.

Use secureOptions logic in both lib/tls.js and lib/crypto.js

3 files changed, 167 insertions, 20 deletions

diff --git a/lib/crypto.js b/lib/crypto.js
index f88c55d0a..597d196f2 100644
--- a/lib/crypto.js
+++ b/lib/crypto.js
@@ -61,6 +61,31 @@ var StringDecoder = require('string_decoder').StringDecoder;
 
 var CONTEXT_DEFAULT_OPTIONS = undefined;
 
+function getSecureOptions(secureProtocol, secureOptions) {
+  if (CONTEXT_DEFAULT_OPTIONS === undefined) {
+    CONTEXT_DEFAULT_OPTIONS = 0;
+
+    if (!binding.SSL3_ENABLE)
+      CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv3;
+
+    if (!binding.SSL2_ENABLE)
+      CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv2;
+  }
+
+  if (secureOptions === undefined) {
+    if (secureProtocol === undefined ||
+        secureProtocol === 'SSLv23_method' ||
+        secureProtocol === 'SSLv23_server_method' ||
+        secureProtocol === 'SSLv23_client_method') {
+      secureOptions |= CONTEXT_DEFAULT_OPTIONS;
+    }
+  }
+
+  return secureOptions;
+}
+exports._getSecureOptions = getSecureOptions;
+
+
 function Credentials(secureProtocol, flags, context) {
   if (!(this instanceof Credentials)) {
     return new Credentials(secureProtocol, flags, context);
@@ -82,24 +107,7 @@ function Credentials(secureProtocol, flags, context) {
     }
   }
 
-  if (CONTEXT_DEFAULT_OPTIONS === undefined) {
-    CONTEXT_DEFAULT_OPTIONS = 0;
-
-    if (!binding.SSL3_ENABLE)
-      CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv3;
-
-    if (!binding.SSL2_ENABLE)
-      CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv2;
-  }
-
-  if (flags === undefined) {
-    if (secureProtocol === undefined ||
-        secureProtocol === 'SSLv23_method' ||
-        secureProtocol === 'SSLv23_server_method' ||
-        secureProtocol === 'SSLv23_client_method') {
-      flags |= CONTEXT_DEFAULT_OPTIONS;
-    }
-  }
+  flags = getSecureOptions(secureProtocol, flags);
 
   this.context.setOptions(flags);
 }
diff --git a/lib/tls.js b/lib/tls.js
index 392f7ad2b..adc8efa63 100644
--- a/lib/tls.js
+++ b/lib/tls.js
@@ -1239,11 +1239,16 @@ Server.prototype.setOptions = function(options) {
   if (options.secureProtocol) this.secureProtocol = options.secureProtocol;
   if (options.crl) this.crl = options.crl;
   if (options.ciphers) this.ciphers = options.ciphers;
-  var secureOptions = options.secureOptions || 0;
+
+  var secureOptions = crypto._getSecureOptions(options.secureProtocol,
+                                               options.secureOptions);
+
   if (options.honorCipherOrder) {
     secureOptions |= constants.SSL_OP_CIPHER_SERVER_PREFERENCE;
   }
-  if (secureOptions) this.secureOptions = secureOptions;
+
+  this.secureOptions = secureOptions;
+
   if (options.NPNProtocols) convertNPNProtocols(options.NPNProtocols, this);
   if (options.SNICallback) {
     this.SNICallback = options.SNICallback;
@@ -1326,6 +1331,9 @@ exports.connect = function(/* [port, host], options, cb */) {
   };
   options = util._extend(defaults, options || {});
 
+  options.secureOptions = crypto._getSecureOptions(options.secureProtocol,
+                                                   options.secureOptions);
+
   var socket = options.socket ? options.socket : new net.Stream();
 
   var sslcontext = crypto.createCredentials(options);
diff --git a/test/simple/test-tls-honorcipherorder-secureOptions.js b/test/simple/test-tls-honorcipherorder-secureOptions.js
new file mode 100644
index 000000000..e70cfb1ef
--- /dev/null
+++ b/test/simple/test-tls-honorcipherorder-secureOptions.js
@@ -0,0 +1,131 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+
+var common = require('../common');
+var assert = require('assert');
+var tls = require('tls');
+var fs = require('fs');
+var nconns = 0;
+var SSL_Method = 'SSLv23_method';
+var localhost = '127.0.0.1';
+var opCipher = process.binding('constants').SSL_OP_CIPHER_SERVER_PREFERENCE;
+
+/*
+ * This test is to make sure we are preserving secureOptions that are passed
+ * to the server.
+ *
+ * Also that if honorCipherOrder is passed we are preserving that in the
+ * options.
+ *
+ * And that if we are passing in secureOptions no new options (aside from the
+ * honorCipherOrder case) are added to the secureOptions
+ */
+
+
+process.on('exit', function() {
+  assert.equal(nconns, 6);
+});
+
+function test(honorCipherOrder, clientCipher, expectedCipher, secureOptions, cb) {
+  var soptions = {
+    secureProtocol: SSL_Method,
+    key: fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem'),
+    cert: fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem'),
+    ciphers: 'AES256-SHA:RC4-SHA:DES-CBC-SHA',
+    secureOptions: secureOptions,
+    honorCipherOrder: !!honorCipherOrder
+  };
+
+  var server = tls.createServer(soptions, function(cleartextStream) {
+    nconns++;
+  });
+
+  if (!!honorCipherOrder) {
+    assert.strictEqual(server.secureOptions & opCipher, opCipher, 'we should preserve cipher preference');
+  }
+
+  if (secureOptions) {
+    var expectedSecureOpts = secureOptions;
+    if (!!honorCipherOrder) expectedSecureOpts |= opCipher;
+
+    assert.strictEqual(server.secureOptions & expectedSecureOpts,
+                       expectedSecureOpts, 'we should preserve secureOptions');
+    assert.strictEqual(server.secureOptions & ~expectedSecureOpts,
+                       0,
+                       'we should not add extra options');
+  }
+
+  server.listen(common.PORT, localhost, function() {
+    var coptions = {
+      rejectUnauthorized: false,
+      secureProtocol: SSL_Method
+    };
+    if (clientCipher) {
+      coptions.ciphers = clientCipher;
+    }
+    var client = tls.connect(common.PORT, localhost, coptions, function() {
+      var cipher = client.getCipher();
+      client.end();
+      server.close();
+      assert.equal(cipher.name, expectedCipher);
+      if (cb) cb();
+    });
+  });
+}
+
+test1();
+
+function test1() {
+  // Client has the preference of cipher suites by default
+  test(false, 'DES-CBC-SHA:RC4-SHA:AES256-SHA','DES-CBC-SHA', 0, test2);
+}
+
+function test2() {
+  // Server has the preference of cipher suites where AES256-SHA is in
+  // the first.
+  test(true, 'DES-CBC-SHA:RC4-SHA:AES256-SHA', 'AES256-SHA', 0, test3);
+}
+
+function test3() {
+  // Server has the preference of cipher suites. RC4-SHA is given
+  // higher priority over DES-CBC-SHA among client cipher suites.
+  test(true, 'DES-CBC-SHA:RC4-SHA', 'RC4-SHA', 0, test4);
+}
+
+function test4() {
+  // As client has only one cipher, server has no choice in regardless
+  // of honorCipherOrder.
+  test(true, 'DES-CBC-SHA', 'DES-CBC-SHA', 0, test5);
+}
+
+function test5() {
+  test(false,
+       'DES-CBC-SHA',
+       'DES-CBC-SHA',
+       process.binding('constants').SSL_OP_SINGLE_DH_USE, test6);
+}
+
+function test6() {
+  test(true,
+       'DES-CBC-SHA',
+       'DES-CBC-SHA',
+       process.binding('constants').SSL_OP_SINGLE_DH_USE);
+}