Skip to content

Commit

Permalink
tls: fix object prototype type confusion
Browse files Browse the repository at this point in the history
Use `Object.create(null)` for dictionary objects so that keys from
certificate strings or the authorityInfoAccess field cannot conflict
with Object.prototype properties.

PR-URL: nodejs/node#14447
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  • Loading branch information
bnoordhuis authored and addaleax committed Sep 5, 2017
1 parent d97d5cb commit 12cd6b5
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 15 deletions.
7 changes: 2 additions & 5 deletions lib/_tls_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,11 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {
if (c.subject != null) c.subject = tls.parseCertString(c.subject);
if (c.infoAccess != null) {
var info = c.infoAccess;
c.infoAccess = {};
c.infoAccess = Object.create(null);

// XXX: More key validation?
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, function(all, key, val) {
if (key === '__proto__')
return;

if (c.infoAccess.hasOwnProperty(key))
if (key in c.infoAccess)
c.infoAccess[key].push(val);
else
c.infoAccess[key] = [val];
Expand Down
2 changes: 1 addition & 1 deletion lib/tls.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
// Example:
// C=US\nST=CA\nL=SF\nO=Joyent\nOU=Node.js\nCN=ca1\[email protected]
exports.parseCertString = function parseCertString(s) {
var out = {};
var out = Object.create(null);
var parts = s.split('\n');
for (var i = 0, len = parts.length; i < len; i++) {
var sepIndex = parts[i].indexOf('=');
Expand Down
13 changes: 12 additions & 1 deletion test/parallel/test-tls-parse-cert-string.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable no-proto */
'use strict';
const common = require('../common');
if (!common.hasCrypto)
Expand All @@ -11,6 +12,7 @@ const tls = require('tls');
'CN=ca1\[email protected]';
const singlesOut = tls.parseCertString(singles);
assert.deepStrictEqual(singlesOut, {
__proto__: null,
C: 'US',
ST: 'CA',
L: 'SF',
Expand All @@ -26,6 +28,7 @@ const tls = require('tls');
'CN=*.nodejs.org';
const doublesOut = tls.parseCertString(doubles);
assert.deepStrictEqual(doublesOut, {
__proto__: null,
OU: [ 'Domain Control Validated', 'PositiveSSL Wildcard' ],
CN: '*.nodejs.org'
});
Expand All @@ -34,5 +37,13 @@ const tls = require('tls');
{
const invalid = 'fhqwhgads';
const invalidOut = tls.parseCertString(invalid);
assert.deepStrictEqual(invalidOut, {});
assert.deepStrictEqual(invalidOut, { __proto__: null });
}

{
const input = '__proto__=mostly harmless\nhasOwnProperty=not a function';
const expected = Object.create(null);
expected.__proto__ = 'mostly harmless';
expected.hasOwnProperty = 'not a function';
assert.deepStrictEqual(tls.parseCertString(input), expected);
}
30 changes: 22 additions & 8 deletions test/parallel/test-tls-translate-peer-certificate.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable no-proto */
'use strict';
const common = require('../common');

Expand All @@ -7,8 +8,12 @@ if (!common.hasCrypto)
const { strictEqual, deepStrictEqual } = require('assert');
const { translatePeerCertificate } = require('_tls_common');

const certString = 'A=1\nB=2\nC=3';
const certObject = { A: '1', B: '2', C: '3' };
const certString = '__proto__=42\nA=1\nB=2\nC=3';
const certObject = Object.create(null);
certObject.__proto__ = '42';
certObject.A = '1';
certObject.B = '2';
certObject.C = '3';

strictEqual(translatePeerCertificate(null), null);
strictEqual(translatePeerCertificate(undefined), null);
Expand All @@ -19,14 +24,14 @@ strictEqual(translatePeerCertificate(1), 1);
deepStrictEqual(translatePeerCertificate({}), {});

deepStrictEqual(translatePeerCertificate({ issuer: '' }),
{ issuer: {} });
{ issuer: Object.create(null) });
deepStrictEqual(translatePeerCertificate({ issuer: null }),
{ issuer: null });
deepStrictEqual(translatePeerCertificate({ issuer: certString }),
{ issuer: certObject });

deepStrictEqual(translatePeerCertificate({ subject: '' }),
{ subject: {} });
{ subject: Object.create(null) });
deepStrictEqual(translatePeerCertificate({ subject: null }),
{ subject: null });
deepStrictEqual(translatePeerCertificate({ subject: certString }),
Expand All @@ -47,9 +52,18 @@ deepStrictEqual(
}

deepStrictEqual(translatePeerCertificate({ infoAccess: '' }),
{ infoAccess: {} });
{ infoAccess: Object.create(null) });
deepStrictEqual(translatePeerCertificate({ infoAccess: null }),
{ infoAccess: null });
deepStrictEqual(
translatePeerCertificate({ infoAccess: 'OCSP - URI:file:///etc/passwd' }),
{ infoAccess: { 'OCSP - URI': ['file:///etc/passwd'] } });
{
const input =
'__proto__:mostly harmless\n' +
'hasOwnProperty:not a function\n' +
'OCSP - URI:file:///etc/passwd\n';
const expected = Object.create(null);
expected.__proto__ = ['mostly harmless'];
expected.hasOwnProperty = ['not a function'];
expected['OCSP - URI'] = ['file:///etc/passwd'];
deepStrictEqual(translatePeerCertificate({ infoAccess: input }),
{ infoAccess: expected });
}

0 comments on commit 12cd6b5

Please sign in to comment.