634834 - It's possible to call history.pushState/replaceState on a cross-origin page

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

The fix for bug 619359 can be circumvented.  After the security check passed,
when stringifying a state data argument, a new document can be loaded in the
associated window.

Comment 1

[moz_bug_r_a4]

Attachment: testcase - XSS

This tries to get cookies for html5demos.com.
This works on trunk.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Funtimes.

Comment 3

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

sg:crit? if not, doesn't block

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Security bug in a new feature, blocking.

Assigning to blake as wrappers should save us.

Comment 5

[Justin Lebar (not reading bugmail)]

mrbkap and I don't think this is a wrappers problem.

I think I have a simple fix; I'll be able to test it out in a few days.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Justin: Do you think the fix will be risky and need beta coverage? Or would putting it just in the RC be enough.

If it's just touching code run by pushState then I'd think the latter.

Comment 7

[Justin Lebar (not reading bugmail)]

I think it's just a few lines in docshell, to make sure that mCurrentURI after the stringification is same-origin as the new URI.

Comment 8

[Justin Lebar (not reading bugmail)]

You can't just test mCurrentURI, because AIUI that doesn't get updated until after the page gets to its destination.  At least, it doesn't get updated early enough for my test.

Testing that the origins of both mCurrentURI and mLoadingURI don't change as a result of the stringify seems to work.  Instead of a same-origin check, I could test that mCurrentURI and mLoadingURI are the same before and after the stringification.

Any thoughts on which might be better?

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to comment #8)
> You can't just test mCurrentURI, because AIUI that doesn't get updated until
> after the page gets to its destination.  At least, it doesn't get updated early
> enough for my test.

Oh, actually, I think my testcase is broken.  mCurrentURI is good enough for the testcase in this bug.

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Verified that the testcase in this bug fails with a security error.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 513402 [details] [diff] [review]
Patch v1

Sholdn't we be comparing principals instead?

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Like so?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 513518 [details] [diff] [review]
Patch v2

If you have an nsIDocument, just GetPrincipal on it you don't need the global obj.

Other than that, looks fine.

Comment 14

[Justin Lebar (not reading bugmail)]

(In reply to comment #13)
> If you have an nsIDocument, just GetPrincipal on it you don't need the global
> obj.

Really?  Maybe I'm missing some special sauce, but

> nsCOMPtr<nsIDocument> origDocument = do_GetInterface(GetAsSupports(this));
> nsCOMPtr<nsIPrincipal> origPrincipal = origDocument->GetPrincipal();

does not compile.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Er, GetNodePrincipal() on the document.

Comment 16

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/b5fdead607d4

Comment 17

[[e-mail]]

Test fix: http://hg.mozilla.org/mozilla-central/rev/c2b30c18d762