Last Comment Bug 825697 - (CVE-2013-0795) Components.lookupMethod allows calling of SOW functions
(CVE-2013-0795)
: Components.lookupMethod allows calling of SOW functions
Status: RESOLVED FIXED
[adv-main20+][adv-esr1705+]
: regression, sec-critical, testcase
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: 20 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla22
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 774245
  Show dependency treegraph
 
Reported: 2012-12-31 15:02 PST by Cody Crews
Modified: 2014-10-01 04:41 PDT (History)
17 users (show)
abillings: sec‑bounty+
bobbyholley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
wontfix
+
wontfix
+
fixed
+
verified
+
verified
20+
fixed
20+
fixed
wontfix
fixed


Attachments
lookupMethodpoc.html (1.63 KB, text/html)
2012-12-31 15:02 PST, Cody Crews
no flags Details
Check for all wrapper types. v1 (1.66 KB, patch)
2013-01-16 20:19 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details | Diff | Splinter Review
Tests. v1 (2.24 KB, patch)
2013-01-16 20:19 PST, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details | Diff | Splinter Review

Description Cody Crews 2012-12-31 15:02:23 PST
Created attachment 696815 [details]
lookupMethodpoc.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121231 Firefox/20.0
Build ID: 20121231030859

Steps to reproduce:

Called Components.lookupMethod to obtain the cloneNode method of a SOW protected node.


Actual results:

I obtained a clone of the node specified in Components.lookupMethod.


Expected results:

