tls: fix object prototype type confusion #14447

bnoordhuis · 2017-07-24T12:48:38Z

Fix for #11771. Note that the other two are fixes for (arguably minor) security vulnerabilities.

I don't think core is directly affected but user applications might be susceptible to type confusion stemming from manipulating __proto__ or properties inherited from Object.prototype.

CI: https://ci.nodejs.org/job/node-test-pull-request/9320/

evanlucas · 2017-07-24T12:50:54Z

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


We have treated other changes like this as semver-major. Think this will cause breakage?

It might, hard to say. It would break conn.getPeerCertificate().hasOwnProperty(k) but then again, such code is vulnerable to object manipulation now.

Yes, but it's still technically semver-major. @nodejs/ctc ... thoughts?

I think it should be semver-major, as currently parseCertString is documented. I would prefer otherwise.

@mcollina parseCertString() isn't documented. You might be thinking of #14193.

Then it's not semver-major for me. I'd go for semver-minor, or even patch.

There are too many PRs interacting with this change.

refack · 2017-07-24T13:02:12Z

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/923/

mscdex · 2017-07-24T13:13:59Z

lib/_tls_common.js

-        return;
-
-      if (c.infoAccess.hasOwnProperty(key))
+      if (key in c.infoAccess)


Perhaps a c.infoAccess[key] === undefined would be more efficient?

Or just save the value to a variable and use that value here and use it to skip the lookups below.

Unlikely to matter because it's only a handful of keys (if it's set at all.) This is shorter so that's what I picked.

Did a micro benchmark for that on TF&I. Small benefit for in

> var o = {a:1, b:2, c:3, d:4, e:5} > var j = 0 > console.time('in'); for (let i = 0; i < 1e8; ++i) {const key = String.fromCharCode(97 + (i % 10)); if (key in o) ++j};console.timeEnd('in') in: 2614.723ms > console.time('!=='); for (let i = 0; i < 1e8; ++i) {const key = String.fromCharCode(97 + (i % 10)); if (o[key] !== undefined) ++j};console.timeEnd('!==') !==: 3160.657ms > console.time('!in'); for (let i = 0; i < 1e8; ++i) {const key = String.fromCharCode(97 + (i % 10)); if (!(key in o)) ++j};console.timeEnd('!in') !in: 2842.438ms > console.time('==='); for (let i = 0; i < 1e8; ++i) {const key = String.fromCharCode(97 + (i % 10)); if (o[key] === undefined) ++j};console.timeEnd('===') ===: 2884.768ms > console.time('!=='); for (let i = 0; i < 1e8; ++i) {const key = String.fromCharCode(97 + (i % 10)); if (o[key] !== undefined) ++j};console.timeEnd('!==') !==: 2999.367ms > console.time('==='); for (let i = 0; i < 1e8; ++i) {const key = String.fromCharCode(97 + (i % 10)); if (o[key] === undefined) ++j};console.timeEnd('===') ===: 3113.552ms > console.time('!in'); for (let i = 0; i < 1e8; ++i) {const key = String.fromCharCode(97 + (i % 10)); if (!(key in o)) ++j};console.timeEnd('!in') !in: 2679.466ms >

mscdex · 2017-07-24T13:16:49Z

lib/_tls_common.js

