From 12cd6b51fae214b45bccab0f745273e2019189f6 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 24 Jul 2017 14:36:36 +0200 Subject: [PATCH] tls: fix object prototype type confusion 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: https://github.com/nodejs/node/pull/14447 Reviewed-By: Colin Ihrig Reviewed-By: Fedor Indutny Reviewed-By: James M Snell Reviewed-By: Sakthipriyan Vairamani Reviewed-By: Ruben Bridgewater --- lib/_tls_common.js | 7 ++--- lib/tls.js | 2 +- test/parallel/test-tls-parse-cert-string.js | 13 +++++++- .../test-tls-translate-peer-certificate.js | 30 ++++++++++++++----- 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 272e17b2f7..ed0bd4c23f 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -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]; diff --git a/lib/tls.js b/lib/tls.js index 30525a254c..bbf73e6e2a 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -231,7 +231,7 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) { // Example: // C=US\nST=CA\nL=SF\nO=Joyent\nOU=Node.js\nCN=ca1\nemailAddress=ry@clouds.org 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('='); diff --git a/test/parallel/test-tls-parse-cert-string.js b/test/parallel/test-tls-parse-cert-string.js index 2e940805c0..165e45cb9a 100644 --- a/test/parallel/test-tls-parse-cert-string.js +++ b/test/parallel/test-tls-parse-cert-string.js @@ -1,3 +1,4 @@ +/* eslint-disable no-proto */ 'use strict'; const common = require('../common'); if (!common.hasCrypto) @@ -11,6 +12,7 @@ const tls = require('tls'); 'CN=ca1\nemailAddress=ry@clouds.org'; const singlesOut = tls.parseCertString(singles); assert.deepStrictEqual(singlesOut, { + __proto__: null, C: 'US', ST: 'CA', L: 'SF', @@ -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' }); @@ -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); } diff --git a/test/parallel/test-tls-translate-peer-certificate.js b/test/parallel/test-tls-translate-peer-certificate.js index 537c00a009..f8499e0c7e 100644 --- a/test/parallel/test-tls-translate-peer-certificate.js +++ b/test/parallel/test-tls-translate-peer-certificate.js @@ -1,3 +1,4 @@ +/* eslint-disable no-proto */ 'use strict'; const common = require('../common'); @@ -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); @@ -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 }), @@ -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 }); +}