Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Commit b9283cf

Browse files
committed
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
1 parent fe2e8a4 commit b9283cf

File tree

3 files changed

+167
-20
lines changed

3 files changed

+167
-20
lines changed

lib/crypto.js

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,31 @@ var StringDecoder = require('string_decoder').StringDecoder;
6161

6262
var CONTEXT_DEFAULT_OPTIONS = undefined;
6363

64+
function getSecureOptions(secureProtocol, secureOptions) {
65+
if (CONTEXT_DEFAULT_OPTIONS === undefined) {
66+
CONTEXT_DEFAULT_OPTIONS = 0;
67+
68+
if (!binding.SSL3_ENABLE)
69+
CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv3;
70+
71+
if (!binding.SSL2_ENABLE)
72+
CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv2;
73+
}
74+
75+
if (secureOptions === undefined) {
76+
if (secureProtocol === undefined ||
77+
secureProtocol === 'SSLv23_method' ||
78+
secureProtocol === 'SSLv23_server_method' ||
79+
secureProtocol === 'SSLv23_client_method') {
80+
secureOptions |= CONTEXT_DEFAULT_OPTIONS;
81+
}
82+
}
83+
84+
return secureOptions;
85+
}
86+
exports._getSecureOptions = getSecureOptions;
87+
88+
6489
function Credentials(secureProtocol, flags, context) {
6590
if (!(this instanceof Credentials)) {
6691
return new Credentials(secureProtocol, flags, context);
@@ -82,24 +107,7 @@ function Credentials(secureProtocol, flags, context) {
82107
}
83108
}
84109

85-
if (CONTEXT_DEFAULT_OPTIONS === undefined) {
86-
CONTEXT_DEFAULT_OPTIONS = 0;
87-
88-
if (!binding.SSL3_ENABLE)
89-
CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv3;
90-
91-
if (!binding.SSL2_ENABLE)
92-
CONTEXT_DEFAULT_OPTIONS |= constants.SSL_OP_NO_SSLv2;
93-
}
94-
95-
if (flags === undefined) {
96-
if (secureProtocol === undefined ||
97-
secureProtocol === 'SSLv23_method' ||
98-
secureProtocol === 'SSLv23_server_method' ||
99-
secureProtocol === 'SSLv23_client_method') {
100-
flags |= CONTEXT_DEFAULT_OPTIONS;
101-
}
102-
}
110+
flags = getSecureOptions(secureProtocol, flags);
103111

104112
this.context.setOptions(flags);
105113
}

