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)

All
Android
defect
Not set
normal

Tracking

()

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

People

(Reporter: bholley, Assigned: bholley)

Details

(Keywords: sec-high, Whiteboard: [adv-track-main17+][adv-track-esr17+])

Attachments

(6 files)

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.
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
Er, I meant to indicate that this was confirmed. ;-)
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://tbpl.mozilla.org/?tree=Try&rev=6c2668005091

I accidentally pushed the testcases in that try push as well. Hopefully nobody notices.
There were two failing tests, which I fixed. Attaching patches and flagging Blake for review.
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)
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)
Attachment #676171 - Flags: review?(mrbkap)
Attachment #676172 - Flags: review?(mrbkap)
Critsmash triage: Can you suggest a security rating for this issue?
I'm going to go with sec-high.
Keywords: sec-high
Attachment #676168 - Flags: review?(mrbkap) → review+
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 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 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+
Attachment #676172 - Flags: review?(mrbkap) → review+
(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.
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?
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
(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. ;-)
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?
Attachment #676171 - Flags: sec-approval? → sec-approval+
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)
Attached patch interdiffSplinter Review
Attaching an interdiff for mrbkap's viewing pleasure.
(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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla19
Attachment #679551 - Flags: review?(mrbkap) → review+
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?
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?
Attachment #676171 - Flags: approval-mozilla-beta?
Attachment #676171 - Flags: approval-mozilla-aurora?
Attachment #679551 - Flags: approval-mozilla-beta?
Attachment #679551 - Flags: approval-mozilla-beta+
Attachment #679551 - Flags: approval-mozilla-aurora?
Attachment #679551 - Flags: approval-mozilla-aurora+
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?
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+
ttps://hg.mozilla.org/releases/mozilla-esr10/rev/12da5c13b5b0
Alias: CVE-2012-5841
Whiteboard: [adv-track-main17+][adv-track-esr17+]
This is fixed everywhere know AFAICT.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.