332182 - [FIX]Consider making about:blank use the parent principal

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I think we should give about:blank the "parent" principal just like we do for javascript: and data: URIs.

This would change the behavior of the following cases:

1) about:blank in a subframe would get the principal of the parent document
2) Clicking on a link to about:blank from document A would give the resulting
   document the principal of document A.
3) window.open("about:blank") calls once the URI loads.

I also think we should make window.open calls in general munge the principal of the opened window _before_ starting the URI load in the case when the opened window has about:blank in it to make that principal be the subject principal.

The net result, if I read our code correctly, is that we should be able to remove the about:blank checks in nsScriptSecurityManager::CheckSameOriginPrincipalInternal, nsPrincipal::Subsumes, nsJSThunk::EvaluateScript.  

The idea is that if I have a subframe that's about:blank and that I fill via DOM manipulation, other sites should NOT be able to access that DOM.  Right now, if they get access to that window, they can access the DOM, as far as I can tell.

Anyone see any obvious problems?  Or know of obvious things to test in a build with this change?

Comment 1

[Brendan Eich [:brendan]]

This is a great idea.  Even Mozilla Classic didn't get this right -- see http://lxr.mozilla.org/classic/source/lib/libmocha/lm_taint.c#1192.

The early access stuff there might shed light on hard cases (or it might be obsolete, from Netscape 4 signed script days).  It suggests multiple principals might be allowed early access to about:blank or similar loaded in a new window or heretofore empty frame-in-frameset.

/be

Comment 2

[Jesse Ruderman]

*** Bug 313070 has been marked as a duplicate of this bug. ***

Comment 3

[Csaba Gabor]

Attachment: Submitting form w. action="about:blank" to iframe

> I think we should give about:blank the "parent" principal
> just like we do for javascript: and data: URIs.

Hurrah!  I've been grousing about this for years on comp.lang.javascript and was just about to enter a report - fortunately, it means I've got a test case ready to go.

> This would change the behavior of the following cases:

> 2) Clicking on a link to about:blank from document A would
> give the resulting document the principal of document A.

Why doesn't it make sense for the new about:blank
to get the principal of the parent document or opener?

> The idea is that if I have a subframe that's about:blank
> and that I fill via DOM manipulation, other sites should
> NOT be able to access that DOM.  Right now, if they get
> access to that window, they can access the DOM, as far as
> I can tell.

> Anyone see any obvious problems?  Or know of obvious
> things to test in a build with this change?

Here's why I'd like this.  I want to be able to submit
my form to an iframe and then be able to process it myself
(via <iframe name=myframe onload="...">).  For example,
there is certain info in the form request that can
otherwise be difficult to get to reasonably (such as
the mouse position on an image click).

So my form's action will be "about:blank" and the resultant url will be about:blank?field1=val1&field2=val2...
(This type of URI works fine for me with Greasmonkey).

The attachment has a form (with one hidden field) and an iframe.  Each time the button is pressed, the form is submitted to the iframe, and it will show Loaded followed by the location.  Only right now, if there's a ? after about:blank, it gets a security error: uncaught exception: Permission denied to get property Location.href

Csaba Gabor from Vienna

Comment 4

[Boris Zbarsky [:bzbarsky]]

Actually, about:blank and about:blank?gunk are different as far as security is concerned....  We could consider doing something like this for the latter, but it would take a lot more thought and care to get right.  So if you care about that, please file a separate bug and make it dependent on this one.

Oh, and there's no need to "sign" your bug comments -- it's already clear who's making the comment.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Need to sort out what the deal is with the various people who load about:blank in an attempt to "clear the decks" security-wise, since that will no longer work.  :(

Comment 6

[Boris Zbarsky [:bzbarsky]]

Testcases to make sure not to break:

https://bugzilla.mozilla.org/attachment.cgi?id=226589

Clicking a link in a gmail message (see bug 342108)

Whatever I end up attaching to this bug.

https://bugzilla.mozilla.org/attachment.cgi?id=199252