lib/tls.js

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1239,11 +1239,16 @@ Server.prototype.setOptions = function(options) {
12391239
if (options.secureProtocol) this.secureProtocol = options.secureProtocol;
12401240
if (options.crl) this.crl = options.crl;
12411241
if (options.ciphers) this.ciphers = options.ciphers;
1242-
var secureOptions = options.secureOptions || 0;
1242+
1243+
var secureOptions = crypto._getSecureOptions(options.secureProtocol,
1244+
options.secureOptions);
1245+
12431246
if (options.honorCipherOrder) {
12441247
secureOptions |= constants.SSL_OP_CIPHER_SERVER_PREFERENCE;
12451248
}
1246-
if (secureOptions) this.secureOptions = secureOptions;
1249+
1250+
this.secureOptions = secureOptions;
1251+
12471252
if (options.NPNProtocols) convertNPNProtocols(options.NPNProtocols, this);
12481253
if (options.SNICallback) {
12491254
this.SNICallback = options.SNICallback;
@@ -1326,6 +1331,9 @@ exports.connect = function(/* [port, host], options, cb */) {
13261331
};
13271332
options = util._extend(defaults, options || {});
13281333

1334+
options.secureOptions = crypto._getSecureOptions(options.secureProtocol,
1335+
options.secureOptions);
1336+
13291337
var socket = options.socket ? options.socket : new net.Stream();
13301338

13311339
var sslcontext = crypto.createCredentials(options);
Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
// Copyright Joyent, Inc. and other Node contributors.
2+
//
3+
// Permission is hereby granted, free of charge, to any person obtaining a
4+
// copy of this software and associated documentation files (the
5+
// "Software"), to deal in the Software without restriction, including
6+
// without limitation the rights to use, copy, modify, merge, publish,
7+
// distribute, sublicense, and/or sell copies of the Software, and to permit
8+
// persons to whom the Software is furnished to do so, subject to the
9+
// following conditions:
10+
//
11+
// The above copyright notice and this permission notice shall be included
12+
// in all copies or substantial portions of the Software.
13+
//
14+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
15+
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
16+
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
17+
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
18+
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
19+
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
20+
// USE OR OTHER DEALINGS IN THE SOFTWARE.
21+
22+
var common = require('../common');
23+
var assert = require('assert');
24+
var tls = require('tls');
25+
var fs = require('fs');
26+
var nconns = 0;
27+
var SSL_Method = 'SSLv23_method';
28+
var localhost = '127.0.0.1';
29+
var opCipher = process.binding('constants').SSL_OP_CIPHER_SERVER_PREFERENCE;
30+
31+
/*
32+
* This test is to make sure we are preserving secureOptions that are passed
33+
* to the server.
34+
*
35+
* Also that if honorCipherOrder is passed we are preserving that in the
36+
* options.
37+
*
38+
* And that if we are passing in secureOptions no new options (aside from the
39+
* honorCipherOrder case) are added to the secureOptions
40+
*/
41+
42+
43+
process.on('exit', function() {
44+
assert.equal(nconns, 6);
45+
});
46+
47+
function test(honorCipherOrder, clientCipher, expectedCipher, secureOptions, cb) {
48+
var soptions = {
49+
secureProtocol: SSL_Method,
50+
key: fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem'),
51+
cert: fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem'),
52+
ciphers: 'AES256-SHA:RC4-SHA:DES-CBC-SHA',
53+
secureOptions: secureOptions,
54+
honorCipherOrder: !!honorCipherOrder
55+
};
56+
57+
var server = tls.createServer(soptions, function(cleartextStream) {
58+
nconns++;
59+
});
60+
61+
if (!!honorCipherOrder) {
62+
assert.strictEqual(server.secureOptions & opCipher, opCipher, 'we should preserve cipher preference');
63+
}
64+
65+
if (secureOptions) {
66+
var expectedSecureOpts = secureOptions;
67+
if (!!honorCipherOrder) expectedSecureOpts |= opCipher;
68+
69+
assert.strictEqual(server.secureOptions & expectedSecureOpts,
70+
expectedSecureOpts, 'we should preserve secureOptions');
71+
assert.strictEqual(server.secureOptions & ~expectedSecureOpts,
72+
0,
73+
'we should not add extra options');
74+
}
75+
76+
server.listen(common.PORT, localhost, function() {
77+
var coptions = {
78+
rejectUnauthorized: false,
79+
secureProtocol: SSL_Method
80+
};
81+
if (clientCipher) {
82+
coptions.ciphers = clientCipher;
83+
}
84+
var client = tls.connect(common.PORT, localhost, coptions, function() {
85+
var cipher = client.getCipher();
86+
client.end();
87+
server.close();
88+
assert.equal(cipher.name, expectedCipher);
89+
if (cb) cb();
90+
});
91+
});
92+
}
93+
94+
test1();
95+
96+
function test1() {
97+
// Client has the preference of cipher suites by default
98+
test(false, 'DES-CBC-SHA:RC4-SHA:AES256-SHA','DES-CBC-SHA', 0, test2);
99+
}
100+
101+
function test2() {
102+
// Server has the preference of cipher suites where AES256-SHA is in
103+
// the first.
104+
test(true, 'DES-CBC-SHA:RC4-SHA:AES256-SHA', 'AES256-SHA', 0, test3);
105+
}
106+
107+
function test3() {
108+
// Server has the preference of cipher suites. RC4-SHA is given
109+
// higher priority over DES-CBC-SHA among client cipher suites.
110+
test(true, 'DES-CBC-SHA:RC4-SHA', 'RC4-SHA', 0, test4);
111+
}
112+
113+
function test4() {
114+
// As client has only one cipher, server has no choice in regardless
115+
// of honorCipherOrder.
116+
test(true, 'DES-CBC-SHA', 'DES-CBC-SHA', 0, test5);
117+
}
118+
119+
function test5() {
120+
test(false,
121+
'DES-CBC-SHA',
122+
'DES-CBC-SHA',
123+
process.binding('constants').SSL_OP_SINGLE_DH_USE, test6);
124+
}
125+
126+
function test6() {
127+
test(true,
128+
'DES-CBC-SHA',
129+
'DES-CBC-SHA',
130+
process.binding('constants').SSL_OP_SINGLE_DH_USE);
131+
}

0 commit comments

Comments
 (0)