825697 - (CVE-2013-0795) Components.lookupMethod allows calling of SOW functions

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Cody Crews]

Attachment: 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)]

(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]

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)]

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)]

Nevermind, Boris pointed out a good trick. Patches forthcoming.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Check for all wrapper types. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 7

[Bobby Holley (:bholley)]

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 - ex-MoCo]]

I need some release management input on when we want to take this one.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Bobby, can you suggest a security rating here?

Comment 10

[Bobby Holley (:bholley)]

(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 - ex-MoCo]]

(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]

> 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)]

(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]

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)]

(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]]

(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)]

Yeah, we can probably just land this whenever.

Comment 18

[Al Billings [:abillings - ex-MoCo]]

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)]

(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]

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]

(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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f5562cbabf

Comment 23

[Bobby Holley (:bholley)]

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]]

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)]

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]]

https://hg.mozilla.org/mozilla-central/rev/d4f5562cbabf

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a2e74c936887
https://hg.mozilla.org/releases/mozilla-beta/rev/f8aaee7e65ea
https://hg.mozilla.org/releases/mozilla-b2g18/rev/29c8694183c1
https://hg.mozilla.org/releases/mozilla-esr17/rev/a867722ce57c

Note that this will need tef+ or shira+ to land on b2g18_v1_0_1.

Comment 28

[Bobby Holley (:bholley)]

I'm concerned that this might be being exploited in the wild. See bug 837370 comment 22.

Comment 29

[Bobby Holley (:bholley)]

Flagging dveditz for some general input here.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

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:]]

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]]

Bobby - does comment 32 match your expectations?

Comment 34

[Bobby Holley (:bholley)]

> 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]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/2328639c245f

Comment 37

[Bobby Holley (:bholley)]

lookupMethod was removed in bug 856437.