http://www.squarefree.com/jsenv/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Basic window-opening testcase

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing with a timeout

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: With timeout and explicit load

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Without timeout, with explicit load

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Without timeout, explicit load for real

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Basic timeout

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Timeout with explicit load

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Testing a frameset

Comment 15

[Boris Zbarsky [:bzbarsky]]

I have a fix.  Testing now.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: This passes all my tests

Basic changes:

1)  Change the handling of mOpenerScriptPrincipal a tad -- set that principal
    on whatever document is in the window we end up opening when the open call
    returns, and use that principal when creating a content viewer due to there
    not being one around at all.  I could kill off mOpenerScriptPrincipal
    completely if we're ok with always forcing document creation in the cases
    when SetOpenerScriptPrincipal is called.  I'm somewhat tempted to do that,
    but would like feedback.
2)  Make subframe about:blank loads inherit the principal from the parent, but
    NOT from the previous document in the same frame (unlike javascript: and
    data:).
3)  Keep track of the "initial" documents in windows so we only special-case
    those for security purposes.
4)  Remove about:blank special-casing in WouldReuseInnerWindow, nsPrincipal,
    and nsScriptSecurityManager.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: With a comment correction

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Tests

The tests need a Firefox (not any other Gecko app) build with jssh.  See http://wiki.mozilla.org/SoftwareTesting:Tools:TestRunnerPython for instructions.

davel, I made some changes to the test harness.  Of particular note, I changed the TextTestRunner() constructor call to use stdout so it interleaves properly with the print() statements elsewhere in the test in the face of output buffering (e.g. when redirecting to a file) and added the loop to do multiple pref values.  As a result of the loop, we don't stop at the first failure but keep trying other pref combinations instead; we could change that, but I think that's acceptable behavior for what we want here.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: With some tweaks to nsJSProtocolHandler too

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 233265 [details] [diff] [review]
With some tweaks to nsJSProtocolHandler too

I mentioned to bz on IRC that the JS protocol handler changes look like they'll regress bug 302618.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233265 [details] [diff] [review]
With some tweaks to nsJSProtocolHandler too

Yeah, need to sort out how to not regress that.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Added a few more tests that the first patch failed

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated patch

This patch passes the updated tests and also doesn't regress bug 302618.  Changes, in addition to the ones listed in comment 16:

5)  Don't use INTERNAL_LOAD_FLAGS_INHERIT_OWNER in nsDocShell::Reload.  We
    really just want to use our existing principal; if we have none we do NOT
    want to inherit from a parent document.
6)  Make GetInheritedPrincipal have two modes -- one for use for loads of
    javascript: and data: and one for use from CreateAboutBlankContentViewer.
    Do not call GetInheritedPrincipal for about:blank anymore -- subframe loads
    have the right owner anyway, and for random nsIWebNavigation callers we
    don't know what principal they want so err on the secure side.
7)  When retargeting the load, don't pass along the
    INTERNAL_LOAD_FLAGS_INHERIT_OWNER (that's just a long-standing bug).
8)  When getting the inherited principal for a javascript: or data: load, try
    to at least create an about:blank content viewer if all else fails.  Note
    that this can also pick up the opener principal as needed.  This change
    refixes bug 302618
9)  Move the call to SetOpenerScriptPrincipal to much earlier in the
    window-open sequence.  Do it for non-JS opens as well as JS ones; use the
    opener window's principal for the former.
10) Don't use object principals for javascript: URIs that have no owner.  Just
    use the null principal.  Callers should pass in owners, dammit!
11) Mark HTML documents that get OpenCommon() called on them as no longer
    initial documents.
12) Remove the code in ScriptWriteCommon() that clobbered the principal with
    the caller's principal.  At this point, being able to write to a document
    means you're either same-origin or have UniversalWhatever bits.  If we want 
    to keep the principal-clobbering behavior for the latter case, let me know
    and I'll see how to work it into the current setup.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

The nsGlobalWindow changes are easier to review here.

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #23)
> 6)  ... have the right owner anyway, and for random nsIWebNavigation callers we
>     don't know what principal they want so err on the secure side.

