451680 - (CVE-2008-5511) XSS by attaching a binding to an element in an unloaded document

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[moz_bug_r_a4]

It's possible to use a binding to perform an XSS attack in a similar way to bug
428672 and bug 433328.

Comment 1

[moz_bug_r_a4]

Attachment: testcase

This tries to get cookies for www.mozilla.com.
This works on trunk, fx3.0.x and fx2.0.0.x.

Comment 2

[Samuel Sidler (old account; do not CC)]

This looks like Blake's bug for the next release.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Not ready for prime time

We *want* this badly. But it's going to cause regressions. I'm attaching it here so jst can run mochitests with it.

Comment 4

[Johnny Stenback (:jst)]

Not blocking the release, but it'd be awesome to at least wall-paper over this for 3.1, but the real fix will have to wait for next release.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 335109 [details] [diff] [review]
Not ready for prime time

This caused mochitests to not run at all and this isn't the right bug for this patch. Wallpaper coming right up.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Whack

The actual offending call to GetScriptGlobalObject that is biting us is buried deep in nsXBLProtoImpl but rather than play whack-a-mole in there too, I say cut this path off at the head. I don't think it's really valid to call addBinding on a document after it's been torn down anyway. Tell me if I'm wrong!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Isn't the right check on the content's ownerDocument?  Otherwise if I take content from doc A and call addBinding() on doc B, won't we do the wrong things (assume A and B same-origin so we pass that check in addBinding)?

Or is the GetScriptGlobalObject happening on some document that's not the ownerDocument of the bound node?

And can we also whack that mole by using GetScopeObject or some such?

(On a side note, I really wish we could just remove addBinding.  It's poorly-tested and all.)

Comment 8

[Samuel Sidler (old account; do not CC)]

Pushing out per Blake.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Whack harder

This appears to work. I did a grep for GetScriptGlobalObject in content/xbl/src/* and these callsites seemed like the ones that wanted patching.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 344712 [details] [diff] [review]
Whack harder

Looks good, but I'm also happy adding bullet-proofing to addBinding if we want (in addition to this change).

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/e92dfeb501cd

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 344712 [details] [diff] [review]
Whack harder

This applies cleanly to the 1.9 branch.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 344712 [details] [diff] [review]
Whack harder

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for the 1.8 branch

The lack of GetScopeObject on the 1.8 branch made this more complicated. However, in writing this patch, I realized that I really need to hold *strong* refs to the return value of nsIDocument::GetScopeObject (since it's implemented as a weak reference underneath the hood). I'll file a new bug on that, though.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Checked into the 1.9 branch.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 348669 [details] [diff] [review]
Patch for the 1.8 branch

Approved for 1.8.1.19, a=dveditz for release-drivers

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branch.

Comment 18

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

Verified for 1.8.1.19 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008112503 BonEcho/2.0.0.19pre.

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008112505 GranParadiso/3.0.5pre.

I also checked this in trunk so I'm marking it as verified.

Comment 19

[Alexander Sack]

Comment on attachment 348669 [details] [diff] [review]
Patch for the 1.8 branch

a=asac for 1.8.0

Comment 20

[Alexander Sack]

Attachment: for 1.8.0 (just context changes in backport)