Skip to content

Commit

Permalink
crypto,tls: fix mutability of return values
Browse files Browse the repository at this point in the history
If you alter the array returned by `tls.getCiphers()`,
`crypto.getCiphers()`, `crypto.getHashes()`, or `crypto.getCurves()`, it
will alter subsequent return values from those functions.

```js
'use strict';

const crypto = require('crypto');

var hashes = crypto.getHashes();

hashes.splice(0, hashes.length);

hashes.push('some-arbitrary-value');

console.log(crypto.getHashes()); // "['some-arbitrary-value']"
```

This is surprising. Change functions to return copy of array instead.

PR-URL: #10795
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
  • Loading branch information
Trott authored and MylesBorins committed Mar 8, 2017
1 parent aa5cf7c commit a3ada12
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 4 deletions.
2 changes: 1 addition & 1 deletion lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ exports.cachedResult = function cachedResult(fn) {
return () => {
if (result === undefined)
result = fn();
return result;
return result.slice();
};
};

Expand Down
6 changes: 3 additions & 3 deletions lib/tls.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ exports.DEFAULT_CIPHERS =

exports.DEFAULT_ECDH_CURVE = 'prime256v1';

exports.getCiphers = internalUtil.cachedResult(() => {
return internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), true);
});
exports.getCiphers = internalUtil.cachedResult(
() => internalUtil.filterDuplicateStrings(binding.getSSLCiphers(), true)
);

// Convert protocols array into valid OpenSSL protocols list
// ("\x06spdy/2\x08http/1.1\x08http/1.0")
Expand Down
14 changes: 14 additions & 0 deletions test/parallel/test-crypto.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,20 @@ assert(crypto.getCurves().includes('secp384r1'));
assert(!crypto.getCurves().includes('SECP384R1'));
validateList(crypto.getCurves());

// Modifying return value from get* functions should not mutate subsequent
// return values.
function testImmutability(fn) {
const list = fn();
const copy = [...list];
list.push('some-arbitrary-value');
assert.deepStrictEqual(fn(), copy);
}

testImmutability(crypto.getCiphers);
testImmutability(tls.getCiphers);
testImmutability(crypto.getHashes);
testImmutability(crypto.getCurves);

// Regression tests for #5725: hex input that's not a power of two should
// throw, not assert in C++ land.
assert.throws(function() {
Expand Down

0 comments on commit a3ada12

Please sign in to comment.