Skip to content

Commit

Permalink
tls: disallow boolean types for key, cert and ca options
Browse files Browse the repository at this point in the history 
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: #12802
  • Loading branch information
@jimmycann
jimmycann committed Aug 14, 2017
1 parent 262d4b2 commit e92e64a
Show file tree
Hide file tree
Showing 4 changed files with 262 additions and 86 deletions.
34 changes: 24 additions & 10 deletions lib/_tls_common.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;


Expand All @@ -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 {
Expand All @@ -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);
}
}
Expand All @@ -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);
}
}
Expand Down
6 changes: 0 additions & 6 deletions lib/https.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'];
}
Expand Down
182 changes: 112 additions & 70 deletions test/parallel/test-https-options-boolean-check.js
View file
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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$/
}));
});
Loading

0 comments on commit e92e64a

Please sign in to comment.