Last Comment Bug 451680 - (CVE-2008-5511) XSS by attaching a binding to an element in an unloaded document
(CVE-2008-5511)
: XSS by attaching a binding to an element in an unloaded document
Status: VERIFIED FIXED
: [sg:high]
: verified1.8.1.19, verified1.9.0.5
Product: Core
Classification: Components
Component: Security
: unspecified
: x86 Windows XP
: P1 normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
: toolkit
:
:
: 464174
  Show dependency treegraph
 
Reported: 2008-08-21 23:55 PDT by moz_bug_r_a4
Modified: 2008-12-16 18:27 PST (History)
10 users (show)
jst: blocking1.9.1-
jst: wanted1.9.1+
samuel.sidler+old: blocking1.9.0.5+
samuel.sidler+old: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.19+
samuel.sidler+old: wanted1.8.1.x+
asac: blocking1.8.0.next+
See Also:
Crash Signature:


Attachments
Not ready for prime time (1.27 KB, patch)
2008-08-22 14:51 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Whack (651 bytes, patch)
2008-10-21 14:30 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Whack harder (2.69 KB, patch)
2008-10-24 18:56 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.9.0.5+
Details | Diff | Splinter Review
Patch for the 1.8 branch (10.23 KB, patch)
2008-11-17 15:50 PST, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
dveditz: approval1.8.1.19+
asac: approval1.8.0.next+
Details | Diff | Splinter Review
for 1.8.0 (just context changes in backport) (5.87 KB, patch)
2008-12-16 00:50 PST, Alexander Sack
no flags Details | Diff | Splinter Review

Summon comment box

Description moz_bug_r_a4 2008-08-21 23:55:57 PDT
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 2008-08-21 23:58:49 PDT
Created attachment 335012 [details]
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) 2008-08-22 02:10:12 PDT
This looks like Blake's bug for the next release.
Comment 3 Blake Kaplan (:mrbkap) 2008-08-22 14:51:06 PDT
Created attachment 335109 [details] [review]
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, jst@mozilla.com) 2008-09-24 15:46:29 PDT
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) 2008-10-21 13:47:06 PDT
Comment on attachment 335109 [details] [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) 2008-10-21 14:30:32 PDT
Created attachment 344184 [details] [review]
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 (:bz) 2008-10-21 20:26:17 PDT
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) 2008-10-24 11:34:39 PDT
Pushing out per Blake.
Comment 9 Blake Kaplan (:mrbkap) 2008-10-24 18:56:35 PDT
Created attachment 344712 [details] [review]
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 (:bz) 2008-10-24 21:05:01 PDT
Comment on attachment 344712 [details] [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) 2008-11-07 15:27:47 PST
http://hg.mozilla.org/mozilla-central/rev/e92dfeb501cd
Comment 12 Blake Kaplan (:mrbkap) 2008-11-13 18:23:37 PST
Comment on attachment 344712 [details] [review]
Whack harder

This applies cleanly to the 1.9 branch.
Comment 13 Daniel Veditz 2008-11-14 11:47:02 PST
Comment on attachment 344712 [details] [review]
Whack harder

Approved for 1.9.0.5, a=dveditz for release-drivers
Comment 14 Blake Kaplan (:mrbkap) 2008-11-17 15:50:57 PST
Created attachment 348669 [details] [review]
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) 2008-11-17 20:35:39 PST
Checked into the 1.9 branch.
Comment 16 Daniel Veditz 2008-11-17 22:19:51 PST
Comment on attachment 348669 [details] [review]
Patch for the 1.8 branch

Approved for 1.8.1.19, a=dveditz for release-drivers
Comment 17 Blake Kaplan (:mrbkap) 2008-11-18 16:12:11 PST
Fixed on the 1.8 branch.
Comment 18 Al Billings [:abillings] 2008-11-25 16:14:29 PST
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 2008-12-16 00:49:11 PST
Comment on attachment 348669 [details] [review]
Patch for the 1.8 branch

a=asac for 1.8.0
Comment 20 Alexander Sack 2008-12-16 00:50:11 PST
Created attachment 353179 [details] [review]
for 1.8.0 (just context changes in backport)

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