Is there a bug on file for the random nsIWebNavigation callers not passing the right (or any) principal?  Maybe it's on file as bug 293363.

> 7)  When retargeting the load, don't pass along the
>     INTERNAL_LOAD_FLAGS_INHERIT_OWNER (that's just a long-standing bug).

Thanks for digging into this old code.  It crawls due to bugs standing on it for too long ;-).

> 9)  Move the call to SetOpenerScriptPrincipal to much earlier in the
>     window-open sequence.  Do it for non-JS opens as well as JS ones; use the
>     opener window's principal for the former.

Do you mean the subject principal of the calling script, or the object principal of the window that's the base object of the "open" reference, e.g., w in w.open(u)?  It may be the same in most cases, since open is not allAccess (please confirm), so a subject p that can call w.open(u) implies that w has object principal p.  But with elevated privileges, w could have q, and we would want any pseudo-URI u (javascript:, etc.) to load in w with principal p, not q.


> 10) Don't use object principals for javascript: URIs that have no owner.  Just
>     use the null principal.  Callers should pass in owners, dammit!

Amen.  This may not be covered by bug 293363.

> 11) Mark HTML documents that get OpenCommon() called on them as no longer
>     initial documents.

Can you remind me of why this crazy property is needed to annotate the initial doc?  mrbkap mentioned it yesterday, I boggled, but didn't retain the reason.

> 12) Remove the code in ScriptWriteCommon() that clobbered the principal with
>     the caller's principal.  At this point, being able to write to a document
>     means you're either same-origin or have UniversalWhatever bits.  If we want 
>     to keep the principal-clobbering behavior for the latter case, let me know
>     and I'll see how to work it into the current setup.

This is the p, not q, case I sketched above.  We need the subject principals to taint the created document.  We shouldn't assume that the last document's object principals are the same.

/be

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Is there a bug on file for the random nsIWebNavigation callers 

Not really.  Part of the problem is that they _can't_ pass in a principal -- nsIWebNavigation::loadURI doesn't have an arg for it.  I suppose bug 293363 can cover this...

> Do you mean the subject principal of the calling script, or the object
> principal of the window that's the base object of the "open" reference

The former, then fall back to the latter if the former returns null (no script on stack, or explicit null jscontext pushed, as happens with link clicks and form submits).

> Can you remind me of why this crazy property is needed 

This property is used in two places:
* A Boris-is-paranoid check in SetOpenerScriptPrincipal.  If the document in
  the window is not an "initial" document when this is called, we just bail
  out instead of mutating its principal.  I don't expect this to actually
  happen barring buggy window providers, but I figured it was worth the extra
  safety-check.
* In WouldReuseInnerWindow we only want to reuse if the previous document has
  this property.  I'm really nor sure how to get around this -- we need that
  constraint.

> We need the subject principals to taint the created document. 

Taint, maybe (reset to the meet of the two).  Clobber, no.  Our behavior without my patch is "if the document URI is about:blank, clobber the principals, otherwise do nothing".  This is patently wrong in the new world where about:blank has sane principals, so I just made us consistently do what we did if you called document.open() on a document that wasn't about:blank.

I do agree it would make sense to reset to the meet of the principals on stack and the document principal in this case; I'd be happy to file a followup bug on doing that if you want.

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #26)
> Not really.  Part of the problem is that they _can't_ pass in a principal --
> nsIWebNavigation::loadURI doesn't have an arg for it.  I suppose bug 293363 can
> cover this...

Right.

> > Do you mean the subject principal of the calling script, or the object
> > principal of the window that's the base object of the "open" reference
> 
> The former, then fall back to the latter if the former returns null (no script
> on stack, or explicit null jscontext pushed, as happens with link clicks and
> form submits).

Ok, that makes sense -- the null context means that the user is making a load gesture that should speak-for the loaded doc.

> * In WouldReuseInnerWindow we only want to reuse if the previous document has
>   this property.  I'm really nor sure how to get around this -- we need that
>   constraint.

