464620 - XSS with SessionStore after bug 463205, bug 463206, and bug 461743 are fixed

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[moz_bug_r_a4]

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]

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

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

Attachment: 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 [:dveditz]]

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]

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

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]

Attachment: unbitrotted (replaces all anti-XSS patches that haven't landed yet)

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 348470 [details] [diff] [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]

Comment on attachment 348470 [details] [diff] [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 [:dveditz]]

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

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

Attachment: branch patch (replaces all anti-XSS patches that haven't landed yet)

Comment 13

[Simon Bünzli]

Attachment: branch patch (1.8.1) (replaces all anti-XSS patches that haven't landed yet)

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348470 [details] [diff] [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 [:dveditz]]

Comment on attachment 348609 [details] [diff] [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 [:dveditz]]

Comment on attachment 348610 [details] [diff] [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)]

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

trunk: http://hg.mozilla.org/mozilla-central/rev/8b81d681d1ad

Comment 19

[Dietrich Ayala (:dietrich)]

Comment on attachment 348609 [details] [diff] [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]

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

(In reply to comment #15)
> (From update of attachment 348609 [details] [diff] [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)]

(In reply to comment #16)
> (From update of attachment 348610 [details] [diff] [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]

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

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]

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

All right. You found the bug so I'm willing to believe you. :-)

Thanks for looking into this.

Comment 27

[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/2008120103 BonEcho/2.0.0.19pre.

Comment 28

[Simon Bünzli]

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

Comment on attachment 366095 [details] [diff] [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]]

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

Yes.

Comment 32

[Aakash Desai [:aakashd]]

ok, verifying as FIXED per comment #31. Thanks!

Comment 33

[Reed Loden [:reed]]

tests -- http://hg.mozilla.org/mozilla-central/rev/5e8579e1057c

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

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