@@ -169,21 +169,18 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {
  if (!c)
    return null;

-  if (c.issuer) c.issuer = tls.parseCertString(c.issuer);
-  if (c.issuerCertificate && c.issuerCertificate !== c) {
+  if (c.issuer != null) c.issuer = tls.parseCertString(c.issuer);


Can't we just do a strict comparison against undefined? parseCertString() shouldn't be returning null?

I'm not sure I understand your question. The idea is to call parseCertString() for empty strings but not null or undefined.

bnoordhuis · 2017-07-25T10:12:36Z

Split off the fix for #11771 into #14473 so we can debate at our leisure whether the type confusion fix is semver-major or not.

bnoordhuis · 2017-07-27T15:48:40Z

Rebased now that #14473 landed. New CI: https://ci.nodejs.org/job/node-test-pull-request/9374/

addaleax · 2017-07-30T11:22:18Z

This needs to be rebased again, sorry.

refack · 2017-07-30T17:34:04Z

So maybe we do need 12981 lib: save a reference to intrinsic constructs

bnoordhuis · 2017-08-07T12:36:36Z

Rebased: https://ci.nodejs.org/job/node-test-pull-request/9515/

bnoordhuis · 2017-08-07T12:37:25Z

@refack Elaborate? I don't understand what that PR has to do with this PR.

refack · 2017-08-16T11:42:20Z

@refack Elaborate? I don't understand what that PR has to do with this PR.

If we are worried about "prototype injection" we could save a pristine sealed copy of Object.prototype and use it instead of Object.create(null). This should have minimal impact on the outputted type while preventing injection.

indutny

LGTM

Use `Object.create(null)` for dictionary objects so that keys from certificate strings or the authorityInfoAccess field cannot conflict with Object.prototype properties.

bnoordhuis · 2017-08-17T12:30:22Z

If we are worried about "prototype injection" we could save a pristine sealed copy of Object.prototype and use it instead of Object.create(null).

Aha, like that. That's actually quite clever but since this is semver-major anyway, I think I'll go for the most obvious approach for now. Maybe we can use it in a back-port to the stable branches.

bnoordhuis · 2017-08-17T12:31:44Z

Rebased + new CI: https://ci.nodejs.org/job/node-test-pull-request/9709/

BridgeAR · 2017-08-30T18:47:05Z

Landed in 0f7c06e

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: #14447 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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#14447 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

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 <cjihrig@gmail.com> Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jul 24, 2017

bnoordhuis changed the title ~~Fix11771~~ tls: fix empty issuer/subject/infoAccess parsing Jul 24, 2017

bnoordhuis mentioned this pull request Jul 24, 2017

tls requires a subject even when altNames are defined #11771

Closed

evanlucas reviewed Jul 24, 2017

View reviewed changes

cjihrig approved these changes Jul 24, 2017

View reviewed changes

mscdex reviewed Jul 24, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 24, 2017

bnoordhuis mentioned this pull request Jul 25, 2017

tls: deprecate parseCertString & move to internal #14249

Closed

4 tasks

bnoordhuis force-pushed the fix11771 branch from 5d3ade5 to 81d1b1f Compare July 25, 2017 10:06

bnoordhuis mentioned this pull request Jul 25, 2017

tls: fix empty issuer/subject/infoAccess parsing #14473

Closed

bnoordhuis changed the title ~~tls: fix empty issuer/subject/infoAccess parsing~~ tls: fix object prototype type confusion Jul 25, 2017

bnoordhuis force-pushed the fix11771 branch from 81d1b1f to f8628a1 Compare July 27, 2017 15:45

bnoordhuis force-pushed the fix11771 branch from f8628a1 to 49fbe91 Compare August 7, 2017 12:35

indutny approved these changes Aug 16, 2017

View reviewed changes

tls: fix object prototype type confusion

16964ca

Use `Object.create(null)` for dictionary objects so that keys from certificate strings or the authorityInfoAccess field cannot conflict with Object.prototype properties.

bnoordhuis force-pushed the fix11771 branch from 49fbe91 to 16964ca Compare August 17, 2017 12:31

jasnell approved these changes Aug 24, 2017

View reviewed changes

thefourtheye approved these changes Aug 25, 2017

View reviewed changes

BridgeAR approved these changes Aug 25, 2017

View reviewed changes

BridgeAR closed this Aug 30, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: fix object prototype type confusion #14447

tls: fix object prototype type confusion #14447

bnoordhuis commented Jul 24, 2017 •

edited

Loading

evanlucas Jul 24, 2017

bnoordhuis Jul 24, 2017

jasnell Jul 24, 2017

mcollina Jul 25, 2017

bnoordhuis Jul 25, 2017

mcollina Jul 25, 2017

mcollina Jul 25, 2017

refack commented Jul 24, 2017

mscdex Jul 24, 2017 •

edited

Loading

bnoordhuis Jul 24, 2017

refack Jul 24, 2017

mscdex Jul 24, 2017

bnoordhuis Jul 24, 2017

bnoordhuis commented Jul 25, 2017

bnoordhuis commented Jul 27, 2017

addaleax commented Jul 30, 2017

refack commented Jul 30, 2017

bnoordhuis commented Aug 7, 2017

bnoordhuis commented Aug 7, 2017

refack commented Aug 16, 2017

indutny left a comment

bnoordhuis commented Aug 17, 2017

bnoordhuis commented Aug 17, 2017

BridgeAR commented Aug 30, 2017

tls: fix object prototype type confusion #14447

tls: fix object prototype type confusion #14447

Conversation

bnoordhuis commented Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Jul 24, 2017

mscdex Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jul 25, 2017

bnoordhuis commented Jul 27, 2017

addaleax commented Jul 30, 2017

refack commented Jul 30, 2017

bnoordhuis commented Aug 7, 2017

bnoordhuis commented Aug 7, 2017

refack commented Aug 16, 2017

indutny left a comment

Choose a reason for hiding this comment

bnoordhuis commented Aug 17, 2017

bnoordhuis commented Aug 17, 2017

BridgeAR commented Aug 30, 2017

bnoordhuis commented Jul 24, 2017 •

edited

Loading

mscdex Jul 24, 2017 •

edited

Loading