Last Comment Bug 464620 - XSS with SessionStore after bug 463205, bug 463206, and bug 461743 are fixed
: XSS with SessionStore after bug 463205, bug 463206, and bug 461743 are fixed
Status: VERIFIED FIXED
: [sg:high]
: verified1.8.1.19, verified1.9.0.5, verified1.9.1
Product: Firefox
Classification: Client Software
Component: Session Restore
: unspecified
: All All
: P2 normal (vote)
: Firefox 3.5
Assigned To: Simon Bünzli
: session.restore
:
: 293363
: 463205 514816
  Show dependency treegraph
 
Reported: 2008-11-12 21:27 PST by moz_bug_r_a4
Modified: 2010-02-13 12:24 PST (History)
16 users (show)
mbeltzner: blocking‑firefox3.5+
dveditz: blocking1.9.0.5+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.19+
dveditz: wanted1.8.1.x+
asac: wanted1.8.0.x-
dietrich: in‑testsuite?
See Also:
Crash Signature:


Attachments
patch (replaces all anti-XSS patches that haven't landed yet) (5.17 KB, patch)
2008-11-13 13:50 PST, Simon Bünzli
no flags Details | Diff | Splinter Review
unbitrotted (replaces all anti-XSS patches that haven't landed yet) (5.06 KB, patch)
2008-11-16 14:02 PST, Simon Bünzli
dietrich: review+
mbeltzner: approval1.9.1b2+
Details | Diff | Splinter Review
branch patch (replaces all anti-XSS patches that haven't landed yet) (4.32 KB, patch)
2008-11-17 11:57 PST, Simon Bünzli
dveditz: approval1.9.0.5+
Details | Diff | Splinter Review
branch patch (1.8.1) (replaces all anti-XSS patches that haven't landed yet) (4.56 KB, patch)
2008-11-17 11:57 PST, Simon Bünzli
dveditz: approval1.8.1.19+
Details | Diff | Splinter Review
tests for bugs 459906, 461743, 463205 and 464620 (26.41 KB, patch)
2008-11-18 15:30 PST, Simon Bünzli
no flags Details | Diff | Splinter Review
tests for bugs 459906, 461743, 463205 and 464620 (27.86 KB, patch)
2009-03-07 05:32 PST, Simon Bünzli
dietrich: review+
Details | Diff | Splinter Review
Testcase 2 & 3 Results Screenshot (26.40 KB, image/png)
2009-07-13 11:14 PDT, Aakash Desai [:aakashd]
no flags Details

Summon comment box

Description moz_bug_r_a4 2008-11-12 21:27:43 PST
With the patches in bug 463205, bug 463206, and bug 461743, there are still
problems.  Content JS can load a new document when SessionStore fires an input
event, and also when SessionStore sets body.innerHTML.
Comment 1 moz_bug_r_a4 2008-11-12 21:29:16 PST
Created attachment 347922 [details]
testcase 1

This tries to inject text data into http://htmledit.squarefree.com/
This works only on fx3.0.x.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.102&mark=1704,1709-1711#1700

After firing an input event, a document in |aContent| can be a different
document.
Comment 2 moz_bug_r_a4 2008-11-12 21:30:18 PST
Created attachment 347923 [details]
testcase 2

This tries to inject innerHTML into
http://devedge-temp.mozilla.org/viewsource/2003/midas/01/example1.html
This works on trunk and fx3.0.x.

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2005

After calling restoreFormData()/restoreTextData(), a document in |aContent| can
be a different document.
Comment 3 moz_bug_r_a4 2008-11-12 21:31:25 PST
Created attachment 347924 [details]
testcase 3

This tries to inject innerHTML into
http://devedge-temp.mozilla.org/viewsource/2003/midas/01/example1.html
This works on trunk and fx3.0.x. (and fx2.0.0.x on Windows.)

Note: this does not work without the patch in bug 461743.

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2005

When setting innerHTML, a content-registered mutation event listener can load a
new document in other subframe.
Comment 4 Daniel Veditz 2008-11-13 10:12:51 PST
mrbkap, jst: is there some deeper fix we can do here to get out of this whack-a-mole with session-restore?
Comment 5 Simon Bünzli 2008-11-13 13:50:17 PST
Created attachment 348045 [details] [review]
patch (replaces all anti-XSS patches that haven't landed yet)

Sure, so lets just make sure that we're seeing the document we expect before every single operation that injects data into a document.
Comment 6 Blake Kaplan (:mrbkap) 2008-11-13 15:05:31 PST
I think the "right" fix here is bug 293363. The exploits come down to the fact that we're taking some HTML from origin (a) [bugzilla in this case] and by the time we inject it as innerHTML we impute the origin to be mozilla. If we remembered that the innerHTML string came from bugzilla, I don't think these attacks would work.
Comment 7 Simon Bünzli 2008-11-16 14:02:50 PST
Created attachment 348470 [details] [review]
unbitrotted (replaces all anti-XSS patches that haven't landed yet)
Comment 8 Dietrich Ayala (:dietrich) 2008-11-17 00:00:05 PST
Comment on attachment 348470 [details] [review]
unbitrotted (replaces all anti-XSS patches that haven't landed yet)

this looks fine, r=me for the change. this should be testable, can you provide one when you have time?
Comment 9 Simon Bünzli 2008-11-17 00:14:52 PST
Comment on attachment 348470 [details] [review]
unbitrotted (replaces all anti-XSS patches that haven't landed yet)

Tests and branch patches are coming later this week.
Comment 10 Daniel Veditz 2008-11-17 11:24:07 PST
(In reply to comment #5)
> patch (replaces all anti-XSS patches that haven't landed yet)

Does that mean that bug 463205, bug 463206, and bug 461743 are no longer checkin-needed, only this patch goes in?
Comment 11 Simon Bünzli 2008-11-17 11:36:18 PST
(In reply to comment #10)
We'll still want the tests from those bugs and bug 461743 could land independently (and should ASAP), as it's about a different issue.
Comment 12 Simon Bünzli 2008-11-17 11:57:12 PST
Created attachment 348609 [details] [review]
branch patch (replaces all anti-XSS patches that haven't landed yet)
Comment 13 Simon Bünzli 2008-11-17 11:57:37 PST
Created attachment 348610 [details] [review]
branch patch (1.8.1) (replaces all anti-XSS patches that haven't landed yet)
Comment 14 Mike Beltzner [:beltzner] 2008-11-17 21:47:52 PST
Comment on attachment 348470 [details] [review]
unbitrotted (replaces all anti-XSS patches that haven't landed yet)

a191=beltzner, show me some follow-up love for the tests.

Dietrich, should this block b2?
Comment 15 Daniel Veditz 2008-11-17 22:22:49 PST
Comment on attachment 348609 [details] [review]
branch patch (replaces all anti-XSS patches that haven't landed yet)

Approved for 1.9.0.5, a=dveditz for release-drivers
Comment 16 Daniel Veditz 2008-11-17 22:23:05 PST
Comment on attachment 348610 [details] [review]
branch patch (1.8.1) (replaces all anti-XSS patches that haven't landed yet)

Approved for 1.8.1.19, a=dveditz for release-drivers
Comment 17 Dietrich Ayala (:dietrich) 2008-11-18 09:37:23 PST
simon, when you make a patch for the test for this, can you also include the tests from the other bugs as well?
Comment 18 Dietrich Ayala (:dietrich) 2008-11-18 10:54:29 PST
trunk: http://hg.mozilla.org/mozilla-central/rev/8b81d681d1ad
Comment 19 Dietrich Ayala (:dietrich) 2008-11-18 12:17:37 PST
Comment on attachment 348609 [details] [review]
branch patch (replaces all anti-XSS patches that haven't landed yet)

1.9.0 branch patch does not apply cleanly
Comment 20 Simon Bünzli 2008-11-18 13:19:01 PST
(In reply to comment #19)
> 1.9.0 branch patch does not apply cleanly

WFM. Modified, out-of-date or wrong tree?

(In reply to comment #17)
Sure, you'll get an all-in-one anti-XSS test suite.
Comment 21 Dietrich Ayala (:dietrich) 2008-11-18 13:49:34 PST
(In reply to comment #15)
> (From update of attachment 348609 [details] [review])
> Approved for 1.9.0.5, a=dveditz for release-drivers

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.106; previous revision: 1.105
done

(yep, my tree was not updated)
Comment 22 Dietrich Ayala (:dietrich) 2008-11-18 13:49:49 PST
(In reply to comment #16)
> (From update of attachment 348610 [details] [review])
> Approved for 1.8.1.19, a=dveditz for release-drivers

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.5.2.53; previous revision: 1.5.2.52
done
Comment 23 Simon Bünzli 2008-11-18 15:30:06 PST
Created attachment 348856 [details] [review]
tests for bugs 459906, 461743, 463205 and 464620

There go the tests. I could use a hand for getting rid of the timeouts, though. Haven't found a different way of waiting until the tests had a chance to fail (even though they shouldn't). At least executeSoon doesn't work except if at least half a dozen of them are nested.
Comment 24 Al Billings [:abillings] 2008-11-25 16:49:35 PST
These manual test cases are a little confusing. 

I'm seeing the same behavior in testcase 2 and 3 above with 3.0.4 and my 1.9.0.5 build: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008112505 GranParadiso/3.0.5pre. With the note on 3 in comment 3, I'm not sure if that's expected or not. 

The only case clearly fixed is the first one.
Comment 25 moz_bug_r_a4 2008-11-25 21:10:14 PST
I verified that testcase 2 and 3 are fixed.

testcase 2:
I can reproduce on fx3.0.4, but not on fx-3.0.5pre-2008-11-25-05.

testcase 3:
I can reproduce on fx-3.0.5pre-2008-11-18-05, but not on
fx-3.0.5pre-2008-11-25-05.
Comment 26 Al Billings [:abillings] 2008-11-25 22:40:54 PST
All right. You found the bug so I'm willing to believe you. :-)

Thanks for looking into this.
Comment 27 Al Billings [:abillings] 2008-12-01 13:53:08 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/2008120103 BonEcho/2.0.0.19pre.
Comment 28 Simon Bünzli 2009-03-07 05:32:10 PST
Created attachment 366095 [details] [review]
tests for bugs 459906, 461743, 463205 and 464620

These are the same tests as above but with only the setTimeouts that are  required in order to mirror the same calls in nsSessionStore.js.
Comment 29 Dietrich Ayala (:dietrich) 2009-04-02 09:54:31 PDT
Comment on attachment 366095 [details] [review]
tests for bugs 459906, 461743, 463205 and 464620

i don't have access to bug 461743, so have limited information (only the test!), but everything else looks good, r=me.
Comment 30 Aakash Desai [:aakashd] 2009-07-13 11:14:06 PDT
Created attachment 388243 [details]
Testcase 2 & 3 Results Screenshot

I'm seeing the following behavior show in the screenshot after finishing the testcase steps. This happens on the latest nightlies for trunk, 1.9.1.1, Fx3.0.0.12 for testcases 2 and 3. Is this correct behavior?
Comment 31 moz_bug_r_a4 2009-07-13 23:24:14 PDT
Yes.
Comment 32 Aakash Desai [:aakashd] 2009-07-20 10:51:42 PDT
ok, verifying as FIXED per comment #31. Thanks!
Comment 33 Reed Loden [:reed] (very busy) 2009-09-04 19:05:32 PDT
tests -- http://hg.mozilla.org/mozilla-central/rev/5e8579e1057c
Comment 34 David Baron [:dbaron] 2009-09-10 06:52:38 PDT
It looks like those tests were disabled soon after they were landed:
http://hg.mozilla.org/mozilla-central/rev/e6c77bac4178
http://hg.mozilla.org/mozilla-central/rev/2600b11db971

Should there be an open bug on getting them enabled again?
Comment 35 Reed Loden [:reed] (very busy) 2009-09-10 06:59:30 PDT
(In reply to comment #34)
> It looks like those tests were disabled soon after they were landed:
> http://hg.mozilla.org/mozilla-central/rev/e6c77bac4178
> http://hg.mozilla.org/mozilla-central/rev/2600b11db971
> 
> Should there be an open bug on getting them enabled again?

Bug 514816.

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