Bugzilla@Mozilla – Bug 464620
XSS with SessionStore after bug 463205, bug 463206, and bug 461743 are fixed
Last modified: 2010-02-13 12:24:35 PST
Summon comment box
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.
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.
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.
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.
mrbkap, jst: is there some deeper fix we can do here to get out of this whack-a-mole with session-restore?
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.
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.
Created attachment 348470 [details] [review] unbitrotted (replaces all anti-XSS patches that haven't landed yet)
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 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.
(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?
(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.
Created attachment 348609 [details] [review] branch patch (replaces all anti-XSS patches that haven't landed yet)
Created attachment 348610 [details] [review] branch patch (1.8.1) (replaces all anti-XSS patches that haven't landed yet)
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 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 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
simon, when you make a patch for the test for this, can you also include the tests from the other bugs as well?
trunk: http://hg.mozilla.org/mozilla-central/rev/8b81d681d1ad
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
(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.
(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)
(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
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.
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.
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.
All right. You found the bug so I'm willing to believe you. :-) Thanks for looking into this.
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.
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 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.
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?
Yes.
ok, verifying as FIXED per comment #31. Thanks!
tests -- http://hg.mozilla.org/mozilla-central/rev/5e8579e1057c
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?
(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.