Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

https: disallow boolean types for key and cert options #14807

Closed
wants to merge 4 commits into from
Closed

https: disallow boolean types for key and cert options #14807

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
262d4b2
https: disallow boolean types for `key` and `cert` options
jimmycann Aug 13, 2017
e92e64a
tls: disallow boolean types for `key`, `cert` and `ca` options
jimmycann Aug 14, 2017
b132c96
tls: type checking for `key`, `cert` and `ca` options
jimmycann Aug 16, 2017
c46fdd4
tls: type checking for `key`, `cert` and `ca` options
jimmycann Aug 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions lib/_tls_common.js
View file
Open in desktop
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))
Copy link
Member

Choose a reason for hiding this comment

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

I think this is too strict, and instanceof Buffer is not a foolproof way to check for Buffers. Can you use ArrayBuffer.isView(value) instead and adjust the message accordingly (Buffers are just special Uint8Arrays, but all Uint8Arrays and other ArrayBufferViews should work, I think.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, brilliant! Replacing the two check works perfectly. I'll change this over and add some additional tests.

Regarding the new error message, it is currently ['string', 'buffer'] and I'm thinking it should change to ['string', 'ArrayBuffer'] with perhaps string => String. Would you have an opinion on how that should look?

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think we can list ArrayBuffer, as those are not ArrayBufferViews themselves.

Maybe ['string', 'Buffer', 'TypedArray', 'DataView']? That would match the message here:

node/lib/zlib.js

Line 231 in cde272a

'Invalid dictionary: it should be a Buffer, TypedArray, or DataView');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect! Thank you, I'll create a commit for this change.

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);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This map can be changed to a forEach because you're not using the return value.

} 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);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

} 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);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

And here.

} else {
validateKeyCert(options.key, 'key');
c.context.setKey(options.key, options.passphrase);
}
}
Expand Down
126 changes: 126 additions & 0 deletions test/parallel/test-https-options-boolean-check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
'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');

const keyBuff = fs.readFileSync(`${common.fixturesDir}/keys/agent1-key.pem`);
Copy link
Member

Choose a reason for hiding this comment

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

There's been some recent work to simplify fixtures access...

example...

const fixtures = require('../common/fixtures');

const keyBuff = fixtures.readKey('agent1-key.pem');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah OK, cool. Thanks, I'll change that over too. The original test file was copied over from another and amended, hence the previous inclusion of the license comments and usage of the fs.readFileSync method of including the pem files. Is there are issue/task that you're aware of to update the older tests to use the newer methods? Something like that I feel like would be a useful task for me to take up in future.

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 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]
});
});
});

// 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]
}));
});

// 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]
});
});
});

// 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$/
}));
});
126 changes: 126 additions & 0 deletions test/parallel/test-tls-options-boolean-check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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$/
}));
});