Closed
Bug 805807
(CVE-2012-5841)
Opened 12 years ago
Closed 12 years ago
Filtering wrapper should filter setters when returning a property descriptor
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: bholley, Assigned: bholley)
Details
(Keywords: sec-high, Whiteboard: [adv-track-main17+][adv-track-esr17+])
Attachments
(6 files)
5.22 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
15.41 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
4.50 KB,
patch
|
mrbkap
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
2.31 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
4.78 KB,
patch
|
mrbkap
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
akeybl
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
1.78 KB,
patch
|
Details | Diff | Splinter Review |
Filing this so that I have a bug number for the mochitest I'm about to write. I'll fill in the details once I'm sure this is a problem.
Assignee | ||
Comment 1•12 years ago
|
||
Yep, testcase does what I thought it did. Here's the situation.
So our security wrappers filter at property access time. This generally works, but means that, once we return a function to the caller, the caller can call the function without any security checks (the CALL wrapper action is always allowed).
This becomes an issue when somebody uses ES5 introspection to look up a property descriptor, because they get both the getter and setter in one go, meaning that there isn't really a sane value we can pass for |bool set|. Jorendorff once commented that this is "completely absurd".
I actually thought about this issue as well on that fateful night in Toulouse, but I was thinking about it in terms of XOWs. Upon returning home, I discovered that the particularities of the cross-origin accessible property make this a non-issue, because the cross-origin-accessible properties that have setters (window.location, location.hash, and location.href) permit cross-origin sets. So in practice, this is a non-issue. (CCing moz_bug_r_a4 in case there's another aspect to this that I missed).
However, I just realized that this affects COWs. In particular, is someone grants 'r' access to an accessor descriptor, the caller can also call the setter, effectively granting 'rw' access.
The impact of this is mitigated by the fact that __exposedProps__ isn't used all that heavily in the wild, and I'd imagine that few if any people are using it in conjunction with accessor descriptors. Still, given that this is our new security story for chrome JS objects, I think we should definitely fix this.
I think the easiest solution is to have FilteringWrapper override get{Own,}PropertyDescriptor and filter the result appropriately.
Status: NEW → UNCONFIRMED
Ever confirmed: false
Assignee | ||
Comment 2•12 years ago
|
||
Er, I meant to indicate that this was confirmed. ;-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 3•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6c2668005091
I accidentally pushed the testcases in that try push as well. Hopefully nobody notices.
Assignee | ||
Comment 4•12 years ago
|
||
There were two failing tests, which I fixed. Attaching patches and flagging Blake for review.
Assignee | ||
Comment 5•12 years ago
|
||
There's really no reason to use the wishy-washy static COW Deny() here.
Also, note that the xpcshell-test wasn't testing what it thought it
was - interfaces is accessible from content code.
Attachment #676168 -
Flags: review?(mrbkap)
Assignee | ||
Comment 6•12 years ago
|
||
This is another one of those annoying situaitons in XPConnect right now where we
can't ask a question without potentially throwing if the answer is no. There's
also a bunch of unused cruft in here (like the Perm*Access stuff), so this stuff
was ripe for a spring cleaning. Unfortunately, I wasn't able to divide this patch
up nicely. Sorry for the big diff. :-(
In a nutshell, this patch changes things so that Policy::check() just becomes
a predicate that says whether the access is allowed or not. There's the remote
possibility that one of the underlying JSAPI calls in a ::check() implementation
might throw, so callers to ::check() should check JS_IsExceptionPending
afterwards (this doesn't catch OOM, but we can just continue along until the
next OOM-triggering operation and throw there).
Aside from exceptional cases, callers should call Policy::deny if they want to
report the failure. Policy::deny returns success value that should be returned
to the wrapper's consumer.
Attachment #676170 -
Flags: review?(mrbkap)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #676171 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #676172 -
Flags: review?(mrbkap)
Comment 9•12 years ago
|
||
Critsmash triage: Can you suggest a security rating for this issue?
Updated•12 years ago
|
Attachment #676168 -
Flags: review?(mrbkap) → review+
Comment 11•12 years ago
|
||
Comment on attachment 676170 [details] [diff] [review]
Part 2 - Rearchitect filtering policies so that check() doesn't throw on denial. v1
Review of attachment 676170 [details] [diff] [review]:
-----------------------------------------------------------------
I like this patch, but I have one non-nit question.
::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +347,4 @@
> {
> JSObject *wrappedObject = Wrapper::wrappedObject(wrapper);
>
> if (act == Wrapper::CALL) {
Nit: The braces should go away.
@@ +423,5 @@
> memset(&desc, 0, sizeof(desc));
> if (!JS_GetPropertyDescriptorById(cx, hallpass, id, JSRESOLVE_QUALIFIED, &desc)) {
> return false; // Error
> }
> + if (desc.obj == NULL || !(desc.attrs & JSPROP_ENUMERATE))
Nit, while you're here: if (!desc.obj || ...)
::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ +40,2 @@
> props[w++] = id;
> + else if (JS_IsExceptionPending(cx))
How come this can happen? It looks like an odd anti-pattern.
Attachment #676170 -
Flags: review?(mrbkap)
Comment 12•12 years ago
|
||
Comment on attachment 676170 [details] [diff] [review]
Part 2 - Rearchitect filtering policies so that check() doesn't throw on denial. v1
Review of attachment 676170 [details] [diff] [review]:
-----------------------------------------------------------------
Ah, you answered that in the comment where you attached the patch. The OOM thing is pretty ugly, though :(
Attachment #676170 -
Flags: review+
Comment 13•12 years ago
|
||
Comment on attachment 676171 [details] [diff] [review]
Part 3 - Filter setters. v1
Should this set the setter to null or to JS_StrictPropertyStub?
Attachment #676171 -
Flags: review?(mrbkap) → review+
Updated•12 years ago
|
Attachment #676172 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Blake Kaplan (:mrbkap) from comment #13)
> Should this set the setter to null or to JS_StrictPropertyStub?
Null makes more sense to me, and it's what I already pushed to try, so I'm going to go with that. If you think we should do JS_StrictPropertyStub I'm happy to change it though.
Assignee | ||
Comment 15•12 years ago
|
||
I landed the first two patches here, because they're big and not security-sensitive.
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ca11f4b470c
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/23c9f61a243b
I'll request sec-approval on part 3 now. We should avoid checking in the testcase until we've shipped a fix on all branches.
Flags: in-testsuite?
Comment 16•12 years ago
|
||
Unfortunately this had to be backed out for mochitest-1 failures on OS X and Windows:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=23c9f61a243b
{
1130 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | an unexpected uncaught JS exception reported through window.onerror - Error: Permission denied to access property 'classes' at http://mochi.test:8888/tests/browser/base/content/test/test_contextmenu.html:23
}
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57eeca4191dd
Assignee | ||
Comment 17•12 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+0] from comment #16)
> Unfortunately this had to be backed out for mochitest-1 failures on OS X and
> Windows:
Heh, looks like that test is disabled on linux, where I did my try push. I guess that's the risk of doing single-platform try pushes. ;-)
Assignee | ||
Comment 18•12 years ago
|
||
Relanded:
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c0e6ebdf3769
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/55ba2ad75e7d
Whiteboard: [leave open]
Comment 19•12 years ago
|
||
Assignee | ||
Comment 20•12 years ago
|
||
Comment on attachment 676171 [details] [diff] [review]
Part 3 - Filter setters. v1
Gah, somehow my last approval request didn't make it into bugzilla. I remember writing the comment though. :\
[Security approval request comment]
How easily can the security issue be deduced from the patch?
It's reasonably clear that we're not filtering setters for getOwnPropertyDescriptor. However, the most obvious implication of this (cross-origin access problems) is a red herring, as described in comment 1. On the other hand, it's not too much of a leap to realize that this is also a problem for COWs.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Testcase, yes. I'll avoid checking it in until we're safe.
Which older supported branches are affected by this flaw?
All.
If not all supported branches, which bug introduced the flaw?
FF4.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't have a backport, but I should be able to write one that doesn't depend on parts 1 and 2 (which are a bit risky to land on branches).
How likely is this patch to cause regressions; how much testing does it need?
The patch itself is pretty safe, but the backport is going to need some testing, because it'll be different from what was landed on central.
Attachment #676171 -
Flags: sec-approval?
Updated•12 years ago
|
Attachment #676171 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 21•12 years ago
|
||
Whiteboard: [leave open]
Assignee | ||
Comment 22•12 years ago
|
||
This is a patch for branches that doesn't depend on the re-architecting of part
1 and 2. It does the same nasty set/clear exception trick.
Attachment #679551 -
Flags: review?(mrbkap)
Assignee | ||
Comment 23•12 years ago
|
||
Attaching an interdiff for mrbkap's viewing pleasure.
Comment 24•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6bbf2b1cfc
https://hg.mozilla.org/mozilla-central/rev/9a6bbf2b1cfc
Leaving open for branch patch.
Updated•12 years ago
|
Attachment #679551 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 25•12 years ago
|
||
Comment on attachment 676171 [details] [diff] [review]
Part 3 - Filter setters. v1
[Approval Request Comment]
Bug caused by (feature/regressing bug #): FF4.
User impact if declined: Security exploits.
Testing completed (on m-c, etc.): Baking on m-i. There are small differences in the backported patch though.
Risk to taking this patch (and alternatives if risky): Not super high risk. No alternatives.
String or UUID changes made by this patch: None
Attachment #676171 -
Flags: approval-mozilla-beta?
Attachment #676171 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 26•12 years ago
|
||
Comment on attachment 679551 [details] [diff] [review]
Filter setters on branch. v1
Er, this is the branch patch, not the other one. Comment 25 applies here.
Attachment #679551 -
Flags: approval-mozilla-beta?
Attachment #679551 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #676171 -
Flags: approval-mozilla-beta?
Attachment #676171 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #679551 -
Flags: approval-mozilla-beta?
Attachment #679551 -
Flags: approval-mozilla-beta+
Attachment #679551 -
Flags: approval-mozilla-aurora?
Attachment #679551 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 27•12 years ago
|
||
status-firefox18:
--- → fixed
Assignee | ||
Comment 28•12 years ago
|
||
Comment on attachment 679551 [details] [diff] [review]
Filter setters on branch. v1
We should probably get this on for esr10 as well.
Attachment #679551 -
Flags: approval-mozilla-esr10?
Assignee | ||
Comment 29•12 years ago
|
||
status-firefox17:
--- → fixed
Updated•12 years ago
|
status-firefox-esr10:
--- → affected
tracking-firefox-esr10:
--- → 17+
Comment 30•12 years ago
|
||
Comment on attachment 679551 [details] [diff] [review]
Filter setters on branch. v1
Approving for ESR10. Please land as soon as possible.
Attachment #679551 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Comment 31•12 years ago
|
||
ttps://hg.mozilla.org/releases/mozilla-esr10/rev/12da5c13b5b0
Updated•12 years ago
|
Alias: CVE-2012-5841
Whiteboard: [adv-track-main17+][adv-track-esr17+]
Assignee | ||
Comment 32•12 years ago
|
||
This is fixed everywhere know AFAICT.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 33•12 years ago
|
||
Comment 34•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #33)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a9a8bb2752
https://hg.mozilla.org/mozilla-central/rev/c5a9a8bb2752
Flags: in-testsuite? → in-testsuite+
Updated•12 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•