From 262d4b29cba3650ac6d826149e1a7e5e2a2a087f Mon Sep 17 00:00:00 2001 From: Jimmy Cann Date: Mon, 14 Aug 2017 00:24:12 +1000 Subject: [PATCH 1/4] https: disallow boolean types for `key` and `cert` options When using https.createServer, passing boolean values for `key` and `cert` properties in the options object parameter doesn't throw an error an could lead to potential issues if they're accidentally passed. This PR aims to throw a reasonable error if a boolean was passed to either of those properties. Fixes: https://github.com/nodejs/node/issues/12802 --- lib/https.js | 6 ++ .../test-https-options-boolean-check.js | 84 +++++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 test/parallel/test-https-options-boolean-check.js diff --git a/lib/https.js b/lib/https.js index fb220872598315..38ae2fa9b93578 100644 --- a/lib/https.js +++ b/lib/https.js @@ -40,6 +40,12 @@ function Server(opts, requestListener) { } opts = util._extend({}, opts); + if (opts && typeof opts.key === 'boolean') + throw new Error('"options.key" must not be a boolean'); + + if (opts && typeof opts.cert === 'boolean') + throw new Error('"options.cert" must not be a boolean'); + if (process.features.tls_npn && !opts.NPNProtocols) { opts.NPNProtocols = ['http/1.1', 'http/1.0']; } diff --git a/test/parallel/test-https-options-boolean-check.js b/test/parallel/test-https-options-boolean-check.js new file mode 100644 index 00000000000000..123b4635b9aa82 --- /dev/null +++ b/test/parallel/test-https-options-boolean-check.js @@ -0,0 +1,84 @@ +// 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. + +'use strict'; +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const https = require('https'); +const fs = require('fs'); + +assert.doesNotThrow(() => + https.createServer({ + key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), + cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) + })); + +assert.throws(() => + https.createServer({ + key: true, + cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) + }), /"options\.key" must not be a boolean/); + +assert.throws(() => + https.createServer({ + key: false, + cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) + }), /"options\.key" must not be a boolean/); + +assert.throws(() => + https.createServer({ + key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), + cert: true + }), /"options\.cert" must not be a boolean/); + +assert.throws(() => + https.createServer({ + key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), + cert: false + }), /"options\.cert" must not be a boolean/); + +assert.throws(() => + https.createServer({ + key: false, + cert: false + }), /"options\.key" must not be a boolean/); + +assert.throws(() => + https.createServer({ + key: true, + cert: true + }), /"options\.key" must not be a boolean/); + +assert.throws(() => + https.createServer({ + key: true, + cert: false + }), /"options\.key" must not be a boolean/); + +assert.throws(() => + https.createServer({ + key: false, + cert: true + }), /"options\.key" must not be a boolean/); From e92e64ade76c21a7d1cc1b96a68cfbda283c8fde Mon Sep 17 00:00:00 2001 From: Jimmy Cann Date: Mon, 14 Aug 2017 23:19:08 +1000 Subject: [PATCH 2/4] tls: disallow boolean types for `key`, `cert` and `ca` options Taking on board the review from the initial PR, multiple changes have been made. - Type checking now a whitelist as opposed to only checking for a boolean - Type checking moved to the underlying `_tls_common` , `tls` and `https` modules now affected - Arrays are now iterated using `map` instead of `for` for affected sections -Testing added for the `tls` module Fixes: https://github.com/nodejs/node/issues/12802 --- lib/_tls_common.js | 34 +++- lib/https.js | 6 - .../test-https-options-boolean-check.js | 182 +++++++++++------- .../test-tls-options-boolean-check.js | 126 ++++++++++++ 4 files changed, 262 insertions(+), 86 deletions(-) create mode 100644 test/parallel/test-tls-options-boolean-check.js diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 36b2ebdad68d0b..c9616e4ca1e01f 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -22,6 +22,8 @@ 'use strict'; const tls = require('tls'); +const Buffer = require('buffer').Buffer; +const errors = require('internal/errors'); const SSL_OP_CIPHER_SERVER_PREFERENCE = process.binding('constants').crypto.SSL_OP_CIPHER_SERVER_PREFERENCE; @@ -52,6 +54,13 @@ function SecureContext(secureProtocol, secureOptions, context) { if (secureOptions) this.context.setOptions(secureOptions); } +function validateKeyCert(value, type) { + if (typeof value !== 'string' && !(value instanceof Buffer)) + throw new errors.TypeError( + 'ERR_INVALID_ARG_TYPE', type, ['string', 'buffer'] + ); +} + exports.SecureContext = SecureContext; @@ -71,10 +80,12 @@ exports.createSecureContext = function createSecureContext(options, context) { // cert's issuer in C++ code. if (options.ca) { if (Array.isArray(options.ca)) { - for (i = 0; i < options.ca.length; i++) { - c.context.addCACert(options.ca[i]); - } + options.ca.map((ca) => { + validateKeyCert(ca, 'ca'); + c.context.addCACert(ca); + }); } else { + validateKeyCert(options.ca, 'ca'); c.context.addCACert(options.ca); } } else { @@ -83,9 +94,12 @@ exports.createSecureContext = function createSecureContext(options, context) { if (options.cert) { if (Array.isArray(options.cert)) { - for (i = 0; i < options.cert.length; i++) - c.context.setCert(options.cert[i]); + options.cert.map((cert) => { + validateKeyCert(cert, 'cert'); + c.context.setCert(cert); + }); } else { + validateKeyCert(options.cert, 'cert'); c.context.setCert(options.cert); } } @@ -96,12 +110,12 @@ exports.createSecureContext = function createSecureContext(options, context) { // which leads to the crash later on. if (options.key) { if (Array.isArray(options.key)) { - for (i = 0; i < options.key.length; i++) { - const key = options.key[i]; - const passphrase = key.passphrase || options.passphrase; - c.context.setKey(key.pem || key, passphrase); - } + options.key.map((k) => { + validateKeyCert(k.pem || k, 'key'); + c.context.setKey(k.pem || k, k.passphrase || options.passphrase); + }); } else { + validateKeyCert(options.key, 'key'); c.context.setKey(options.key, options.passphrase); } } diff --git a/lib/https.js b/lib/https.js index 38ae2fa9b93578..fb220872598315 100644 --- a/lib/https.js +++ b/lib/https.js @@ -40,12 +40,6 @@ function Server(opts, requestListener) { } opts = util._extend({}, opts); - if (opts && typeof opts.key === 'boolean') - throw new Error('"options.key" must not be a boolean'); - - if (opts && typeof opts.cert === 'boolean') - throw new Error('"options.cert" must not be a boolean'); - if (process.features.tls_npn && !opts.NPNProtocols) { opts.NPNProtocols = ['http/1.1', 'http/1.0']; } diff --git a/test/parallel/test-https-options-boolean-check.js b/test/parallel/test-https-options-boolean-check.js index 123b4635b9aa82..0415f500f2a5ff 100644 --- a/test/parallel/test-https-options-boolean-check.js +++ b/test/parallel/test-https-options-boolean-check.js @@ -1,25 +1,5 @@ -// 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. - 'use strict'; + const common = require('../common'); if (!common.hasCrypto) @@ -29,56 +9,118 @@ const assert = require('assert'); const https = require('https'); const fs = require('fs'); -assert.doesNotThrow(() => - https.createServer({ - key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), - cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) - })); - -assert.throws(() => - https.createServer({ - key: true, - cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) - }), /"options\.key" must not be a boolean/); - -assert.throws(() => - https.createServer({ - key: false, - cert: fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`) - }), /"options\.key" must not be a boolean/); +const keyBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`); +const certBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`); +const keyBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-key.pem`); +const certBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-cert.pem`); +const caCert = fs.readFileSync(`${common.fixturesDir}/keys/ca1-cert.pem`); +const caCert2 = fs.readFileSync(`${common.fixturesDir}/keys/ca2-cert.pem`); +const keyStr = keyBuff.toString(); +const certStr = certBuff.toString(); +const keyStr2 = keyBuff2.toString(); +const certStr2 = certBuff2.toString(); +const caCertStr = caCert.toString(); +const caCertStr2 = caCert2.toString(); +const invalidKeyRE = /^The "key" argument must be one of type string or buffer$/; +const invalidCertRE = /^The "cert" argument must be one of type string or buffer$/; -assert.throws(() => - https.createServer({ - key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), - cert: true - }), /"options\.cert" must not be a boolean/); +// Checks to ensure https.createServer doesn't throw an error +// Format ['key', 'cert'] +[ + [keyBuff, certBuff], + [false, certBuff], + [keyBuff, false], + [keyStr, certStr], + [false, certStr], + [keyStr, false], + [false, false], + [[keyBuff, keyBuff2], [certBuff, certBuff2]], + [[keyStr, keyStr2], [certStr, certStr2]], + [[keyStr, keyStr2], false], + [false, [certStr, certStr2]], + [[{ pem: keyBuff }], false], + [[{ pem: keyBuff }, { pem: keyBuff }], false] +].map((params) => { + assert.doesNotThrow(() => { + https.createServer({ + key: params[0], + cert: params[1] + }); + }); +}); -assert.throws(() => - https.createServer({ - key: fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`), - cert: false - }), /"options\.cert" must not be a boolean/); - -assert.throws(() => - https.createServer({ - key: false, - cert: false - }), /"options\.key" must not be a boolean/); - -assert.throws(() => - https.createServer({ - key: true, - cert: true - }), /"options\.key" must not be a boolean/); +// Checks to ensure https.createServer predictably throws an error +// Format ['key', 'cert', 'expected message'] +[ + [true, certBuff, invalidKeyRE], + [keyBuff, true, invalidCertRE], + [true, certStr, invalidKeyRE], + [keyStr, true, invalidCertRE], + [true, true, invalidCertRE], + [true, false, invalidKeyRE], + [false, true, invalidCertRE], + [true, false, invalidKeyRE], + [{ pem: keyBuff }, false, invalidKeyRE], + [false, { pem: keyBuff }, invalidCertRE], + [1, false, invalidKeyRE], + [false, 1, invalidCertRE], + [[keyBuff, true], [certBuff, certBuff2], invalidKeyRE], + [[true, keyStr2], [certStr, certStr2], invalidKeyRE], + [[keyBuff, keyBuff2], [true, certBuff2], invalidCertRE], + [[keyStr, keyStr2], [certStr, true], invalidCertRE], + [[true, false], [certBuff, certBuff2], invalidKeyRE], + [[keyStr, keyStr2], [true, false], invalidCertRE], + [[keyStr, keyStr2], true, invalidCertRE], + [true, [certBuff, certBuff2], invalidKeyRE] +].map((params) => { + assert.throws(() => { + https.createServer({ + key: params[0], + cert: params[1] + }); + }, common.expectsError({ + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: params[2] + })); +}); -assert.throws(() => - https.createServer({ - key: true, - cert: false - }), /"options\.key" must not be a boolean/); +// Checks to ensure https.createServer works with the CA parameter +// Format ['key', 'cert', 'ca'] +[ + [keyBuff, certBuff, caCert], + [keyBuff, certBuff, [caCert, caCert2]], + [keyBuff, certBuff, caCertStr], + [keyBuff, certBuff, [caCertStr, caCertStr2]], + [keyBuff, certBuff, false], +].map((params) => { + assert.doesNotThrow(() => { + https.createServer({ + key: params[0], + cert: params[1], + ca: params[2] + }); + }); +}); -assert.throws(() => - https.createServer({ - key: false, - cert: true - }), /"options\.key" must not be a boolean/); +// Checks to ensure https.createServer throws an error for CA assignment +// Format ['key', 'cert', 'ca'] +[ + [keyBuff, certBuff, true], + [keyBuff, certBuff, {}], + [keyBuff, certBuff, 1], + [keyBuff, certBuff, true], + [keyBuff, certBuff, [caCert, true]] +].map((params) => { + assert.throws(() => { + https.createServer({ + key: params[0], + cert: params[1], + ca: params[2] + }); + }, common.expectsError({ + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: /^The "ca" argument must be one of type string or buffer$/ + })); +}); diff --git a/test/parallel/test-tls-options-boolean-check.js b/test/parallel/test-tls-options-boolean-check.js new file mode 100644 index 00000000000000..3451d9ce7cc2a7 --- /dev/null +++ b/test/parallel/test-tls-options-boolean-check.js @@ -0,0 +1,126 @@ +'use strict'; + +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const tls = require('tls'); +const fs = require('fs'); + +const keyBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`); +const certBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`); +const keyBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-key.pem`); +const certBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-cert.pem`); +const caCert = fs.readFileSync(`${common.fixturesDir}/keys/ca1-cert.pem`); +const caCert2 = fs.readFileSync(`${common.fixturesDir}/keys/ca2-cert.pem`); +const keyStr = keyBuff.toString(); +const certStr = certBuff.toString(); +const keyStr2 = keyBuff2.toString(); +const certStr2 = certBuff2.toString(); +const caCertStr = caCert.toString(); +const caCertStr2 = caCert2.toString(); +const invalidKeyRE = /^The "key" argument must be one of type string or buffer$/; +const invalidCertRE = /^The "cert" argument must be one of type string or buffer$/; + +// Checks to ensure tls.createServer doesn't throw an error +// Format ['key', 'cert'] +[ + [keyBuff, certBuff], + [false, certBuff], + [keyBuff, false], + [keyStr, certStr], + [false, certStr], + [keyStr, false], + [false, false], + [[keyBuff, keyBuff2], [certBuff, certBuff2]], + [[keyStr, keyStr2], [certStr, certStr2]], + [[keyStr, keyStr2], false], + [false, [certStr, certStr2]], + [[{ pem: keyBuff }], false], + [[{ pem: keyBuff }, { pem: keyBuff }], false] +].map((params) => { + assert.doesNotThrow(() => { + tls.createServer({ + key: params[0], + cert: params[1] + }); + }); +}); + +// Checks to ensure tls.createServer predictably throws an error +// Format ['key', 'cert', 'expected message'] +[ + [true, certBuff, invalidKeyRE], + [keyBuff, true, invalidCertRE], + [true, certStr, invalidKeyRE], + [keyStr, true, invalidCertRE], + [true, true, invalidCertRE], + [true, false, invalidKeyRE], + [false, true, invalidCertRE], + [true, false, invalidKeyRE], + [{ pem: keyBuff }, false, invalidKeyRE], + [false, { pem: keyBuff }, invalidCertRE], + [1, false, invalidKeyRE], + [false, 1, invalidCertRE], + [[keyBuff, true], [certBuff, certBuff2], invalidKeyRE], + [[true, keyStr2], [certStr, certStr2], invalidKeyRE], + [[keyBuff, keyBuff2], [true, certBuff2], invalidCertRE], + [[keyStr, keyStr2], [certStr, true], invalidCertRE], + [[true, false], [certBuff, certBuff2], invalidKeyRE], + [[keyStr, keyStr2], [true, false], invalidCertRE], + [[keyStr, keyStr2], true, invalidCertRE], + [true, [certBuff, certBuff2], invalidKeyRE] +].map((params) => { + assert.throws(() => { + tls.createServer({ + key: params[0], + cert: params[1] + }); + }, common.expectsError({ + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: params[2] + })); +}); + +// Checks to ensure tls.createServer works with the CA parameter +// Format ['key', 'cert', 'ca'] +[ + [keyBuff, certBuff, caCert], + [keyBuff, certBuff, [caCert, caCert2]], + [keyBuff, certBuff, caCertStr], + [keyBuff, certBuff, [caCertStr, caCertStr2]], + [keyBuff, certBuff, false], +].map((params) => { + assert.doesNotThrow(() => { + tls.createServer({ + key: params[0], + cert: params[1], + ca: params[2] + }); + }); +}); + +// Checks to ensure tls.createServer throws an error for CA assignment +// Format ['key', 'cert', 'ca'] +[ + [keyBuff, certBuff, true], + [keyBuff, certBuff, {}], + [keyBuff, certBuff, 1], + [keyBuff, certBuff, true], + [keyBuff, certBuff, [caCert, true]] +].map((params) => { + assert.throws(() => { + tls.createServer({ + key: params[0], + cert: params[1], + ca: params[2] + }); + }, common.expectsError({ + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError, + message: /^The "ca" argument must be one of type string or buffer$/ + })); +}); From b132c96673b37f6b3e3baf4c8a6c7eeb49106f9a Mon Sep 17 00:00:00 2001 From: Jimmy Cann Date: Wed, 16 Aug 2017 21:26:31 +1000 Subject: [PATCH 3/4] tls: type checking for `key`, `cert` and `ca` options Additional changes in line with PR review - Loosen type checking for buffers using the ArrayBuffer method - Require pem files using updated fixture access method - Add tests for ArrayBuffer and DataView types Fixes: #12802 --- lib/_tls_common.js | 6 +-- .../test-https-options-boolean-check.js | 50 +++++++++++++++---- .../test-tls-options-boolean-check.js | 50 +++++++++++++++---- 3 files changed, 83 insertions(+), 23 deletions(-) diff --git a/lib/_tls_common.js b/lib/_tls_common.js index c9616e4ca1e01f..a680fe172edc27 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -22,7 +22,6 @@ 'use strict'; const tls = require('tls'); -const Buffer = require('buffer').Buffer; const errors = require('internal/errors'); const SSL_OP_CIPHER_SERVER_PREFERENCE = @@ -55,9 +54,10 @@ function SecureContext(secureProtocol, secureOptions, context) { } function validateKeyCert(value, type) { - if (typeof value !== 'string' && !(value instanceof Buffer)) + if (typeof value !== 'string' && !ArrayBuffer.isView(value)) throw new errors.TypeError( - 'ERR_INVALID_ARG_TYPE', type, ['string', 'buffer'] + 'ERR_INVALID_ARG_TYPE', type, + ['string', 'Buffer', 'TypedArray', 'DataView'] ); } diff --git a/test/parallel/test-https-options-boolean-check.js b/test/parallel/test-https-options-boolean-check.js index 0415f500f2a5ff..eae319988e7c6a 100644 --- a/test/parallel/test-https-options-boolean-check.js +++ b/test/parallel/test-https-options-boolean-check.js @@ -1,28 +1,46 @@ 'use strict'; const common = require('../common'); +const fixtures = require('../common/fixtures'); if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const https = require('https'); -const fs = require('fs'); -const keyBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`); -const certBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`); -const keyBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-key.pem`); -const certBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-cert.pem`); -const caCert = fs.readFileSync(`${common.fixturesDir}/keys/ca1-cert.pem`); -const caCert2 = fs.readFileSync(`${common.fixturesDir}/keys/ca2-cert.pem`); +function toArrayBuffer(buf) { + const ab = new ArrayBuffer(buf.length); + const view = new Uint8Array(ab); + return buf.map((b, i) => view[i] = b); +} + +function toDataView(buf) { + const ab = new ArrayBuffer(buf.length); + const view = new DataView(ab); + return buf.map((b, i) => view[i] = b); +} + +const keyBuff = fixtures.readKey('agent1-key.pem'); +const certBuff = fixtures.readKey('agent1-cert.pem'); +const keyBuff2 = fixtures.readKey('ec-key.pem'); +const certBuff2 = fixtures.readKey('ec-cert.pem'); +const caCert = fixtures.readKey('ca1-cert.pem'); +const caCert2 = fixtures.readKey('ca2-cert.pem'); const keyStr = keyBuff.toString(); const certStr = certBuff.toString(); const keyStr2 = keyBuff2.toString(); const certStr2 = certBuff2.toString(); const caCertStr = caCert.toString(); const caCertStr2 = caCert2.toString(); -const invalidKeyRE = /^The "key" argument must be one of type string or buffer$/; -const invalidCertRE = /^The "cert" argument must be one of type string or buffer$/; +const keyArrBuff = toArrayBuffer(keyBuff); +const certArrBuff = toArrayBuffer(certBuff); +const caArrBuff = toArrayBuffer(caCert); +const keyDataView = toDataView(keyBuff); +const certDataView = toDataView(certBuff); +const caArrDataView = toDataView(caCert); +const invalidKeyRE = /^The "key" argument must be one of type string, Buffer, TypedArray, or DataView$/; +const invalidCertRE = /^The "cert" argument must be one of type string, Buffer, TypedArray, or DataView$/; // Checks to ensure https.createServer doesn't throw an error // Format ['key', 'cert'] @@ -34,6 +52,12 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer [false, certStr], [keyStr, false], [false, false], + [keyArrBuff, certArrBuff], + [keyArrBuff, false], + [false, certArrBuff], + [keyDataView, certDataView], + [keyDataView, false], + [false, certDataView], [[keyBuff, keyBuff2], [certBuff, certBuff2]], [[keyStr, keyStr2], [certStr, certStr2]], [[keyStr, keyStr2], false], @@ -56,6 +80,10 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer [keyBuff, true, invalidCertRE], [true, certStr, invalidKeyRE], [keyStr, true, invalidCertRE], + [true, certArrBuff, invalidKeyRE], + [keyArrBuff, true, invalidCertRE], + [true, certDataView, invalidKeyRE], + [keyDataView, true, invalidCertRE], [true, true, invalidCertRE], [true, false, invalidKeyRE], [false, true, invalidCertRE], @@ -92,6 +120,8 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer [keyBuff, certBuff, [caCert, caCert2]], [keyBuff, certBuff, caCertStr], [keyBuff, certBuff, [caCertStr, caCertStr2]], + [keyBuff, certBuff, caArrBuff], + [keyBuff, certBuff, caArrDataView], [keyBuff, certBuff, false], ].map((params) => { assert.doesNotThrow(() => { @@ -121,6 +151,6 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer }, common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError, - message: /^The "ca" argument must be one of type string or buffer$/ + message: /^The "ca" argument must be one of type string, Buffer, TypedArray, or DataView$/ })); }); diff --git a/test/parallel/test-tls-options-boolean-check.js b/test/parallel/test-tls-options-boolean-check.js index 3451d9ce7cc2a7..4cc0aaf3771bf0 100644 --- a/test/parallel/test-tls-options-boolean-check.js +++ b/test/parallel/test-tls-options-boolean-check.js @@ -1,28 +1,46 @@ 'use strict'; const common = require('../common'); +const fixtures = require('../common/fixtures'); if (!common.hasCrypto) common.skip('missing crypto'); const assert = require('assert'); const tls = require('tls'); -const fs = require('fs'); -const keyBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`); -const certBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-cert.pem`); -const keyBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-key.pem`); -const certBuff2 = fs.readFileSync(`${common.fixturesDir}/keys/ec-cert.pem`); -const caCert = fs.readFileSync(`${common.fixturesDir}/keys/ca1-cert.pem`); -const caCert2 = fs.readFileSync(`${common.fixturesDir}/keys/ca2-cert.pem`); +function toArrayBuffer(buf) { + const ab = new ArrayBuffer(buf.length); + const view = new Uint8Array(ab); + return buf.map((b, i) => view[i] = b); +} + +function toDataView(buf) { + const ab = new ArrayBuffer(buf.length); + const view = new DataView(ab); + return buf.map((b, i) => view[i] = b); +} + +const keyBuff = fixtures.readKey('agent1-key.pem'); +const certBuff = fixtures.readKey('agent1-cert.pem'); +const keyBuff2 = fixtures.readKey('ec-key.pem'); +const certBuff2 = fixtures.readKey('ec-cert.pem'); +const caCert = fixtures.readKey('ca1-cert.pem'); +const caCert2 = fixtures.readKey('ca2-cert.pem'); const keyStr = keyBuff.toString(); const certStr = certBuff.toString(); const keyStr2 = keyBuff2.toString(); const certStr2 = certBuff2.toString(); const caCertStr = caCert.toString(); const caCertStr2 = caCert2.toString(); -const invalidKeyRE = /^The "key" argument must be one of type string or buffer$/; -const invalidCertRE = /^The "cert" argument must be one of type string or buffer$/; +const keyArrBuff = toArrayBuffer(keyBuff); +const certArrBuff = toArrayBuffer(certBuff); +const caArrBuff = toArrayBuffer(caCert); +const keyDataView = toDataView(keyBuff); +const certDataView = toDataView(certBuff); +const caArrDataView = toDataView(caCert); +const invalidKeyRE = /^The "key" argument must be one of type string, Buffer, TypedArray, or DataView$/; +const invalidCertRE = /^The "cert" argument must be one of type string, Buffer, TypedArray, or DataView$/; // Checks to ensure tls.createServer doesn't throw an error // Format ['key', 'cert'] @@ -34,6 +52,12 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer [false, certStr], [keyStr, false], [false, false], + [keyArrBuff, certArrBuff], + [keyArrBuff, false], + [false, certArrBuff], + [keyDataView, certDataView], + [keyDataView, false], + [false, certDataView], [[keyBuff, keyBuff2], [certBuff, certBuff2]], [[keyStr, keyStr2], [certStr, certStr2]], [[keyStr, keyStr2], false], @@ -56,6 +80,10 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer [keyBuff, true, invalidCertRE], [true, certStr, invalidKeyRE], [keyStr, true, invalidCertRE], + [true, certArrBuff, invalidKeyRE], + [keyArrBuff, true, invalidCertRE], + [true, certDataView, invalidKeyRE], + [keyDataView, true, invalidCertRE], [true, true, invalidCertRE], [true, false, invalidKeyRE], [false, true, invalidCertRE], @@ -92,6 +120,8 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer [keyBuff, certBuff, [caCert, caCert2]], [keyBuff, certBuff, caCertStr], [keyBuff, certBuff, [caCertStr, caCertStr2]], + [keyBuff, certBuff, caArrBuff], + [keyBuff, certBuff, caArrDataView], [keyBuff, certBuff, false], ].map((params) => { assert.doesNotThrow(() => { @@ -121,6 +151,6 @@ const invalidCertRE = /^The "cert" argument must be one of type string or buffer }, common.expectsError({ code: 'ERR_INVALID_ARG_TYPE', type: TypeError, - message: /^The "ca" argument must be one of type string or buffer$/ + message: /^The "ca" argument must be one of type string, Buffer, TypedArray, or DataView$/ })); }); From c46fdd41dd02bd14ee84d313db6c0b71402c1354 Mon Sep 17 00:00:00 2001 From: Jimmy Cann Date: Thu, 17 Aug 2017 07:22:07 +1000 Subject: [PATCH 4/4] tls: type checking for `key`, `cert` and `ca` options - Change iteration method from map -> forEach Fixes: #12802 --- lib/_tls_common.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/_tls_common.js b/lib/_tls_common.js index a680fe172edc27..d2de21dd065555 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -80,7 +80,7 @@ exports.createSecureContext = function createSecureContext(options, context) { // cert's issuer in C++ code. if (options.ca) { if (Array.isArray(options.ca)) { - options.ca.map((ca) => { + options.ca.forEach((ca) => { validateKeyCert(ca, 'ca'); c.context.addCACert(ca); }); @@ -94,7 +94,7 @@ exports.createSecureContext = function createSecureContext(options, context) { if (options.cert) { if (Array.isArray(options.cert)) { - options.cert.map((cert) => { + options.cert.forEach((cert) => { validateKeyCert(cert, 'cert'); c.context.setCert(cert); }); @@ -110,7 +110,7 @@ exports.createSecureContext = function createSecureContext(options, context) { // which leads to the crash later on. if (options.key) { if (Array.isArray(options.key)) { - options.key.map((k) => { + options.key.forEach((k) => { validateKeyCert(k.pem || k, 'key'); c.context.setKey(k.pem || k, k.passphrase || options.passphrase); });