Last Comment Bug 805807 - (CVE-2012-5841) Filtering wrapper should filter setters when returning a property descriptor
(CVE-2012-5841)
: Filtering wrapper should filter setters when returning a property descriptor
Status: RESOLVED FIXED
[adv-track-main17+][adv-track-esr17+]
: sec-high
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: mozilla19
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-26 07:44 PDT by Bobby Holley (PTO through June 13)
Modified: 2013-03-18 13:11 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed
17+
fixed


Attachments
Part 1 - Make Components wrapper throw on denial. v1 (5.22 KB, patch)
2012-10-29 08:58 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
Details | Diff | Review
Part 2 - Rearchitect filtering policies so that check() doesn't throw on denial. v1 (15.41 KB, patch)
2012-10-29 08:59 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
Details | Diff | Review
Part 3 - Filter setters. v1 (4.50 KB, patch)
2012-10-29 08:59 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
abillings: sec‑approval+
Details | Diff | Review
Part 4 - Tests. v1 (2.31 KB, patch)
2012-10-29 08:59 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
Details | Diff | Review
Filter setters on branch. v1 (4.78 KB, patch)
2012-11-07 22:35 PST, Bobby Holley (PTO through June 13)
mrbkap: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Review
interdiff (1.78 KB, patch)
2012-11-07 22:37 PST, Bobby Holley (PTO through June 13)
no flags Details | Diff | Review

Description Bobby Holley (PTO through June 13) 2012-10-26 07:44:44 PDT
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.
Comment 1 Bobby Holley (PTO through June 13) 2012-10-26 08:51:23 PDT
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.
Comment 2 Bobby Holley (PTO through June 13) 2012-10-26 11:20:13 PDT
Er, I meant to indicate that this was confirmed. ;-)
Comment 3 Bobby Holley (PTO through June 13) 2012-10-26 11:20:47 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6c2668005091

I accidentally pushed the testcases in that try push as well. Hopefully nobody notices.
Comment 4 Bobby Holley (PTO through June 13) 2012-10-29 08:58:23 PDT
There were two failing tests, which I fixed. Attaching patches and flagging Blake for review.
Comment 5 Bobby Holley (PTO through June 13) 2012-10-29 08:58:45 PDT
Created attachment 676168 [details] [diff] [review]
Part 1 - Make Components wrapper throw on denial. v1

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.
Comment 6 Bobby Holley (PTO through June 13) 2012-10-29 08:59:09 PDT
Created attachment 676170 [details] [diff] [review]
Part 2 - Rearchitect filtering policies so that check() doesn't throw on denial. v1

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.
Comment 7 Bobby Holley (PTO through June 13) 2012-10-29 08:59:25 PDT
Created attachment 676171 [details] [diff] [review]
Part 3 - Filter setters. v1
Comment 8 Bobby Holley (PTO through June 13) 2012-10-29 08:59:39 PDT
Created attachment 676172 [details] [diff] [review]
Part 4 - Tests. v1
Comment 9 Al Billings [:abillings] 2012-10-31 10:27:44 PDT
Critsmash triage: Can you suggest a security rating for this issue?
Comment 10 Bobby Holley (PTO through June 13) 2012-11-01 00:13:26 PDT
I'm going to go with sec-high.
Comment 11 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-11-01 17:21:40 PDT
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.
Comment 12 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-11-01 17:24:59 PDT
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 :(
Comment 13 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-11-01 17:25:40 PDT
Comment on attachment 676171 [details] [diff] [review]
Part 3 - Filter setters. v1

Should this set the setter to null or to JS_StrictPropertyStub?
Comment 14 Bobby Holley (PTO through June 13) 2012-11-02 05:23:56 PDT
(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.
Comment 15 Bobby Holley (PTO through June 13) 2012-11-02 05:34:05 PDT
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.
Comment 16 Ed Morley [:emorley] 2012-11-02 07:16:35 PDT
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
Comment 17 Bobby Holley (PTO through June 13) 2012-11-02 17:28:06 PDT
(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 20 Bobby Holley (PTO through June 13) 2012-11-07 10:50:50 PST
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.
Comment 21 Bobby Holley (PTO through June 13) 2012-11-07 15:46:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a6bbf2b1cfc
Comment 22 Bobby Holley (PTO through June 13) 2012-11-07 22:35:56 PST
Created attachment 679551 [details] [diff] [review]
Filter setters on branch. v1

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.
Comment 23 Bobby Holley (PTO through June 13) 2012-11-07 22:37:44 PST
Created attachment 679552 [details] [diff] [review]
interdiff

Attaching an interdiff for mrbkap's viewing pleasure.
Comment 24 Ed Morley [:emorley] 2012-11-08 03:07:58 PST
(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.
Comment 25 Bobby Holley (PTO through June 13) 2012-11-08 14:45:10 PST
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
Comment 26 Bobby Holley (PTO through June 13) 2012-11-08 14:46:11 PST
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.
Comment 27 Bobby Holley (PTO through June 13) 2012-11-09 11:02:59 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/6b57929bcd7e
Comment 28 Bobby Holley (PTO through June 13) 2012-11-09 11:05:53 PST
Comment on attachment 679551 [details] [diff] [review]
Filter setters on branch. v1

We should probably get this on for esr10 as well.
Comment 29 Bobby Holley (PTO through June 13) 2012-11-09 11:26:28 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/a61dc39f8fd7
Comment 30 Alex Keybl [:akeybl] 2012-11-12 09:39:17 PST
Comment on attachment 679551 [details] [diff] [review]
Filter setters on branch. v1

Approving for ESR10. Please land as soon as possible.
Comment 31 Bobby Holley (PTO through June 13) 2012-11-12 13:06:06 PST
ttps://hg.mozilla.org/releases/mozilla-esr10/rev/12da5c13b5b0
Comment 32 Bobby Holley (PTO through June 13) 2012-11-18 17:47:14 PST
This is fixed everywhere know AFAICT.
Comment 33 Bobby Holley (PTO through June 13) 2013-03-11 14:47:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a9a8bb2752
Comment 34 Ryan VanderMeulen [:RyanVM] 2013-03-12 13:00:06 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.