I understand this case.  I wonder whether we couldn't, some fine day, avoid overloading about:blank by creating windows without content, and use the lack of content loaded in the window to distinguish this case.  Then there's no hazard of inheriting the wrong principal or clobbering the right one in this case.

> > We need the subject principals to taint the created document. 
> 
> Taint, maybe (reset to the meet of the two).  Clobber, no.  Our behavior
> without my patch is "if the document URI is about:blank, clobber the
> principals, otherwise do nothing".

Is ScriptWriteCommon another observation point that faces ambiguity between overwriting (document.open) and self-modifying (document.write from a script tag)?  Overwriting should clobber the principal.

> This is patently wrong in the new world
> where about:blank has sane principals, so I just made us consistently do what
> we did if you called document.open() on a document that wasn't about:blank.

Which is to assume that the writing script's principal is the same as the principal of the document being overwritten?  That seems wrong in the elevated privileges case.

> I do agree it would make sense to reset to the meet of the principals on stack
> and the document principal in this case; I'd be happy to file a followup bug on
> doing that if you want.

The overwriting case should clobber principal, not compute a mixture, unless you know of an exploit based on data from the old doc (the one being overwritten) remaining in the new one, or otherwise mixing.  But any such information leak would be a long-standing bug in the same-origin model, and I know of no such bug.  So I think the problem is that we haven't distinguished overwriting from self-modifying.

/be

Comment 28

[Boris Zbarsky [:bzbarsky]]

> and use the lack of content loaded in the window to distinguish this case

