Closed
Bug 811343
Opened 12 years ago
Closed 12 years ago
Fatal assertion when creating proxy with null proto
Categories
(Core :: JavaScript Engine, defect)
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)
2.18 KB,
patch
|
bholley
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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. :(
Reporter | ||
Updated•12 years ago
|
Assignee: general → ejpbruel
Assignee | ||
Comment 1•12 years ago
|
||
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)
Comment 2•12 years ago
|
||
Tracking for FF18 as this needs to be uplifted as well in case we land Bug 793160 on aurora
Comment 3•12 years ago
|
||
There's also
> s = Cu.Sandbox(window)
> Cu.evalInSandbox("p = new Proxy(Object.create(this), {})", s)
> Cu.getGlobalForObject(s.p)
Comment 4•12 years ago
|
||
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-
Assignee | ||
Comment 5•12 years ago
|
||
(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?
Comment 6•12 years ago
|
||
(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).
Assignee | ||
Comment 7•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #682600 -
Flags: review?(bobbyholley+bmo) → review+
Assignee | ||
Comment 8•12 years ago
|
||
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 9•12 years ago
|
||
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+
Updated•12 years ago
|
status-firefox-esr10:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
status-firefox-esr17:
--- → affected
tracking-firefox-esr10:
--- → ?
tracking-firefox-esr17:
--- → ?
Assignee | ||
Comment 10•12 years ago
|
||
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?
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/02eeaba348fb
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Assignee | ||
Comment 12•12 years ago
|
||
(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?
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
(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.
Assignee | ||
Comment 15•12 years ago
|
||
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?
Comment 16•12 years ago
|
||
What's the security rating of this bug?
Updated•12 years ago
|
Flags: needinfo?(ejpbruel)
Assignee | ||
Comment 17•12 years ago
|
||
(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)
Comment 18•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
(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?
Comment 20•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
(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.
Comment 22•12 years ago
|
||
Thanks. That sounds reasonable and I've marked this as a sec-moderate.
Keywords: sec-moderate
Comment 23•12 years ago
|
||
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+
Comment 24•12 years ago
|
||
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.
Assignee | ||
Comment 25•12 years ago
|
||
(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.
Assignee | ||
Comment 26•12 years ago
|
||
(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?
Assignee | ||
Comment 27•12 years ago
|
||
(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.
Comment 28•12 years ago
|
||
You're good to go - land away!
Assignee | ||
Comment 29•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/d8a8efc10f67
Assignee | ||
Comment 30•12 years ago
|
||
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
Assignee | ||
Comment 31•12 years ago
|
||
(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.
Updated•12 years ago
|
Updated•12 years ago
|
Flags: in-testsuite? → in-testsuite+
Comment 32•12 years ago
|
||
It looks like you landed the original, r-'ed patch?
Assignee | ||
Comment 33•12 years ago
|
||
(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.
Updated•11 years ago
|
Whiteboard: [adv-main18-]
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•