895557 - (CVE-2014-1530) It's possible to set a document's URI to a different document's URI by confusing docshell

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[moz_bug_r_a4]

Bug 803870 was fixed by disallowing cross-origin history access, but the key problem is not fixed and it is still abusable with subframe.

the problem is:

history.forward(); // A
location.hash='a'; // B

1) A sets mLSHE to a SHEntry to be loaded.
2) B sets mLSHE to null and sets mOSHE to a new SHEntry.
3) the history load is processed, but nsDocShell::Embed does not update mOSHE since mLSHE is null.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - fake login page

This works on fx17,22-25.

Target page is https://www.rooms.hp.com/signin.aspx

This creates a fake login page whose URI is the target page's URI. And, if you already saved a password for the target page, the Password Manager will automatically fill in the password.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - XSS

This works on fx17,22-25.

Target page is http://www8.hp.com/us/en/sitemap.html

This injects a script that opens an alert dialog with cookies into the target page.

Comment 3

[moz_bug_r_a4]

Please use the following URL to run testcase 2:

jar:https://bugzilla.mozilla.org/attachment.cgi?id=777994!/x.html

Comment 4

[Bobby Holley (:bholley)]

I think we need justin or bz or someone else who understands docshell to fix this. My involvement in bug 803870 was just at level of cross-origin history access, which we knew wasn't the underlying problem and apparently isn't enough to fix the security bug.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I guess I should take this then.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Olli any sparks?

Comment 7

[Johnny Stenback (:jst)]

Olli, ping? Can you look at this reasonably soon, or do we need to find another owner here?

Comment 8

[Andrew McCreight [:mccr8]]

Olli, any progress on this?  Is there somebody else who can look at this?

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

This is so for me, but sorry no progress. I'll try to look at this soon.

Comment 10

[Andrew McCreight [:mccr8]]

One month reminder! ;) This is the oldest open DOM sec-high or -critical bug.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

So as far as I see this is a regression from bug 791011, but this was in the tree also before
Bug 775009.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Or actually, even the fix for bug 775009 was just missing some cases, so not a regression.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Oh, there is also the fix for bug 757376 involved.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

And bug 737307. Patches to all those had some issues.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP (v2)

Need to go through the tests for those other xss bugs, and session history mochitests.
This is super regression prone stuff, but this should, assuming the correct interpretation, actually bring us closer
to the spec. "cancel any preexisting but not yet mature attempt to navigate the browsing context"

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8375183 [details] [diff] [review]
WIP (v2)

While I'm testing the stuff, does this ring any alarm bells?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I need to still test unload handling. We probably should prevent hash navigation during unload
(follow the spec better there). That doesn't affect to the xss issue here though.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Ah, but we deal with unload already.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8375183 [details] [diff] [review]
WIP (v2)

I believe this patch would break this testcase:

  <a href="#" onclick="location = 'http://mozilla.org'">Click me</a>

which should load mozilla.org.  :(

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8375183 [details] [diff] [review]
WIP (v2)

Bah, this is too strict.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP (v3)

Need to test still other relevant bugs, but something like this should be
right.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8382676 [details] [diff] [review]
WIP (v3)

This seems fairly plausible if it doesn't regress bug 775009.

That said, why do we need the JustStartedNetworkLoad() conditional?  Why can we not unconditionally set lsheForNewlyStartedLoad = mLSHE?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Yeah, we could probably remove that condition.

Bug 775009 as such doesn't regress, but I need to try a bit different testcase which doesn't
rely on history object being accessible cross domain.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v4

I enabled cross domain history and history.back() etc. and tested the other
bugs, and this doesn't regress those.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8383397 [details] [diff] [review]
v4

r=me

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8383397 [details] [diff] [review]
v4

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not very easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I guess the commit message could say something like:
"Make fragment loading work per spec even in case of session history loads".
(because that is another aspect the patch fixes)
I should actually add a testcase for that. It wouldn't reveal anything about this bug.


Which older supported branches are affected by this flaw?
All

If not all supported branches, which bug introduced the flaw?
NA

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch seems to apply with some --fuzz

How likely is this patch to cause regressions; how much testing does it need?
This is regression prone. Needs as much testing time it can get.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Or, I could add the testcase to bug 775009 and make it use todo(), 
and then change that todo() to is() in this bug.

Comment 28

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

Comment on attachment 8383397 [details] [diff] [review]
v4

sec-approval+ for trunk. Please make and nominate Aurora, Beta, and ESR24 patches ASAP after it lands.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Attachment: todo_is() -> is() for the test in Bug 978408

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90fb37782bb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc271727af48

Comment 31

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/90fb37782bb5
https://hg.mozilla.org/mozilla-central/rev/cc271727af48

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8383397 [details] [diff] [review]
v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old stuff
User impact if declined: xss
Testing completed (on m-c, etc.): landed to m-c 
Risk to taking this patch (and alternatives if risky): risky. Needs real world testing. Artificial testcases are not enough.
String or IDL/UUID changes made by this patch: NA

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8383397 [details] [diff] [review]
v4

So we only have a week of "real world testing" left for FF28 - what happens if we don't take this up to beta/esr24 on this release?  Can this wait for a full cycle on Beta with 29?

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

This bug has been open for months, but that actually doesn't matter.
What matters is whether it is easy to construct the exploit from the patch.
I'd say it is not very easy.

Could we re-evaluate beta/esr24 after couple of days testing in m-c?
I'd like to see at least some Nightly testing.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

I folded the test from bug 978408 into the second patch for the branch uplifts.

https://hg.mozilla.org/releases/mozilla-aurora/rev/75ce83516916
https://hg.mozilla.org/releases/mozilla-aurora/rev/4db655b66945

Comment 36

[Lukas Blakk [:lsblakk] use ?needinfo]

It's been a couple of days and our last 28 beta goes to build today - what have the few days on central showed?  What's the risk/reward here for landing at this late point?  If there's risk to stability of the product, it would be best to wait one more cycle.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

I haven't heard of any regressions.

Risk is that some page navigations don't work as expected, so it is not about stability, but
whether web pages work as expected.
Reward is to fix this xss.

I think waiting for one more cycle is ok. Perhaps dveditz has some opinion here.

Comment 38

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

Since it is already a problem on 27, I think we can "won't fix" for 28 and let it ride the trains.

Comment 39

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

Comment on attachment 8383397 [details] [diff] [review]
v4

I would like to see this on ESR24 but I don't want to destabilize it just before release either so I think this should wait and land there after we make release builds.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

v1.1 will be EOL by the time this is approved for esr24, so wontfix for b2g18.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

FWIW, this applies cleanly all the way down to esr24 and is green on Try on all branches. If you want to take this as a Pwn2Own ride-along fix, it shouldn't get in the way.

Comment 42

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

I think the existing reasoning stands and we should probably not take this at the last minute.

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

This is a) only sec-high but also b) carries risk of web incompatibility and/or site breakage so we wouldn't take this as a last-minute landing to esr24 that is shipping with FF28.  It can wait a cycle.

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8383397 [details] [diff] [review]
v4

Will leave the esr24 nomination for uplift until after we're clear of ESR24.4.0

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/8ca0fab764c0
https://hg.mozilla.org/releases/mozilla-esr24/rev/a606379e17c9

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/df2cd7737e7a
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/e6f6438a1732

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/df2cd7737e7a
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/e6f6438a1732

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/cf446bc80f25
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7dd7bac99876

Comment 46

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue on Fx28 using both tests.
Verified fixed on Fx24esr, Fx29 and Fx30.