The problem is that various people (like the Firefox UI's tab-creation code) access window.document, which automatically creates a document in the window.  I'm basically using the initialDocument flag to flag documents that weren't "really loaded".

> Overwriting should clobber the principal.

That could be done in OpenCommon() (at the point where we actually blow away the old document), and maybe we should do it (so the code that _used_ to own the document can't access the content we're putting into it).  But is that different from replacing the documentElement via DOM manipulation, conceptually?  If so, how?  I really don't think we should clobber the principal for replaceChild(documentElement)....  No matter what, ScriptWriteCommon is the wrong place for it.

That said, how should we treat a document.write() from origin A in to a document of origin B while the latter is loading (so that it's not clobbered, but the data is just inserted into the parse stream)?  And again, how is this different from DOM manipulation?

> Which is to assume that the writing script's principal is the same 

Yeah, basically.

> The overwriting case should clobber principal, not compute a mixture

I can buy this if we don't preserve the inner window in this case, but again see the above questions.  Again, I'd prefer to do that in a followup bug.

Comment 29

[Brendan Eich [:brendan]]

(In reply to comment #28)
> > and use the lack of content loaded in the window to distinguish this case
> 
> The problem is that various people (like the Firefox UI's tab-creation code)
> access window.document, which automatically creates a document in the window. 
> I'm basically using the initialDocument flag to flag documents that weren't
> "really loaded".

Sounds like this is "as good as it gets", then.

> > Overwriting should clobber the principal.
> 
> That could be done in OpenCommon() (at the point where we actually blow away
> the old document), and maybe we should do it (so the code that _used_ to own
> the document can't access the content we're putting into it).

Yes, if the principals differ.  This seems like another case where, under same origin and without allAccess for document.open.call, some extension of the default security model must be in effect for the principals to differ, e.g., enablePrivilege has been called -- check?  Still seems worth getting the trust label calculus right, not mistaking current- or default-model correlation for causality.

> But is that
> different from replacing the documentElement via DOM manipulation,
> conceptually?

I'm the DOM level 0 perpetrator, and document.open replaces the contents of a window, so it's analogous to loading the window via location.href = ... or a targeted window.open.  This case is clear: clobber.

For DOM level N, I'd like to appeal to the w3c standards, but I bet they say bupkis about security models and access control.

Conceptually, the two cases could differ only if there is a finer granularity of trust containment than the window (frame, iframe) object.  But I do not believe there is any such smaller container, so I would be content if these two levels of DOM clobbered.

> If so, how?  I really don't think we should clobber the
> principal for replaceChild(documentElement)....

Why no?  Just looking for more info, not doubting (except based on trust label pigeon-hole reasoning above).

> No matter what,
> ScriptWriteCommon is the wrong place for it.

Agreed.  Followup bug for sure.

> That said, how should we treat a document.write() from origin A in to a
> document of origin B while the latter is loading (so that it's not clobbered,
> but the data is just inserted into the parse stream)?

That's a same-origin violation unless you suppose that A has enabled privilege. If it has, then we need new policy.  I suspect the Netscape 4 code would clobber B's principal with A's.  I do not remember whether Netscape 3 would compute the meet of A and B -- it might have, it had machinery to do that.

> And again, how is this
> different from DOM manipulation?

It's not, in my book.

> > The overwriting case should clobber principal, not compute a mixture
> 
> I can buy this if we don't preserve the inner window in this case, but again
> see the above questions.  Again, I'd prefer to do that in a followup bug.

Sure, and double-check on new inner window being needed.

/be

Comment 30

[Boris Zbarsky [:bzbarsky]]

> Why no?

Because why clobber for the documentElement but not other elements?  What about scripts included via PI (if we ever support that), which would be outside the documentElement?  And so forth.  In addition to which, none of these cases clear the window scope, unlike document.open.

This last seems like a strong argument for having document.open clobber the principal, for what it's worth.  And guess what?  We do!  It's just a little better hidden, so I didn't see it at first.  ;)  See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.693&mark=1957-1958,1966,1968-1970#1956 and http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/document/src/nsHTMLDocument.cpp&rev=3.693&mark=2001,2007-2009#1997

So things are already more or less OK here, except we clobber even when preserving the inner window.  This can lead to exploits.  For example:

1)  Evilsite creates a blank document (so has "initialDocument" flag set), sets
    some properties on the window (getters, setters, whatever).
2)  Goodsite with privileges is tricked into document.writing to this document.
3)  The properties on that window now have goodsite's principal.

So I feel that whenever we'd preserve the inner window we should clobber with the meet of current and caller, not with the caller, to protect the caller.  Alternately, we should not preserve the inner window on document.open if the caller and current are not sameOrigin.  As in, if that happens, remove the "initialDocument" flag before calling SetNewWindow.

Either solution is secure; which do you prefer?

> That's a same-origin violation unless you suppose that A has enabled 
> privilege.

Sure.  So make that assumption as needed.

> If it has, then we need new policy.

Agreed.  Wiki, right?

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)
> This last seems like a strong argument for having document.open clobber the
> principal, for what it's worth.  And guess what?  We do!  It's just a little
> better hidden, so I didn't see it at first.  ;)

Whew.  Somehow (I wish) it should all be easier to verify by inspection.

> So I feel that whenever we'd preserve the inner window we should clobber with
> the meet of current and caller, not with the caller, to protect the caller. 
> Alternately, we should not preserve the inner window on document.open if the
> caller and current are not sameOrigin.  As in, if that happens, remove the
> "initialDocument" flag before calling SetNewWindow.

Objective is not to use meet-of-principals just because we can (or could -- no mechanism yet, let alone policy).  The right thing, I believe, is to clobber and that means a new inner window.  But we don't want to break the case where props are preset on a brand new window.

> Either solution is secure; which do you prefer?

The backward compatible one ;-).  We will get to use meet (or join, IIRC the literature uses join and of course they are duals) as we evolve toward better security models and information flow tracking (in my opinion).  But for now we really ought to avoid meets, esp. when fixing existing use-cases to be secure.

> > If it has, then we need new policy.
> 
> Agreed.  Wiki, right?

Sure.  Your point about documentElement replacement not being the same as document.open is good, but I don't believe any browser is ready to mix trust labels within a window.

/be

Comment 32

[Brendan Eich [:brendan]]

Of course, we can use null principals as the meet of any two codebase principals, but that's not a useful greatest lower bound.  It's only greatest in our current world because we lack mechanism to do better.

