Skip to content

Commit

Permalink
tls: type checking for key, cert and ca options
Browse files Browse the repository at this point in the history 
PR-URL: #14807
Fixes: #12802
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  • Loading branch information
@jimmycann @BridgeAR
jimmycann authored and BridgeAR committed Aug 27, 2017
1 parent 0097794 commit a7dccd0
Show file tree
Hide file tree
Showing 3 changed files with 336 additions and 10 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,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;
Expand Down Expand Up @@ -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;


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.forEach((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.forEach((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.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);
}
}
Expand Down
156 changes: 156 additions & 0 deletions test/parallel/test-https-options-boolean-check.js
View file
Original file line number Diff line number Diff line change
@@ -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$/
}));
});
156 changes: 156 additions & 0 deletions test/parallel/test-tls-options-boolean-check.js
View file
Original file line number Diff line number Diff line change
@@ -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$/
}));
});

3 comments on commit a7dccd0

@Trott
Copy link
Member

@Trott Trott commented on a7dccd0 Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but FYI the first line of the commit message is longer than 50 chars. Nothing worth fixing now, but for the future, be aware. Thanks!

@BridgeAR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for notifying me. I am going to add a commit-hook to prevent that from happening again.

@refack
Copy link
Contributor

@refack refack commented on a7dccd0 Aug 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my push hook I run core-validate-commit if push target == 'upstream'

Please sign in to comment.