397828 - Make window.document not be allAccess

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

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

This should allow for a lot of security improvement since there would no longer be a way to hold a reference to a document from another domain. Though we'd need to also fix at least <iframe>.contentDocument too to deny accecss to documents from another origin.

This shouldn't break too many sites as microsoft already made the same change in IE7.

We should also make document.open not be allAccess as you shouldn't be able to get to such a document anyway.

Further, if we did this we could possibly make it so that document.open() wouldn't actually change the principal of the document it's called on. Though just to be safe we should add checks that made sure that the subject principal subsumes the object principal and bailing if that is not the case.

Comment 1

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

Blake, at the very least, could we simply make the pref-change and change frame.contentDocument here?

bz: are there more APIs that you know of that could return cross-origin documents?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Maybe some event targets in some cases?  Probably not, though.  So no, not that I know of.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, correction.  As things stand, XMLHttpRequest.  But the access check story there is all screwed up anyway.

Also, a document can _become_ cross-origin even if it didn't start that way as a result of XMLDocument::Load(), no?

Comment 4

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

Aw crap, forgot about document.load. But really, can't we keep the principal there too since we don't allow cross-origin loads (yet)?

What i'm trying to accomplish is that we never change the principal of an existing document. I'm not sure if there is that much value in that? Or if it's not really doable anyway?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #4)
> I'm not sure if there is that much value in that? Or if it's
> not really doable anyway?

The value would be that we wouldn't have to wrap documents in XOWs.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Do we want to never change the principal?  Or never change the origin?

We use principals for things like base URIs too in some cases, I believe...  In fact, document.open might be such an example.

I think not changing the origin is quite feasible.  We'd have to sort out what happens in the "subsumes but not same origin" case in terms of base URIs, and such.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Sorry, I was referring to never changing the origin.

Comment 8

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

I was actually hoping to not change principal even, but that might be too much to hope for. The reason was that I had some vague recollection of bugs where we checked the principal in one place but used the object later, thereby opening up some sort of vector of attack. But it's entirely possible that it's not a real win in keeping the principal, and not changing origin would be quite enough.

In any case, we should do the best we can and eventually we'll see if there are security checks we can remove.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Blake and I talked a bit... Note that .domain changes the origin without changing the principal object.  Any proposed setup has to handle that.

Comment 10

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

Blake, is this just a matter of flipping the pref? Do we have a pref for iframe.contentDocument?

Comment 11

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

Attachment: Patch to fix

This seems to do it. I tested a few sites, including google maps, google calendar, and some news sites, and nothing seemed to be broken.

I'm sure we'll break a few sites, but I think it might be worth it given the security win here.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 290938 [details] [diff] [review]
Patch to fix

- In nsHTMLDocument::OpenCommon():

NS_ASSERTION(nsContentUtils::CanCallerAccess(static_cast<nsIDOMHTMLDocument*>(this)),
>+               "XPConnect should have cought this!");

Typo, "caught", and given that our scriptable helpers override the XPConnect method, it's XOW that should catch this.

r+sr=jst

Comment 13

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

Seems like somehow this patch caused documents to not be bfcached

Comment 14

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

test_bug310107.html failed with the "Prop still 2 because we're in bfcache" test failing. And a pile of chrome tests got the wrong events (load instead of pageshow etc)

Comment 15

[Boris Zbarsky [:bzbarsky]]

That's odd.  Can you step through CanCachePresentation and see what's up?

Comment 16

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

The extra weird thing is that this seemed to only happen on the mac tinderbox. However I don't fully trust the times on tinderbox so it's possible that the windows and linux boxes didn't have the patch.

Comment 17

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I just hit the same issue, also on only the Mac box, when I checked in the patch for bug 401496....

I wonder what the deal there is.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Er, bug 401946

Comment 19

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

So it seems like that box simply randomly fails to use fastback :( I filed bug 406532 for that.

Marking this bug fixed.

For the record, we are aware that this might break some sites. However IE7 has already made the same change so hopefully very few sites are still relying on this. The reason we're doing this is extra levels of security. We have had bugs relating to the now disabled functions in the past, and it's possible that it could happen in the future.

Comment 20

[Eric Shepherd [:sheppy]]

added a note to http://developer.mozilla.org/en/docs/DOM:window.document mentioning this change.

Comment 21

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

Could you also add a note to
http://developer.mozilla.org/en/docs/DOM:document.open saying that this method is as-of-firefox3 subject to the same same-origin policy used for other properties. Partially to make it easier to find, partially because you can actually still get hold of another windows document using iframeelement.contentDocument, and so the call to .open might be the one that actually fails.

Comment 22

[Eric Shepherd [:sheppy]]

Done.

Comment 23

[Rick Curry]

This fix will cause window.document to always fail permission when reading a local html file.  That is, if document.domain is the empty string, window.document will fail for all newly created windows.  This is not an issue if the file is read through a local server (i.e. http://127.0.0.1/...).  It is only an issue when the html is read as a file (i.e. file:///...).

This occurs on windows and linux machines.

Since it appears that nsContentUtils::CanCallerAccess(frameElement) is the call which is failing for local files, this issue may affect other aspects of DOM when using local files.

Comment 24

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

Please file a separate bug and mark it blocking this one (that's how we track regressions in bugzilla). And attach a testcase to that bug.