/be

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Clobber the inner on cross-domain document.open

The only difference from the previous patch is that I added the following in nsHTMLDocument::OpenCommon right before calling SetNewWindow:

    if (!callerPrincipal ||
        NS_FAILED(nsContentUtils::GetSecurityManager()->
          CheckSameOriginPrincipal(callerPrincipal, NodePrincipal()))) {
      UnsetProperty(nsGkAtoms::initialDocumentInWindow);
    }

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 233524 [details] [diff] [review]
Clobber the inner on cross-domain document.open

- In nsPrincipal::Equals():

-    nsIURI *origin = mDomain ? mDomain : mCodebase;
-    nsCOMPtr<nsIURI> otherOrigin;
-    aOther->GetDomain(getter_AddRefs(otherOrigin));
-    if (!otherOrigin) {
-      aOther->GetURI(getter_AddRefs(otherOrigin));
-    }
-
-    return nsScriptSecurityManager::GetScriptSecurityManager()
-           ->SecurityCompareURIs(origin, otherOrigin, aResult);
+    *aResult =
+      NS_SUCCEEDED(nsScriptSecurityManager::GetScriptSecurityManager()
+                   ->CheckSameOriginPrincipal(this, aOther));
+    return NS_OK;
   }

As a side note, it seems odd to do the principal equality checks this way. It'd make more sense if the script security manager would ask a principal whether it's equal to another rather than a principal asking the security manager whether the principal itself is equal (with some definition of equal) to another principal. Would be more logical to have this pricipal comparison logic live in the principal code rather than in the script security manager code. Either way, this isn't introduced here per se, so leaving it as is, or filing a separate bug on this is fine with me...

- In nsPrincipal::Subsumes():

 {
-  // First, check if aOther is an about:blank principal. If it is, then we can
[...]
-  }
-
   return Equals(aOther, aResult);
 }

Now that Subsumes() is identical to Equals(), would now be a good time to remove either from the API? Not something we'd want to do for any branches, but maybe for the trunk?

- In nsScriptSecurityManager::CheckSameOrigin():

+    // These booleans are only used when !aIsCheckConnect.  Default
+    // them to true, and change if that turns out wrong.
+    PRBool subjectSetDomain = PR_TRUE;
+    PRBool objectSetDomain = PR_TRUE;

This seems a bit backwards and potentially error-prone to me. I'd rather see these default to false and only set to true when they're really set to true. Pretty trivial in the code as is, but who knows how this code might change down the road and defaulting to the "safer" value seems safer to me.

- In content/base/src/nsGkAtomList.h:

+GK_ATOM(initialDocumentInWindow, "initialDocumentInWindow")

This seems a bit hokey to me (sorry). If this is something we need documents to know I'd much rather see this explicitly expressed in the nsIDocument API, not as a ad-hoc property that's set on the document (if the implementation wants to use a property, fine, but there should be an getter/setter in nsIDocument for this IMO).

- In nsFrameLoader::LoadURI():

+    // XXX bz I'd love to nix this, but the problem is chrome calling
+    // setAttribute() on an iframe or browser and passing in a javascript: URI.
+    // We probably don't want to run that with chrome privileges... Though in
+    // similar circumstances, if one sets window.location.href from chrome we
+    // _do_ run that with chrome privileges, so maybe we should do the same
+    // here?
     loadInfo->SetInheritOwner(PR_TRUE);

I think we should do the same here, yes. But that's a change for a separate bug IMO.

- In nsDocShell::Reload():

+        nsCOMPtr<nsIDOMDocument> domDoc(do_GetInterface(GetAsSupports(this)));
+        nsCOMPtr<nsIDOMNSDocument> domNSDoc(do_QueryInterface(domDoc));
+        if (domNSDoc) {
+            domNSDoc->GetContentType(contentTypeHint);
+        }
+
+        nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));

nsIDocument also has a GetContentType() method in it, so no need to QI the document to nsIDOMNSDocument here since you now QI to nsIDocument anyways.