Components.lookupMethod should throw an exception as it did in ff 17.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-12-31 15:52:17 PST
(In reply to codyc from comment #0)
> Components.lookupMethod should throw an exception as it did in ff 17.

Are you sure it wasn't 16? I reimplemented lookupMethod in 17 (bug 774245), and clearly messed this one up. :-(
Comment 2 Cody Crews 2013-01-01 05:18:07 PST
I was only making a comparison to using lookupMethod to bypass SOW's, so I'm not entirely sure.  I believe this has been a problem on and off since around ff13/14 and I assume that's why its already been reimplemented(maybe?)  I know that in 17 I was already playing with accessing methods on SOW's using it with practically no success.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2013-01-16 19:36:54 PST
We're fixing the XBL situation in general, but in the mean time I think we want to write a point fix here.

codyc has clever ways of getting at NAC that I don't really want to writing into my testcase. I'd rather use SpecialPowers, but it's not clear to me what the simplest way is to get a piece of NAC from a video control or somesuch. Dolske, any idea on the easiest way for privileged code to get a piece of NAC?
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2013-01-16 20:12:06 PST
Nevermind, Boris pointed out a good trick. Patches forthcoming.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2013-01-16 20:19:20 PST
Created attachment 703163 [details] [diff] [review]
Check for all wrapper types. v1
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2013-01-16 20:19:31 PST
Created attachment 703164 [details] [diff] [review]
Tests. v1
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2013-01-22 21:14:06 PST
Comment on attachment 703163 [details] [diff] [review]
Check for all wrapper types. v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Fairly obvious. It's clear that we're now handling all security wrappers rather than just cross-compartment wrappers, and the wrapper not in the intersection there is the SOW, which is what this is about. As a mitigation factor, very few people, even Mozillians, understand the intricacies of XBL and NAC.

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

The test does for sure.

Which older supported branches are affected by this flaw?

FF17 and above.

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

bug 774245. Though it may have existed before in some other form.

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

I don't have backports. There backport to FF17 and FF18 will be slightly nontrivial due to the lack of bug 800915 on those branches, but I think it should be fine.

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

Backports not withstanding, this patch is very low risk.

Another thing to keep in mind is that we're about to churn a bunch of the XBL stuff in bug 821850, which will fix a lot of outstanding XBL security issues. It might be worth waiting until that's on trunk before landing this patch anywhere.
Comment 8 Al Billings [:abillings] 2013-01-23 16:38:09 PST
I need some release management input on when we want to take this one.
Comment 9 Al Billings [:abillings] 2013-01-24 14:18:46 PST
Bobby, can you suggest a security rating here?
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2013-01-25 02:26:12 PST
(In reply to Al Billings [:abillings] from comment #9)
> Bobby, can you suggest a security rating here?

SOW bypass, so probably sec-critical.
Comment 11 Al Billings [:abillings] 2013-01-30 14:23:58 PST
(In reply to Bobby Holley (:bholley) from comment #7)

> Another thing to keep in mind is that we're about to churn a bunch of the
> XBL stuff in bug 821850, which will fix a lot of outstanding XBL security
> issues. It might be worth waiting until that's on trunk before landing this
> patch anywhere.

I think we want to see bug 821850 land first.
Comment 12 Cody Crews 2013-02-01 16:17:04 PST
> I think we want to see bug 821850 land first.

Anyway I could be CC'ed on bug 821850? I don't know how you guys like to handle that internally, but I've just been curious on the progress of it.  Wouldn't mind seeing how you guys are tackling the task of locking down XBL either, seems like a major undertaking with all the different ways to manipulate it.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2013-02-01 16:39:48 PST
(In reply to codyc from comment #12)
> > I think we want to see bug 821850 land first.
> 
> Anyway I could be CC'ed on bug 821850? I don't know how you guys like to
> handle that internally, but I've just been curious on the progress of it. 
> Wouldn't mind seeing how you guys are tackling the task of locking down XBL
> either, seems like a major undertaking with all the different ways to
> manipulate it.

Sure thing - given that you're probably one of less than a dozen people in the world who understands how XBL works, it'd be great to have your eyes on it. :-) The patches work and we just need to get them reviewed and landed.
Comment 14 Cody Crews 2013-02-02 09:09:15 PST
I would definitely love to take a look at the changes made, and depending on the overall changes I might have a few new tricks to try out too. ;)  So the changes from what I've been able to gather is a different compartment with a special principal right?  Have I been added to the CC list yet?  I thought that would let me see the bug but right now I'm still getting access denied when trying to view it.  No real hurry on it though because its the weekend and I know like me everyone is trying to relax and enjoy it.  Ill have a few new things being brought to light here soon, trying to find out the full extent of what can be done with them, and one of them involves one of the still experimental APIs so no rush but definitely some interesting things in the works.
Comment 15 Bobby Holley (:bholley) (busy with Stylo) 2013-02-03 08:53:11 PST
(In reply to codyc from comment #14)
> I would definitely love to take a look at the changes made, and depending on
> the overall changes I might have a few new tricks to try out too. ;)  So the
> changes from what I've been able to gather is a different compartment with a
> special principal right?  Have I been added to the CC list yet?  I thought
> that would let me see the bug but right now I'm still getting access denied
> when trying to view it.

Whoops, my bad. I still had the tab open but never clicked "Save Changes". Should work now. ;-)
Comment 16 Daniel Veditz [:dveditz] 2013-02-04 13:41:10 PST
(In reply to Al Billings [:abillings] from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #7)
> > Another thing to keep in mind is that we're about to churn a bunch of the
> > XBL stuff in bug 821850, which will fix a lot of outstanding XBL security
> > issues. It might be worth waiting until that's on trunk before landing
> > this patch anywhere.
> 
> I think we want to see bug 821850 land first.

The patches in bug 821850 are huge and will never be back-ported. Why would we wait for those to land rather than work up the back-ports for old branches now and get it in?
Comment 17 Bobby Holley (:bholley) (busy with Stylo) 2013-02-08 05:48:09 PST
Yeah, we can probably just land this whenever.
Comment 18 Al Billings [:abillings] 2013-02-12 11:50:11 PST
Sigh. We're at the end of the cycle. We need to wait a few weeks into the next one, based on the comments about how obvious this is, security wise, on check in. I'd say March 12, at this point. 

Any comments from anyone before I give is sec-approval+ for 3/12/2013?
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2013-02-12 14:37:38 PST
(In reply to Al Billings [:abillings] from comment #18)
> Sigh. We're at the end of the cycle. We need to wait a few weeks into the
> next one, based on the comments about how obvious this is, security wise, on
> check in. I'd say March 12, at this point. 
> 
> Any comments from anyone before I give is sec-approval+ for 3/12/2013?

That sounds reasonable. I think we'll want to land on all branches (mostly) simultaneously.
Comment 20 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-13 11:29:36 PST
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2013-03-08 15:35:10 PST
(In reply to Al Billings [:abillings] from comment #18)
> Sigh. We're at the end of the cycle. We need to wait a few weeks into the
> next one, based on the comments about how obvious this is, security wise, on
> check in. I'd say March 12, at this point. 
> 
> Any comments from anyone before I give is sec-approval+ for 3/12/2013?

Let's get this in *before* 3/12 so it's in the beta going to build on that day.  Please land to trunk and nominate for branch uplift with risk assessment.
Comment 22 Bobby Holley (:bholley) (busy with Stylo) 2013-03-11 10:13:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f5562cbabf
Comment 23 Bobby Holley (:bholley) (busy with Stylo) 2013-03-11 10:16:39 PDT
Comment on attachment 703163 [details] [diff] [review]
Check for all wrapper types. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 774245
User impact if declined: Security problems.
Testing completed (on m-c, etc.): Just landed to inbound.
Risk to taking this patch (and alternatives if risky): Not very risky. It's possible that this could make some code throw that previously didn't throw, but this is Components.lookupMethod we're talking about, which is a pretty obscure gecko-only feature that's been long-deprecated anyway. We have telemetry that says that <0.05% of users ever encounter a page that calls lookupMethod, and the chance that those instances involve doing lookups on security wrappers is insanely small. More to the point, if they were doing that, we really _don't_ want to continue allowing it.
 
String or UUID changes made by this patch: None.
Comment 24 Alex Keybl [:akeybl] 2013-03-11 16:15:15 PDT
Comment on attachment 703163 [details] [diff] [review]
Check for all wrapper types. v1

Approving for branches to get as much testing with our pre-release populations as possible prior to FF20's release.

Please also nominate for ESR17/B2G18 approval when you get the chance. Thanks!
Comment 25 Bobby Holley (:bholley) (busy with Stylo) 2013-03-11 16:17:29 PDT
Comment on attachment 703163 [details] [diff] [review]
Check for all wrapper types. v1

Flagging for b2g18 and esr17. The same analysis given in comment 23 applies.
Comment 26 Ryan VanderMeulen [:RyanVM] 2013-03-11 17:20:44 PDT
https://hg.mozilla.org/mozilla-central/rev/d4f5562cbabf
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2013-03-14 12:01:07 PDT
I'm concerned that this might be being exploited in the wild. See bug 837370 comment 22.
Comment 29 Bobby Holley (:bholley) (busy with Stylo) 2013-03-14 12:01:33 PDT
Flagging dveditz for some general input here.
Comment 30 Al Billings [:abillings] 2013-03-14 14:49:17 PDT
This has been checked in for all releases. Without direct evidence of a zero day, we can probably wait the two weeks until the next release.
Comment 32 Matt Wobensmith [:mwobensmith][:matt:] 2013-03-28 15:02:36 PDT
I can repro this issue in affected Aurora and m-c builds, and can see it fixed post-patch.

However, I can't reproduce the issue at all in 17esr builds, 17 through 17.0.5. Re-reading comments 1 and 2, this appears to be true because the bug was regressed post-17.

Based on that, perhaps we should change the status flag for 17 to "unaffected"?
Comment 33 Alex Keybl [:akeybl] 2013-04-02 14:52:04 PDT
Bobby - does comment 32 match your expectations?
Comment 34 Bobby Holley (:bholley) (busy with Stylo) 2013-04-02 15:23:15 PDT
> However, I can't reproduce the issue at all in 17esr builds, 17 through
> 17.0.5. Re-reading comments 1 and 2, this appears to be true because the bug
> was regressed post-17.

As comment 1 indicates, the regressing changeset was from bug 774245, which landed on esr17. But if we're having trouble reproducing the problem on old esr17, I don't think we should spend too much effort on it. As long as this changeset landed there we should be fine.
Comment 35 Ryan VanderMeulen [:RyanVM] 2013-04-02 18:45:22 PDT
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2328639c245f
Comment 37 Bobby Holley (:bholley) (busy with Stylo) 2014-10-01 04:41:42 PDT
lookupMethod was removed in bug 856437.

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