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.

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

tls: fix object prototype type confusion #14447

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 24, 2017

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/

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jul 24, 2017
@bnoordhuis bnoordhuis changed the title Fix11771 tls: fix empty issuer/subject/infoAccess parsing Jul 24, 2017
@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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 it should be semver-major, as currently parseCertString is documented. I would prefer otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

There are too many PRs interacting with this change.

@refack
Copy link
Contributor

refack commented Jul 24, 2017

return;

if (c.infoAccess.hasOwnProperty(key))
if (key in c.infoAccess)
Copy link
Contributor

@mscdex mscdex Jul 24, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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
>

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 24, 2017
@bnoordhuis bnoordhuis changed the title tls: fix empty issuer/subject/infoAccess parsing tls: fix object prototype type confusion Jul 25, 2017
@bnoordhuis
Copy link
Member Author

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
Copy link
Member Author

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

@addaleax
Copy link
Member

This needs to be rebased again, sorry.

@refack
Copy link
Contributor

refack commented Jul 30, 2017

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

@bnoordhuis
Copy link
Member Author

@bnoordhuis
Copy link
Member Author

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

@refack
Copy link
Contributor

refack commented Aug 16, 2017

@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.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member Author

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
Copy link
Member Author

@BridgeAR
Copy link
Member

Landed in 0f7c06e

@BridgeAR BridgeAR closed this Aug 30, 2017
BridgeAR pushed a commit that referenced this pull request Aug 30, 2017
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 <[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]>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Aug 31, 2017
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 <[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]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.