From a7dccd040d72ce7de61d9160ec031420c52a49d4 Mon Sep 17 00:00:00 2001 From: Jimmy Cann Date: Mon, 14 Aug 2017 00:24:12 +1000 Subject: [PATCH] tls: type checking for `key`, `cert` and `ca` options PR-URL: https://github.com/nodejs/node/pull/14807 Fixes: https://github.com/nodejs/node/issues/12802 Reviewed-By: Colin Ihrig Reviewed-By: Roman Reiss Reviewed-By: Anna Henningsen Reviewed-By: Ruben Bridgewater --- lib/_tls_common.js | 34 ++-- .../test-https-options-boolean-check.js | 156 ++++++++++++++++++ .../test-tls-options-boolean-check.js | 156 ++++++++++++++++++ 3 files changed, 336 insertions(+), 10 deletions(-) create mode 100644 test/parallel/test-https-options-boolean-check.js create mode 100644 test/parallel/test-tls-options-boolean-check.js diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 36b2ebdad6..d2de21dd06 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -22,6 +22,7 @@ 'use strict'; const tls = require('tls'); +const errors = require('internal/errors'); const SSL_OP_CIPHER_SERVER_PREFERENCE = process.binding('constants').crypto.SSL_OP_CIPHER_SERVER_PREFERENCE; @@ -52,6 +53,14 @@ function SecureContext(secureProtocol, secureOptions, context) { if (secureOptions) this.context.setOptions(secureOptions); } +function validateKeyCert(value, type) { + if (typeof value !== 'string' && !ArrayBuffer.isView(value)) + throw new errors.TypeError( + 'ERR_INVALID_ARG_TYPE', type, + ['string', 'Buffer', 'TypedArray', 'DataView'] + ); +} + 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.forEach((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.forEach((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.forEach((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/test/parallel/test-https-options-boolean-check.js b/test/parallel/test-https-options-boolean-check.js new file mode 100644 index 0000000000..eae319988e --- /dev/null +++ b/test/parallel/test-https-options-boolean-check.js @@ -0,0 +1,156 @@ +'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'); + +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 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'] +[ + [keyBuff, certBuff], + [false, certBuff], + [keyBuff, false], + [keyStr, certStr], + [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], + [false, [certStr, certStr2]], + [[{ pem: keyBuff }], false], + [[{ pem: keyBuff }, { pem: keyBuff }], false] +].map((params) => { + assert.doesNotThrow(() => { + https.createServer({ + key: params[0], + cert: params[1] + }); + }); +}); + +// 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, certArrBuff, invalidKeyRE], + [keyArrBuff, true, invalidCertRE], + [true, certDataView, invalidKeyRE], + [keyDataView, 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] + })); +}); + +// 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, caArrBuff], + [keyBuff, certBuff, caArrDataView], + [keyBuff, certBuff, false], +].map((params) => { + assert.doesNotThrow(() => { + https.createServer({ + key: params[0], + cert: params[1], + ca: params[2] + }); + }); +}); + +// 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, Buffer, TypedArray, or DataView$/ + })); +}); 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 0000000000..4cc0aaf377 --- /dev/null +++ b/test/parallel/test-tls-options-boolean-check.js @@ -0,0 +1,156 @@ +'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'); + +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 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'] +[ + [keyBuff, certBuff], + [false, certBuff], + [keyBuff, false], + [keyStr, certStr], + [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], + [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, certArrBuff, invalidKeyRE], + [keyArrBuff, true, invalidCertRE], + [true, certDataView, invalidKeyRE], + [keyDataView, 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, caArrBuff], + [keyBuff, certBuff, caArrDataView], + [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, Buffer, TypedArray, or DataView$/ + })); +});