344494 - Arbitrary code execution with DOM Inspector by using Array.prototype methods (and Firebug)

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect, P1)

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1

This works on the trunk, fx2.0 and fx1.5.0.5.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2

This works on the trunk, fx2.0, fx1.5.0.5, fx1.0.8 and moz1.7.13.

Comment 3

[Daniel Veditz [:dveditz]]

With Firebug installed the victim doesn't even have to explicitly launch something as is the case with DOM Inspector.

1. Open testcase 1
2. Switch to another tab
3. return to testcase tab --> exploit fires.

Every time you return to the testcase tab the alert will fire three times from Firebug's utils.js line 322 (version 0.4), and another once when you close the tab.

Firebug makes the problem far worse -- every page is a potential attack, not just pages you actively inspect. If you have new windows set to open in tabs (the default in FF2!) it's trivial to force this by opening a window and then closing it. Doesn't seem to work with real windows, just tabs for some reason (different events that Firebug's looking for?)

If Firebug has XML spying turned on then the presence of window.__firebug__ will alert hackers that it's there. I've been worried that the presence of functions that chrome will call means that's hackable, too, though I think all the obvious stuff has been fixed in 1.5.0.5.  I guess that's for another bug (moz_bug_r_a4: would any of your PAC-hacking techniques work on this?)

Interesting: disabling Firebug just now from this window caused the exploit alert to fire one last time in another window containing testcase1.

Comment 4

[moz_bug_r_a4]

(In reply to comment #3)
> If Firebug has XML spying turned on then the presence of window.__firebug__
> will alert hackers that it's there.

In that case, Firebug will call content-defined window.__firebug__ getter, and
the call to getter can trigger an exploit.  Also, by default, window.console
getter, document.styleSheets getter, window.addEventListener, etc. etc. can be
used as a trigger of an exploit.  I have a testcase for Firebug, which can
exploit without user interaction.  Should I attach it here?

Comment 5

[Daniel Veditz [:dveditz]]

> I have a testcase for Firebug, which can
> exploit without user interaction.  Should I attach it here?

Let's start a new bug for the Firebug issues.

Comment 6

[moz_bug_r_a4]

(In reply to comment #5)
> Let's start a new bug for the Firebug issues.

Bug 344751.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So if we turned XPCNativeWrapper on for DOM inspector, the only thing affected would be the "JS Object" view, right?

Frankly, that may be the way to go for the time being -- I'm not sure how much the "examine random page-set JS stuff" case is really used in the JS Object view. If we do want to leave that case allowed, we can control security better by moving the impl into C++ and pushing JSContexts on the stack or whatever it takes to get the right principals into play, but we'd still be running arbitrary code (with the page's privileges) on every prop access.  So we're still kinda stuck, since the page can always set up getters that will, e.g., close the window when one gets some property (to disallow inspection altogether)....

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For JS-defined getters we could actually skip calling the getter altogether and just indicate that a getter was defined.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll have a stab at this.

Comment 10

[moz_bug_r_a4]

It seems that the patch (attachment 229913 [details] [diff] [review]) in Bug 344495 also fixes the
privilege escalation bugs.

Comment 11

[Daniel Veditz [:dveditz]]

Appears fixed by the patch in bug 344495

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Reassigning to moz_bug_r_a4 since he's working on this

Comment 13

[Daniel Veditz [:dveditz]]

No safe fix, mrbkap's unavailable, try for 1.8.0.8

Comment 14

[Mike Schroepfer]

Pushing out to 1.8.1.1

Comment 15

[Daniel Veditz [:dveditz]]

No good fix yet, clearing 1.8.1.2/1.8.0.10 blocking flags

Comment 16

[Damon Sicore (:damons)]

Targeting to B1 per conversation with Blake.

Comment 17

[Daniel Veditz [:dveditz]]

This appears fixed on the trunk in M8, by XOW (bug 367911, which fixed bug 344495)?

Comment 18

[moz_bug_r_a4]

DOMi is still exploitable.

If content does document.__defineGetter__("nodeType", x), and DOMi accesses
document.nodeType, x is not called.  But, if content does
document.__proto__.__defineGetter__("nodeType", x), x can be called by DOMi. 
Also, e.g. document.documentElement.__defineGetter__("nodeType", x) can be used
since document.documentElement is not wrapped in XOW.

Comment 19

[moz_bug_r_a4]

Attachment: testcase 3 - document.__proto__.__defineGetter__("nodeType", x)

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, the general solution for this bug will ensure that DOMi always gets a wrapper of some kind. Right now, it's relying on what's basically a happy coincidence.

Comment 21

[moz_bug_r_a4]

Attachment: testcase 4 - document.documentElement.__defineGetter__("nodeType", x)

Comment 22

[Damon Sicore (:damons)]

Blake, I know you are super busy, but any progress on this?  Getting really close to freeze date for beta.

Comment 23

[Mike Schroepfer]

So if this is not yet fixed on 1.8 why block 1.9b1 for it?

Comment 24

[Daniel Veditz [:dveditz]]

For scary-risky changes such as these that rely on new or tweaked wrappers we really need them tested on trunk before back-porting the fixes to 1.8 (if back-porting is even possible). b1 will get a lot of use, nightlies not so much.

Comment 25

[Daniel Veditz [:dveditz]]

That said we've been waiting a long time on this, it could potentially wait for b2 if we have to and you don't mind pushing the risk out that late. I don't know what the actual risk is of what Blake's proposing, I'd imagine most of the arch-level risk was in the cross-origin wrappers (XOW) themselves and that's already in.

Comment 26

[Damon Sicore (:damons)]

Just to clarify here, this security issue is not publicly known, correct?  If not, I agree that we could probably ship beta without it.

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

For what it's worth, I've been hoping to find a general solution that will fix this without changing any DOMI code. However, there are a couple of DOMI changes that will fix this for good (using XPCSafeJSObjectWrapper on content nodes explicitly in DOMI). While that's not a hack, my ideal solution would be to do said wrapping in XPConnect and let extensions Just Work.

Comment 28

[Damon Sicore (:damons)]

Any progress, Blake?  :-)

Comment 29

[Damon Sicore (:damons)]

Retargeting to M10.  Not a public sec issue, and there is no way a fix can make it for beta.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

This should be fixed by wrapperizing harder. P1 due to the severity.

Comment 31

[Damon Sicore (:damons)]

We're not sure if this can get done before Beta 2 here.  Blake, are you sure
you can get this done?  Moving to P2 for now.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch I'm looking at

This patch isn't ready yet. SJOWs need some more lovin' before they'll be ready for prime time. However, this does stop testcase 3 in its tracks. I'll file (probably non-ss) bugs to track SJOW that needs to happen before this goes in.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 229060 [details]
testcase 1

Obsoleted because we no longer allow indirect eval.

Comment 34

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 229061 [details]
testcase 2

Depends on indirect eval.

Comment 35

[chris hofmann]

we should try and get any security bug work for this area done before b3, so there is testing time for any compatibility and stability fall out.  blake, think that is possible?

Comment 36

[Mike Schroepfer]

Blake can you set the TM to either B3 or B4 depending on when we can get this in?

Comment 37

[Johnny Stenback (:jst)]

Builds with the patch in this bug (and s/JS_GET_CLASS(cx, /STOBJ_GET_CLASS(/) applied are now available at:

https://build.mozilla.org/tryserver-builds/2008-02-19_16:[e-mail]-wrapper-fun/

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to trunk

Comment 39

[Justin Dolske [:Dolske]]

I gave the tryserver build a quick twirl with Firebug 1.2. It didn't seem obviously broken (the DOMi thing worked, and I was able to set a breakpoint and trace JS in a page), but I'm not sure exactly what to bang on.

Comment 40

[Damon Sicore (:damons)]

Anyone else see any issues?  Johnny, Blake, does it look like the latest patch from Blake is a go here for beta 4?

Comment 41

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 304532 [details] [diff] [review]
Updated to trunk

This should be ready to go.

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 43

[Damon Sicore (:damons)]

Fixing the blocking flag here.  Was still carrying the tracking1.9 flag for some reason.

Comment 44

[moz_bug_r_a4]

Attachment: testcase 5

A testcase that works on fx-2.0.0.14pre-2008-03-16-03, as promised in bug
363891 comment #36.