Other than that this looks really good to me! sr=jst with those issues addressed.

Comment 35

[Brendan Eich [:brendan]]

Subsumes is not the same as equals for all principals.

/be

Comment 36

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to jst's comments

> it seems odd to do the principal equality checks this way.

Agreed, but in general equality and being same-origin are slightly different things...  I do agree that once we move everything to principals instead of URIs we should be able to move the impl into the principal.

> Now that Subsumes() is identical to Equals()

Only for nsPrincipal.  For nsSystemPrincipal they're different.

> This seems a bit backwards and potentially error-prone to me

Good catch.  Fixed.

> I'd much rather see this explicitly expressed in the nsIDocument API,

Done.

> I think we should do the same here, yes. But that's a change for a separate
> bug

Filed bug 348672.

> so no need to QI the document to nsIDOMNSDocument 

Good catch!  Done.

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 233684 [details] [diff] [review]
Updated to jst's comments

This looks awesome.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 39

[Jesse Ruderman]

Do we want to fix this on the Firefox 2 branch?

Comment 40

[Boris Zbarsky [:bzbarsky]]

I personally do not, for several reasons:

1)  It's a web compat change (for the better, imo, but still).
2)  I don't really have the time to port this patch to branch (make it not
    change APIs, etc).
3)  The patch may or may not work on branch as-is -- I have no idea what other
    already-happened trunk work it happens to depend on.  Sorting this out
    would take a lot of time.
4)  We have no guarantee that this patch doesn't break things badly, given the
    current state of trunk testing.  I wouldn't trust it to any sort of stable
    branch.  _Maybe_ if it had gone in for Gecko 1.8.1RC1.  Maybe.
5)  If I spend all my time backporting security arch work from 1.9 to 1.8, I
    won't get anything done for 1.9.  Yes, I know I've already said something
    like this in #2, but I can't stress it enough.

Comment 41

[Boris Zbarsky [:bzbarsky]]

This patch depends on bug 325816.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Sorta-port into JS of that jssh harness

The key here is the pref-setting and the test iterator; this was written to run as a browser test, which is not what we want.

Also, the tests should use the multiple-domain support mochitests have instead of using google.com and the like.

Comment 43

[Eric Shepherd [:sheppy]]

Can someone please explain to me the ways in which this impacts documentation?  I don't really understand what the parent principal is, so it's hard for me to gauge how this needs to be written up.

Comment 44

[Boris Zbarsky [:bzbarsky]]

The main change here is that before this patch any site could access any about:blank document.  After this patch, only the site that created the about:blank document can access it.

Comment 45

[Eric Shepherd [:sheppy]]

There can be more than one about:blank page?  The stuff I learn...

Comment 46

[Eric Shepherd [:sheppy]]

By that I mean that there's no information anywhere on MDC that indicates that about:blank is ever anything but a local file of some kind.  I need to know more about this subject in order to document it properly; any idea where I can get that info?

Comment 47

[Boris Zbarsky [:bzbarsky]]

Basically, about:blank is a placeholder document that we use when there is no other document around.  So for example:

  <iframe></iframe>

with no src attribute set will load about:blank.  Doing:

  window.open()

in JavaScript without specifying a URI opens a window that has about:blank loaded in it.

Something like <iframe src="http://www.mozilla.org/"></iframe> and then accessing the iframe's contentDocument before we've gotten a response from www.mozilla.org will create an about:blank document in that iframe and return that Document object.  When the server responds, we'll load the http://www.mozilla.org/ document in place of the about:blank one.

The issue here was that if, say, one did window.open() and then put some content in that window using DOM APIs (appendChild, innerHTML, etc) then anyone who could get a handle to that window somehow could read that content.

Comment 48

[Eric Shepherd [:sheppy]]

Hopefully this is now adequately covered here:

http://developer.mozilla.org/en/docs/DOM:document#Security_notes

Please re-mark this as doc needed if that text doesn't cover things well enough.

Comment 49

[Boris Zbarsky [:bzbarsky]]

That looks great.  Thank you!