Closed Bug 811343 Opened 12 years ago Closed 12 years ago

Fatal assertion when creating proxy with null proto

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox17 --- unaffected
firefox18 + fixed
firefox19 + fixed
firefox-esr10 - unaffected
firefox-esr17 --- unaffected

People

(Reporter: bzbarsky, Assigned: ejpbruel)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [adv-main18-])

Attachments

(1 file, 1 obsolete file)

From bug 793160 comment 19:

> a = new Proxy(Object.create(null), {});
> w = Components.utils.getGlobalForObject(a);

now gives an
Assertion failure: isGlobal(), at ../../../js/src/vm/GlobalObject.h:499
(and in release builds has w === a)

Assertions generally means bad.  Note the questions in bug 793160 comment 8.  Also note that this used to just crash with a null deref before bug 793160, but not it lands us in an invariant-violating state, hence the security flag.  :(
Assignee: general → ejpbruel
Attached patch Patch to be reviewed (obsolete) — — Splinter Review
The problem is that the fix in bug 793160 is incomplete. It does not access the prototype if it is NULL. However, since the prototype is used to obtain the parent for the proxy, this leaves us with a NULL parent. In this case, the parent of the proxy should be the parent of the callee.

I've attached a patch to remedy the problem. Test included.
Attachment #682004 - Flags: review?(bobbyholley+bmo)
Tracking for FF18 as this needs to be uplifted as well in case we land  Bug 793160 on aurora
There's also
> s = Cu.Sandbox(window)
> Cu.evalInSandbox("p = new Proxy(Object.create(this), {})", s)
> Cu.getGlobalForObject(s.p)
Comment on attachment 682004 [details] [diff] [review]
Patch to be reviewed

As discussed on #jsapi, I think it's a bug the object creation paths allow us to create non-global parentless objects at all. They should default to cx->global() IMO.

Also, why are we being so particular about the parent chain here? Is there any reason for it, or can we just _always_ use cx->global?
Attachment #682004 - Flags: review?(bobbyholley+bmo) → review-
(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 682004 [details] [diff] [review]
> Patch to be reviewed
> 
> As discussed on #jsapi, I think it's a bug the object creation paths allow
> us to create non-global parentless objects at all. They should default to
> cx->global() IMO.
> 
> Also, why are we being so particular about the parent chain here? Is there
> any reason for it, or can we just _always_ use cx->global?

I don't actually know. Who can we ask?
(In reply to Eddy Bruel [:ejpbruel] from comment #5)
> > Also, why are we being so particular about the parent chain here? Is there
> > any reason for it, or can we just _always_ use cx->global?
> 
> I don't actually know. Who can we ask?

Well, you wrote this code, right? If you're not using the parent chain for anything special, just either set the parent to cx->global(), or leave it null and rely on JSAPI fixing it up as discussed (the former is probably more clear to casual readers though).
Attached patch Patch to be reviewed — — Splinter Review
New patch that always sets a proxy's parent to cx->global(), regardless of target.
Attachment #682004 - Attachment is obsolete: true
Attachment #682600 - Flags: review?(bobbyholley+bmo)
Attachment #682600 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 682600 [details] [diff] [review]
Patch to be reviewed

[Security approval request comment]
How easily can the security issue be deduced from the patch?

The problem is trivial. The parent of a non-global proxy was set to NULL when passed a target with a NULL prototype. We only use this parent to obtain the global object from the proxy, so we just set it to cx->global().

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

There is a single test that covers exactly the problem described above.

Which older supported branches are affected by this flaw?

This flaw affects all branches since the introduction of direct proxies.

If not all supported branches, which bug introduced the flaw?

Bug 703537

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

No, but creating them is almost trivial. This bug is fallout from a patch that fixed a crasher with the same code. This patch landed on central, but not on aurora and beta. Backporting to the latter two would require some minor rebasing for this patch.

How likely is this patch to cause regressions; how much testing does it need?

I don't expect any major regressions. Parent chains are covered pretty well. The proxy stuff is an exception since it's still fairly new.
Attachment #682600 - Flags: sec-approval?
Comment on attachment 682600 [details] [diff] [review]
Patch to be reviewed

sec-approval+ but we should hold off on checking in the test that makes this super visible.
Attachment #682600 - Flags: sec-approval? → sec-approval+
Comment on attachment 682600 [details] [diff] [review]
Patch to be reviewed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 703537

User impact if declined:
Users will be able to crash the browser by triggering an assert

Testing completed (on m-c, etc.):
Landed on inbound. Requesting approval anyway since this bug is tracked for FF18, for which the train leaves next monday.

Risk to taking this patch (and alternatives if risky)
Very low

String or UUID changes made by this patch:
None
Attachment #682600 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/02eeaba348fb
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(In reply to Ryan VanderMeulen from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/02eeaba348fb

Just to make sure, since you're the one that set the in-testsuite? flag, does that mean that you will check at some point if the tests have already landed? Or is that still my responsibility?
(In reply to Eddy Bruel [:ejpbruel] from comment #12)
> (In reply to Ryan VanderMeulen from comment #11)
> > https://hg.mozilla.org/mozilla-central/rev/02eeaba348fb
> 
> Just to make sure, since you're the one that set the in-testsuite? flag,
> does that mean that you will check at some point if the tests have already
> landed? Or is that still my responsibility?

In general, I would say that responsibility falls on the bug assignee.
(In reply to Ryan VanderMeulen from comment #13)
> (In reply to Eddy Bruel [:ejpbruel] from comment #12)
> > (In reply to Ryan VanderMeulen from comment #11)
> > > https://hg.mozilla.org/mozilla-central/rev/02eeaba348fb
> > 
> > Just to make sure, since you're the one that set the in-testsuite? flag,
> > does that mean that you will check at some point if the tests have already
> > landed? Or is that still my responsibility?
> 
> In general, I would say that responsibility falls on the bug assignee.

Ok. Thanks for pointing that out.
Comment on attachment 682600 [details] [diff] [review]
Patch to be reviewed

This should probably also land on beta, if that's still possible. The approval request comment is the same as the one for aurora (see above)
Attachment #682600 - Flags: approval-mozilla-beta?
What's the security rating of this bug?
Flags: needinfo?(ejpbruel)
(In reply to Alex Keybl [:akeybl] from comment #16)
> What's the security rating of this bug?

I'm sorry if this sounds daft, but it's always better to ask than to pretend you know something: who decides that, and based on what information?
Flags: needinfo?(ejpbruel)
(In reply to Eddy Bruel [:ejpbruel] from comment #17)
> (In reply to Alex Keybl [:akeybl] from comment #16)
> > What's the security rating of this bug?
> 
> I'm sorry if this sounds daft, but it's always better to ask than to pretend
> you know something: who decides that, and based on what information?

Well, the security *can* decide but we try to get input from the devs that know this code because we aren't necessarily experts in it. Since it is assigned to you, Eddy, I think we were hopingt hat you might have a suggestion.
(In reply to Al Billings [:abillings] from comment #18)
> (In reply to Eddy Bruel [:ejpbruel] from comment #17)
> > (In reply to Alex Keybl [:akeybl] from comment #16)
> > > What's the security rating of this bug?
> > 
> > I'm sorry if this sounds daft, but it's always better to ask than to pretend
> > you know something: who decides that, and based on what information?
> 
> Well, the security *can* decide but we try to get input from the devs that
> know this code because we aren't necessarily experts in it. Since it is
> assigned to you, Eddy, I think we were hopingt hat you might have a
> suggestion.

Ok. Are security ratings level based? If so, what levels are there, and how are they defined. If not, what information do you need?
Security ratings are defined at https://wiki.mozilla.org/Security_Severity_Ratings. They are keywords in bugzilla. We also have a Mozilla wide security site at https://wiki.mozilla.org/Security.
(In reply to Al Billings [:abillings] from comment #20)
> Security ratings are defined at
> https://wiki.mozilla.org/Security_Severity_Ratings. They are keywords in
> bugzilla. We also have a Mozilla wide security site at
> https://wiki.mozilla.org/Security.

Thanks Al, that is very useful information.

So, as bz already indicated, this bug is caused by a patch that fixed a crasher. Some of the cases that used to crash now trigger an assertion. This assertion is caused by us trying to fetch the corresponding global object for a proxy, but since the parent pointer of the proxy isn't properly set, we end up using the proxy itself. This is bad.

In short, this bug allows people to use proxies as arbitrary global objects. I don't fully understand the implications of this, or how it could be abused, but based on the fact that this is a corruption of state, it could conceivably be used for temporary DoS attacks. My feeling is therefore that this bug should be marked as sec-moderate.
Thanks. That sounds reasonable and I've marked this as a sec-moderate.
Keywords: sec-moderate
Comment on attachment 682600 [details] [diff] [review]
Patch to be reviewed

[Triage Comment]
I think we're ready to land this bug on branches, especially since it also resolves tracked bug 793160.
Attachment #682600 - Flags: approval-mozilla-beta?
Attachment #682600 - Flags: approval-mozilla-beta+
Attachment #682600 - Flags: approval-mozilla-aurora?
Attachment #682600 - Flags: approval-mozilla-aurora+
I'm confused by the status flags here. This is an additional fix for bug 793160, which was a regression from bug 703537. We don't/shouldn't need this in Firefox 18 (beta) unless we're also taking bug 793160 -- which sounds like a good idea but I don't see any noms in that direction.
(In reply to Daniel Veditz [:dveditz] from comment #24)
> I'm confused by the status flags here. This is an additional fix for bug
> 793160, which was a regression from bug 703537. We don't/shouldn't need this
> in Firefox 18 (beta) unless we're also taking bug 793160 -- which sounds
> like a good idea but I don't see any noms in that direction.

This patch fixes both bug 811343 *and* bug 793160.
(In reply to Daniel Veditz [:dveditz] from comment #24)
> I'm confused by the status flags here. This is an additional fix for bug
> 793160, which was a regression from bug 703537. We don't/shouldn't need this
> in Firefox 18 (beta) unless we're also taking bug 793160 -- which sounds
> like a good idea but I don't see any noms in that direction.

I'm about to land this patch on beta, but I want to make sure I'm doing the right thing here. Does the above comment take away your confusion?
(In reply to Alex Keybl [:akeybl] from comment #23)
> Comment on attachment 682600 [details] [diff] [review]
> Patch to be reviewed
> 
> [Triage Comment]
> I think we're ready to land this bug on branches, especially since it also
> resolves tracked bug 793160.

So are we good to land this on beta? I don't think we should wait any longer.
You're good to go - land away!
Oops. Mercurial complained that the test was already there, so it didn't get included with the patch. Fixed that oversight here:
https://hg.mozilla.org/releases/mozilla-beta/rev/97df81d9fbb4
(In reply to Alex Keybl [:akeybl] from comment #28)
> You're good to go - land away!

Landed the patch. Please let me know if I did anything wrong so I can fix it ASAP.
Flags: in-testsuite? → in-testsuite+
It looks like you landed the original, r-'ed patch?
(In reply to Simon Lindholm from comment #32)
> It looks like you landed the original, r-'ed patch?

Damn. I took the second, r+'d, patch, but that r+ was conditional on the change proposed by bholley.

Luckily, it doesn't really matter if we use cx->global() or the parent of the callee, so this patch still resolves the issue, and the patch that landed on m-c has the requested change. No further action should be required.
Whiteboard: [adv-main18-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: