Set correct loadingPrincipal and loadingNode in docshell

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Security
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: tanvi, Assigned: tanvi)

Tracking

35 Branch
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46- affected, firefox48 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(8 attachments, 28 obsolete attachments)

1.36 KB, patch
francois
: review+
Details | Diff | Splinter Review
3.16 KB, patch
sicking
: review+
Details | Diff | Splinter Review
14.42 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
2.67 KB, patch
ckerschb
: review+
tanvi
: review+
Details | Diff | Splinter Review
3.58 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
5.08 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
6.95 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
3.91 KB, patch
tanvi
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
This bug is to complete the items listed in this bug comment:
https://bugzilla.mozilla.org/show_bug.cgi?id=1083422#c92
(Assignee)

Comment 1

4 years ago
Created attachment 8533260 [details] [diff] [review]
Bug1105556.patch

There are two main issues to fix in this bug:

1) When we create the loadInfo for a web socket channel, we use the triggeringPrincipal as the requestingPrincipal if there is no document.  This shouldn't happen anywhere.  We need to be able to trust that the requestingPrincipal does not fallback to the triggeringPrincipal.  In other parts of the code where i) a trigger principal exists and ii) we have no document, we assert that the principal is system.  I try and do the same here.

However, we end up with a problem for shared workers.
https://mxr.mozilla.org/mozilla-central/source/dom/workers/test/test_webSocket_sharedWorker.html?force=1
https://bugzilla.mozilla.org/show_bug.cgi?id=1090183

Here we don't have a document and the principal is not system.  How should we handle this case?  Is there a way to detect that the web socket connection is coming from a shared worker?  What should the requestingPrincipal be in that case?

2) When we create loadInfo in nsDocShell, we use the triggeringPrincipal if there is no document.  Again, this shouldn't happen anywhere.  However, the approach we have taken in other parts of the codebase doesn't work here.  We cannot asset that the principal is system, since the triggeringPrincipal may be another document that triggered the load of a new document or it could be a referrer.

If we are in nsDocShell::DoURILoad() and we do not have a document yet, can we assume that the requestingPrincipal is system and explicitly set it that way?  This should only occur in top level TYPE_DOCUMENT loads, since frames will have a requestingNode.  Can we say that top level loads are requested by system?  I spoke to Jonas a bit about this and Boris, he would like your opinion.

Attached is a patch that implements the above.
Flags: needinfo?(bzbarsky)
> Here we don't have a document and the principal is not system.

Wouldn't this be a problem for workers in general?  This doesn't seem to be restricted to shared workers.

Though I guess for dedicated workers started (transitively) from a document you can associate the load with a document if you try hard enough and are willing to force activity for all loads to happen on the main thread...

> How should we handle this case?

The only sane handling is to make the requestingPrincipal be the triggeringPrincipal in this case.

> Is there a way to detect that the web socket connection is coming from a shared worker?

If there's no document involved?  How are we getting the document (if at all) for dedicated workers right now in this case?

> If we are in nsDocShell::DoURILoad() and we do not have a document yet, can we assume
> that the requestingPrincipal is system and explicitly set it that way? 

No.  It sorta-happens-to-be-true in desktop Firefox because of the way the XUL stuff interacts with new tabs/windows it creates, I think, but I wouldn't be surprised if this were not the case in other cases (e.g. Fennec) for something like window.open().

Furthermore, I don't think in the docshell case it makes any sense to use whatever is currently loaded in the docshell as any sort of requesting principal at all.  Consider this testcase, loaded from foo.com:

  <script>
    window.open("http://bar.com/", "something");
  </script>
  <a target="something" href="http://baz.com">Click me</a>

In this case, the triggering principal when the link is clicked is "http://foo.com", right?  But that should, imo, also be the requesting principal, because using "http://bar.com" for the "requesting principal" in this case is a total lie.

In my opinion, for toplevel loads in docshells the requesting principal should in fact be the triggering principal.
Flags: needinfo?(bzbarsky)
> In my opinion, for toplevel loads in docshells the requesting principal
> should in fact be the triggering principal.

Top level loads are a complicated, so I'm glad we're discussing it.

The most important part I think is that you can tell top-level loads from any other loads, which I think is possible through the contentPolicyType, right? As long as that's the case it's at least possible to implement any desired policy. Though it of course requires that you remember to look for it.

Tanvi's proposal was to use the system principal as the requesting principal for top level loads. Using the system principal here definitely seems scary. But at the same time it does sort of seem accurate since CheckLoadURI checks aside (which should be done against both triggeringPrincipal and loadingPrincipal), you should be allowed to load any URI as a top-level load.

But again, I agree that it feels risky.


I would be fine with using the triggeringPrincipal as requestingPrincipal. It seems like that might force people to write special code to make exceptions for top-level loads more often, but at least it'd likely fail safe if they don't.


Another option is to leave the requestingPrincipal as null in this case. We could even hard-code this into the nsLoadInfo implementation. That makes it even more likely that people will have to write exceptions for top-level loads. But it likely also makes it more likely that people will discover that they need to.
> Tanvi's proposal was to use the system principal as the requesting principal

This scares the crap out of me.  Can we please clearly define what we mean by "requesting principal" again?  Because clearly it's not what I thought it was....

> you should be allowed to load any URI as a top-level load.

Hmm.  Is that true?  Should a page that opens a popup be allowed to load things in there that it can't load in subframes?  Why?
(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #4)
> > Tanvi's proposal was to use the system principal as the requesting principal
> 
> This scares the crap out of me.  Can we please clearly define what we mean
> by "requesting principal" again?  Because clearly it's not what I thought it
> was....

We normalized the name to be "loading principal" everywhere now. Docs are here:

https://hg.mozilla.org/integration/mozilla-inbound/file/3f4166fb5e37/netwerk/base/public/nsNetUtil.h#l212

But I agree that it scares me.

> > you should be allowed to load any URI as a top-level load.
> 
> Hmm.  Is that true?  Should a page that opens a popup be allowed to load
> things in there that it can't load in subframes?  Why?

The two main differences from a security point of view are:
* For a popup the user can always see the URL of the loaded page. (Which I agree is a generally weak
  protection against anything)
* The initiating page can't render stuff on top of the loaded page, and so can't launch clickjacking
  attacks against it.

So x-frame-options code can avoid worrying about who loaded the contained frame. And CSP code doesn't worry about inheriting from parent documents (I think CSP otherwise inherits from parent same-origin frames).

That said, I am really not certain what the right thing to do here is. Comment 3 was not at all intended as rhetorical questions, but rather questions that I genuinely don't know the answer to. So I'd love your input on what you think the right thing to do is.

Generally, it seems to me like there isn't a correct "loadingNode" or "loadingPrincipal" for top-level loads. I.e. the resulting resource isn't loaded "into" any other document at all. I.e. it seems like any code that implements some security policy and looks and the loadingNode or loadingPrincipal of a top-level load is doing the wrong thing.

For document loads, what you probably most often care about is "who caused this document to get loaded", since the loadingPrincipal doesn't really get access to the resulting resource if it's a cross-origin load. This is slightly less true for iframe loads since the loadingPrincipal does get the ability to launch clickjacking and potentially other attacks against the resulting document. But it still seems like generally the loadingPrincipal is much less important than for a CSS or JS load for example.

So the null option seems interesting to me. Since this forces you to look at the triggeringPrincipal for top level loads.

The main downside that I can see is that it *might* be interesting for a security policy to know which exact document cause a top-level page to be loaded. But we only have a triggeringPrincipal, not a triggeringNode.

Thoughts?

I'm also happy to jump onto vidyo to discuss.
> We normalized the name to be "loading principal" everywhere now. Docs are here:

I see.  Which is the principal we plan to use for content policy checks?  Presumably the triggering principal?  Same for CheckLoadURI checks?

If so, then it seems like "loading principal" is not exposed to addons at all for now, except via the new loadInfo setup, right?

The terminology here is slightly unfortunate, because there's other code in the tree that seems to use aLoadingPrincipal to mean what "triggering principal" means in loadinfo.  For example, various nsContentUtils methods.  We need to align the terminology here, and we need to do it ASAP before people start passing the system principal around where they shouldn't.

In addition to that, the comments currently in nsNetUtil.h should be improved to make it clear that toplevel loads are system and need to be present in nsILoadInfo.idl, because reading the IDL it's not at all obvious that loadingPrincipal can be system in all sorts of random cases.  In fact, the "responsible for the load" language in the IDL there is _really_ misleading.  The nsNetUtil documentation explains the meaning of loadingPrincipal in a somewhat clearer way.

If and only if all of that happens happens, I'm probably OK with using the system principal here, since then at least it will be clear from the documentation what that means, exactly.

That said, I just audited the existing consumers of loadingPrincipal.  They are:

1)  Performance timing code that I _think_ never gets it for toplevel document loads.  It also only uses it in Equals() nsIPrincipal checks, so at least it would fail-safe for a system principal.

2)  Docshell and security manager code that uses it for nsNullPrincipal::CreateWithInheritedAttributes in the sandboxed case.  What exactly is supposed to happen here for a popup from inside a sandboxed iframe (which is therefore itself sandboxed)?  Should it inherit attributes (whatever those are) from system, from the iframe that opened the popup, from nowhere, or something else?

3)  Something in the CSP service to do with redirects.  What should happen here in cases when the new toplevel thing is supposed to share the CSP of the page that opened it (e.g. it's a blob: or javascript: URL)?  I guess we just need to move the CSP out of principals and directly onto Document and LoadInfo, but until we do that, using the loadingPrincipal here seems broken if we start using the system principal for loadingPrincipal... or if we start using null, for that matter.  And this is NOT failing safe afaict, since the failure is to not get the CSP we should get.

4)  Something in the mixed content blocker.  I think this part is ok.

5)  IPC code in FTPChannelChild/HttpChannelChild/WyciwygChannelChild, which calls it "requestingPrincipal", not "loadingPrincipal".  We should fix that.

6)  Sending of referrers in HttpBaseChannel::SetReferrerWithPolicy.  This relies on extracting a URI from the "loadingPrincipal", so would fail for both null and system.  The failure mode is to assume cross-origin referrer behavior, so would likely break what referrers we send for popups.  This is clearly wrong and needs to be fixed somehow.

7)  Something with redirect timing in HttpBaseChannel.  Again, this would fail closed, but I don't know whether this ever happens for toplevel document loads...

So we should fix up these issues as well if we want to make changes here, no matter whether we change to system or null.

> I'm also happy to jump onto vidyo to discuss.

I'm happy to do that too, come January, if it's still needed at that point.
(In reply to Boris Zbarsky (Vacation Dec 15-31) [:bz] from comment #6)
> > We normalized the name to be "loading principal" everywhere now. Docs are here:
> 
> I see.  Which is the principal we plan to use for content policy checks? 
> Presumably the triggering principal?

I think that depends on the callsite. For nsIContentPolicy implementations I think we're going to try to do our outmost to keep backwards compatibility and pass whatever is currently passed. Ideally by checking the contentpolicytype on the loadinfo and then depending on the type pass different values to nsIContentPolicy.

That has been the plan for the 'context' argument to nsIContentPolicy, so we should probably use that plan too for the principal argument.

> Same for CheckLoadURI checks?

CheckLoadURI checks should be done on both principals I would imagine.


> In addition to that, the comments currently in nsNetUtil.h should be
> improved to make it clear that toplevel loads are system and need to be
> present in nsILoadInfo.idl,

Again, no decision has been made to use the system principal. I'm still looking to hear your thoughts about the three options from comment 3.

> because reading the IDL it's not at all obvious
> that loadingPrincipal can be system in all sorts of random cases.  In fact,
> the "responsible for the load" language in the IDL there is _really_
> misleading.

Suggestions welcome. But I'd really rather first that we actually decide on one of the options in comment 3. That hasn't happened yet.


> That said, I just audited the existing consumers of loadingPrincipal.  They
> are:

This is very useful. But I would also expect that we'll end up with a lot more uses of nsILoadInfo over time.

I'll leave to Tanvi and Christoph to comment on the current consumers.
OK, let's think about the three options from comment 3.

It seems like the desired semantics of loadingPrincipal are basically such that for a toplevel load the concept simply makes no sense.

This means that consumers may need to be able to tell that a load is toplevel and the loadingPrincipal needs to be ignored (e.g. I bet the referrer code is in that situation).  But what would consumers do in that case?  Should they do no test at all (this is the mixed content blocker situation, afaict).  Or should they do the test on the triggeringPrincipal (I bet this is the referrer situation right this second)?  Something else?

Right now it seems like the behavior of loadingPrincipal is designed around the needs of things like the mixed content code: it's set to system for toplevel loads so you can just do a security check and have it pass and not block the load.

If we think all consumers will want such behavior, then it's OK to make it system.

If we think all consumers will want to treat loadingPrincipal the same as triggeringPrincipal for toplevel loads (I suspect this is not true for mixed contnet blocking), then we want to set loadingPrincipal to trigeringPrincipal.

If we think consumers will differ in what they want, then we need a way to communicate to them that this is a toplevel load, either by setting the loadingPrincipal to null or via an explicit flag on LoadInfo that basically says consumers should not look at the loadingPrincipal if that flag is set.  Probably combined with asserts that no one gets it in that case...

My personal preference in the third situation would be to just use a null loadingPrincipal to indicate a toplevel load.
(Assignee)

Comment 9

4 years ago
Regarding the 3 proposals in comment 3:
* setting the loadingPrincipal to the triggeringPrincipal is not the right option.  I'd really like to keep these two things separate.
* setting the loadingPrincipal to the systemPrincipal for top level loads is scary.  For CSP and MixedContentBlocker, we check for contentpolicytype TYPE_DOCUMENT and return early, so we will never actually look at the loadingPrincipal.  But for other security checks, that may not be the case.
* setting the loadingPrincipal to null seems like the best option, but it will add some complexity to loadInfo.  We can pass null to loadingPrincipal in docshell.  In loadInfo, we will have to make the asserts for loadingPrincipal conditional on !TYPE_DOCUMENT.

I'd like to look into the CheckLoadURI call sites to see what principal they are using currently.

I'll also need to look into the consumers of LoadInfo and how this affects them, as Boris suggested.  Perhaps Christoph can look at #3 in comment 6 and see how CSP handles this case today.
(Assignee)

Updated

4 years ago
Summary: TriggeringPrincipal followup items and setting correct requestingPrincipal in docshell → Setting correct loadingPrincipal and triggeringPrincipal in docshell
(Assignee)

Comment 10

4 years ago
Did some testing to figure out what the current behavior is for top level document loads[1] in the non-e10s case.  Here are the results:


The loadingPrincipal for a top level loads is system.  The triggering Principal varies depending on how the page is loaded.  What is particularly interesting is that if a user types in a location and navigates a tab, it gets a nullPrincipal for the trigger (from aOwner).  But if the user launches a bookmark  to that same location or hits refresh, there is no aOwner and it gets the systemPrincipal as the trigger.


When opening a new blank tab
* loadingPrincipal is systemPrincipal - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is set to systemPrincipal (no aOwner)

When typing into the address bar to navigate a tab

* loadingPrincipal is systemPrincipal - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is nullPrincipal - comes from aOwner

When launching a bookmark OR refreshing a page.
* loadingPrincipal is systemPrincipal - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is set to systemPrincipal (no aOwner)

Clicking a link in one document to navigate the tab to another document
* loadingPrincipal is systemPrincipal - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is the principal of the original document where the click occurred - comes from aOwner

Frames:
* loadingprincipal is the principal of the parent document - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is the principal of the parent document - comes from aOwner

Navigating frames
* loadingprincipal is the principal of the parent document - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is the principal of the original iframe - comes from aOwner

Load associated with a window.open
* loadingPrincipal is systemPrincipal - comes from mScriptGlobal->GetFrameElementInternal
* triggeringPrincipal is the principal of the original document that called window.open - comes from aOwner

The only time we used GetExtantDoc instead of GetFrameElementInternal was when we launched chrome://global/content/viewSource.xul. for a view-source load.

[1] https://etherpad.mozilla.org/docShellPrincipals
(Assignee)

Comment 11

4 years ago
Also see bug https://bugzilla.mozilla.org/show_bug.cgi?id=1113196 for the e10s case.  There, we don't have the the frame element in the child process, so we fall to GetExtantDoc, which seems to return the existing document.  We need to fix the behavior in general and make sure it's consistent for e10s and non-e10s.
See Also: → bug 1113196
(Assignee)

Updated

4 years ago
See Also: → bug 1066868
(Assignee)

Comment 12

4 years ago
Jonas and Boris spoke today and came up with the below proposal for Top level loads:
loadingPrincipal:
All toplevel loads get "null" as the loadingPrincipal (regardless of how the load was initiated)

triggeringPrincipal:
* The triggeringprincipal for toplevel loads that are triggered from a webpage behave like normal. I.e. the page calling window.open or that contains the <a href=... (target=...)> is used as triggeringPrincipal

* For bookmarks and urls that users type into the address bar (which always load in toplevel) attempt to create a nsIPrincipal from the bookmark/typed URL and use that principal as triggeringPrincipal.  The goal is:
** http(s) and ftp URLs should get a normal codebase principal. (Note that we also discussed using null here, but Jonas was in favor of using the codebase principal.  That way addons won't end up in situations where they have a null loading and triggeringPrincipal, and don't know whether to allow or reject the load.)
** chrome:// (and maybe resource:// ?) URLs get a systemprincipal
** data: URLs get a nullprincipal (does checkLoadURI(nullprincipal, data:-uri) fail?)
** javascript: use the principal of the current page. This one likely needs some form of special-casing. This is needed to get bookmarklets to work. Unclear if the specialcasing should be based on the "javascript" scheme, or some protocol flag. Look at what other code does.
* The code that handles aLoadType == LOAD_NORMAL_EXTERNAL likely does everything we need to safely treat things like desktop shortcuts and commandline options as normal bookmarks.
(Assignee)

Updated

4 years ago
Blocks: 1113196
(Assignee)

Updated

3 years ago
Blocks: 1143922
(Assignee)

Updated

3 years ago
Depends on: 1066868
(Assignee)

Updated

3 years ago
Depends on: 1198559
(Assignee)

Updated

3 years ago
Blocks: 1042699

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 13

3 years ago
Updated etherpad's with current docshell behavior:
non-e10s: https://etherpad-mozilla.org/docShellPrincipals2
e10s: https://etherpad-mozilla.org/docShellPrincipals-e10s
(Assignee)

Comment 14

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #13)
> Updated etherpad's with current docshell behavior:
> non-e10s: https://etherpad-mozilla.org/docShellPrincipals2
> e10s: https://etherpad-mozilla.org/docShellPrincipals-e10s

Updated post change to public etherpad:
https://public.etherpad-mozilla.org/p/docShellPrincipals2
https://public.etherpad-mozilla.org/p/docShellPrincipals-e10s
(Assignee)

Comment 15

3 years ago
I have been investigating the following in nsDocShell InternalLoad() and DoURILoad():
* nsISupports requestingContext that is passed to Content Policy API (most often a Node)
* nsIPrincipal requestingPrincipal that is passed into Content Policy API
* loadInfo->loadingNode and the loadInfo->loadingPrincipal (which = loadInfo->LoadingNode->NodePrincipal())
* loadInfo->triggeringPrincipal


I find that the requestingPrincipal used in the Content Policy API is the triggeringPrincipal.  While in calls to NS_CheckContentLoadPolicy that get invoked through AsyncOpen2(), we use the loadingInfo->loadingPrincipal.  We need to decide what we want to do here:

1) Continue to use the trigger/referrer for TYPE_DOCUMENT calls to Content Policy.  All other content types result in passing loadingPrincipal to Content Policy.
2) Use the loadingPrincipal for all calls to Content Policy.  The trigger would only be used in cases where we have to fallback when no loadingPrincipal exists.
3) Use trigger/referrer for all calls to Content Policy.

We need to be careful that we don't break security checks and that we don't break addons.

1) seems like the safest thing to do, because it maintains existing behavior.  Addons won't break.  But it is confusing for someone who implements a content policy - if a subresource is loading, the principal passed in is the document; but if a document is loading, the principal passed in is the referring document.
2) Using the loadingPrincipal everywhere might break addons, since some may be depending on the referrer for top level loads (example policy: only navigate to bank.com from bank.com or a search site.  don't navigate to bank.com from anywhere else?).  The referrer is also a nice piece of information for a content policy to have; even if we can't think of great use cases for it today, that may change.
3) There are cases where no requestingContext is passed to content policies.  In those cases, the requestingPrincipal is all we have to go on.  If this is the trigger instead of the loadingPrincipal, it could break both gecko content policies and addons.

None of these options are great in my opinion.  It would be nice if we could add the triggeringPrincipal as an argument to nsIContentPolicy, but changing that API is a project in itself.
(Assignee)

Comment 16

3 years ago
Note that the behavior for requestingPrincipal passed to Content Policies for top level loads is already inconsistent...
if a user types a url in the to address bar, we pass nullPrincipal.
if a user clicks a link in their history, we pass systemPrincipal.
if a user refreshes a page, we pass nothing.

The principal generally comes from aOwner.
(Assignee)

Comment 18

3 years ago
Proposed implementation for nsDocShell::InternalLoad and nsDocShell::DoURILoad() below.  I will start implementing next week, push to try, and see what happens.  BZ, can you take a look at this?

requestingContext passed to Content Policies:
* Use mScriptGlobal->GetFrameElementInternal.  If we can't get an element, use ToSupports(mScriptGlobal)[1].
* No change from existing behavior
* Note that this could result in a xul frame element.  We could consider passing null for TYPE_DOCUMENT, but that may break addons.  We could change this if and when we change the content policy API.

requestingPrincipal passed to Content Policies:
* if aOwner exists, use it for top level loads (TYPE_DOCUMENT); this is the trigger.  If aOwner doesn't exist, fall back to aReferrer[2] if it exists.  Otherwise pass nothing (null but not nullPrincipal)
* use GetFrameElementInternal for subdocuments (TYPE_SUBDOCUMENT). This should give you the parent documents principal.  Add an assertion[3]
* Changes existing behavior

loadInfo->loadingNode
* null for top level loads (TYPE_DOCUMENT)
* mScriptGlobal->GetFrameElementInternal() for TYPE_SUBDOCUMENT.  Add an assertion[3]
* No more use of GetExtantDoc()
* Changes existing behavior

loadInfo->loadingPrincipal
* loadInfo->loadingNode ? loadInfo->loadingNode->NodePrincipal : null (null not nullprincipal)
* This will result in null for TYPE_DOCUMENT loads and should always result in an actual principal for TYPE_SUBDOCUMENT.
* I don't think we need to fall back to triggeringPrincipal as we do today.    We should never have a subdocument with no parent.
* Changes existing behavior.

loadInfo->triggeringPrincipal
* As described in comment 12 - https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c12 - though we may need to revisit this.
* I'm going to start off the other pieces first and look closer at how to fix triggeringPrincipal later.  It is already wrong (bug 1181370) so leaving it wrong while we fix up the other stuff.

[1] Q. What are some cases where we can't get an element from mScriptGlobal?
A. Show History, browser startup - these are chrome:// loads

[2] Q. What are some cases where we fall back to aReferrer because aOwner doesn’t exist?
A. When we Refresh a page / Enable/Disable mixed content protection / view source on a page and the initial page load had a referrer.  In those cases, aOwner doesn’t exist anymore.  If the original page load came from somewhere (i.e a link click from a previous page), then aReferrer exists and we can use that.  Otherwise, we don’t have a Referrer either and end up sending null.

[3] Where we call GetFrameElementInternal for TYPE_SUBDOCUMENT, we can assert that the return element comes from a docshell that has the same type as the current docshell.  (Note that we can't do that when we call GetFrameElementInternal() for requestingContext, where we are calling GetFrameElementInternal for documents and subdocuments.)
Flags: needinfo?(bzbarsky)
I'll try to get to this on Monday.
> We could consider passing null for TYPE_DOCUMENT, but that may break addons.

Won't they then break with e10s anyway?  In e10s, places that are TYPE_DOCUMENT will typically return null for mScriptGlobal->GetFrameElementInternal().  Except the ones that run in the chrome process, of course, which will _really_ confuse things...

> [1] Q. What are some cases where we can't get an element from mScriptGlobal?

(1) Any toplevel webpage load in e10s mode.  (2) Loads of browser.xul and similar toplevel XUL things.  I think that should be about it.

> requestingPrincipal passed to Content Policies:

> [2] Q. What are some cases where we fall back to aReferrer because aOwner doesn’t exist?

Assuming you mean in nsDocShell::InternalLoad, I think this is basically cases in which the load comes from the Firefox UI passing us a string, I would think.  In all other cases, and in particular for all loads triggered from inside Gecko itself, I expect aOwner to exist.

> A. When we Refresh a page

This should have a usable owner normally.  In particular, nsDocShell::Reload normally goes through LoadHistoryEntry, which will grab the owner from the session history entry, no?  If we're not always storing an owner in the session history entry then we should just fix that.

That leaves the case of Reload when there is no mOSHE/mLSHE.  But even then we grab a principal from GetDocument() if there is one.  The only remaining case is Reload() when no mOSHE, no mLSHE, and no document.  Is that ever hit in practice?

> Enable/Disable mixed content protection

Is this coming through Reload, or taking some other codepath?

> view source on a page

This one might need some thought, but see below.

> In those cases, aOwner doesn’t exist anymore. 

Why can we not use as owner the triggering principal of the loadinfo for the page being reloaded or view-sourced?  No need to guess based on referrer.

As far as requestingPrincipal passed to content policies, the fundamental question to ask yourself is what should happen when script running in origin A sets window.location on a subframe whose parent document has origin B.  Your proposal is to use B as the requestingPrincipal in this case.  I sort of feel like it should be A.  That is, it seems to me like requestingPrincipal should be aOwner if aOwner is not null, period....

> loadInfo->loadingNode

This one sounds good to me.

>loadInfo->loadingPrincipal

Seems reasonable if null is an OK value here.

>loadInfo->triggeringPrincipal

I would like to understand under what conditions we would ever want this to differ from the "requestingPrincipal passed to Content Policies".  In other words, why wouldn't we use comment 12 for the requestingPrincipal too?

Thank you for digging into this, and sorry for the horrible lag.  :(
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #20)
> >loadInfo->triggeringPrincipal
> 
> I would like to understand under what conditions we would ever want this to
> differ from the "requestingPrincipal passed to Content Policies".  In other
> words, why wouldn't we use comment 12 for the requestingPrincipal too?

Our goal has been to make loadInfo->triggeringPrincipal be as consistent, useful and understandable as possible. Without any constraints regarding existing code, since loadInfo->triggeringPrincipal is still a pretty new concept. Comment 12 was our best attempt at that.

With the requestingPrincipal in nsIContentPolicy we need to additionally need to consider that existing addons might depend on current behavior. So we wanted to be more conservative regarding "cleaning up" the requestingPrincipal.

If you think that can change requestingPrincipal to also match comment 12, without too much addon breakage, then I'm all for doing that.
(Assignee)

Comment 22

3 years ago
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #20)
> > We could consider passing null for TYPE_DOCUMENT, but that may break addons.
> 
> Won't they then break with e10s anyway?  In e10s, places that are
> TYPE_DOCUMENT will typically return null for
> mScriptGlobal->GetFrameElementInternal().  Except the ones that run in the
> chrome process, of course, which will _really_ confuse things...
> 
Yes, in e10s we are passing null as the requestingContext to ContentPolicy for TYPE_DOCUMENT loads.  Maybe we can have a followup to pass null in all TYPE_DOCUMENT cases when e10s is enabled?  Or I can do i now if we aren't worried about breakage.

> > In those cases, aOwner doesn’t exist anymore.
I'm looking into the cases in which aOwner doesn't exist anymore (refresh, enable/disable protection, and view source).  I'll respond on that later.


> As far as requestingPrincipal passed to content policies, the fundamental
> question to ask yourself is what should happen when script running in origin
> A sets window.location on a subframe whose parent document has origin B.
> Your proposal is to use B as the requestingPrincipal in this case.  I sort
> of feel like it should be A.  That is, it seems to me like
> requestingPrincipal should be aOwner if aOwner is not null, period....
...
> requestingPrincipal should be aOwner if aOwner is not null, period....
> I would like to understand under what conditions we would ever want this to
> differ from the "requestingPrincipal passed to Content Policies".  In other
> words, why wouldn't we use comment 12 for the requestingPrincipal too?

Ideally, we could pass both loadingPrincipal and triggeringPrincipal to content policies.  But the API doesn't allow for that right now.  So we have to try to give as much information as possible to the API, while trying to avoid breaking addons.

I would like requestingPrincipal = loadingPrincipal.  But loadingPrincipal = null for TYPE_DOCUMENT loads.  In those cases, we are giving consumers no information.  Consumers might want to now the trigger for top level loads.  So we can do:
requestingPrincipal = loadingPrincipal for TYPE_SUBDOCUMENT
requestingPrincipal = triggeringPrincipal for TYPE_DOCUMENT

This is what the proposal aims to do.  However, in the TYPE_SUBDOCUMENT case, the loadingPrincipal may already be available from the context.  So the question becomes:
Are there cases where (!requestingContext && loadingPrincipal !=null && loadingPrincipal!=triggeringPrincipal)
i.e. are there places where there is no context, a loadingPrincipal exists, and it differs from triggeringPrincipal?  

If not, then to provide the most information possible to content policy consumers (without regard to breakage), we should pass the triggeringPrincipal as requestingPrincipal.
(Assignee)

Comment 23

3 years ago
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #20)
> > A. When we Refresh a page
> 
> This should have a usable owner normally.  In particular, nsDocShell::Reload
> normally goes through LoadHistoryEntry, which will grab the owner from the
> session history entry, no?  If we're not always storing an owner in the
> session history entry then we should just fix that.
>

nsDocShell::Reload and nsDocShell:ReloadDocument() are not called when I click the refresh link in the url bar, or when I hit ctrl+R, or when I hit ctrl+shift+R.  Or when I restart the browser and restore previous session.  This is the sequence of calls when I refresh page:

nsSHistory::Reload
nsSHistory::LoadEntry
nsSHistory::InitiateLoad
nsDocShell::LoadURI
nsDocShell::LoadHistoryEntry
nsDocShell::InternalLoad

The nsISHEntry's are created in LoadEntry() from an nsSHistory index.  The prevEntry and nextEntry created do not have owners.
https://mxr.mozilla.org/mozilla-central/source/docshell/shistory/nsSHistory.cpp#1538

Owner can also be set in docShellLoadInfo.  The docShellLoadInfo is created in nsSHistory::InitiateLoad, without an owner.

Boris, where do I get the owner for Reloads?

(There is also a lot of logic around owner here.  This doesn't make a difference in this case, but I don't follow what all these different cases are for.
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1466)
(Assignee)

Comment 24

3 years ago
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #20)
> > Enable/Disable mixed content protection
> 
> Is this coming through Reload, or taking some other codepath?

This also goes through nsSHistory::Reload so has the same problem as refresh


> > view source on a page
> 
> This one might need some thought, but see below.
> 
This comes through nsDocShell::LoadPage() and then nsDocShell::LoadHistoryEntry.  LoadPage creates an nsSHEntry from nsISupports* aPageDescriptor, which has no owner.
(Assignee)

Comment 25

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #22)
> I would like requestingPrincipal = loadingPrincipal.  But loadingPrincipal =
> null for TYPE_DOCUMENT loads.  In those cases, we are giving consumers no
> information.  Consumers might want to now the trigger for top level loads. 
> So we can do:
> requestingPrincipal = loadingPrincipal for TYPE_SUBDOCUMENT
> requestingPrincipal = triggeringPrincipal for TYPE_DOCUMENT
> 
> This is what the proposal aims to do.  However, in the TYPE_SUBDOCUMENT
> case, the loadingPrincipal may already be available from the context.  So
> the question becomes:
> Are there cases where (!requestingContext && loadingPrincipal !=null &&
> loadingPrincipal!=triggeringPrincipal)
> i.e. are there places where there is no context, a loadingPrincipal exists,
> and it differs from triggeringPrincipal?  
> 
> If not, then to provide the most information possible to content policy
> consumers (without regard to breakage), we should pass the
> triggeringPrincipal as requestingPrincipal.

If:
1) we put breakage aside (both from our internal content policies and addon content policies)
2) our goal is to provide the most information possible to current and future consumers of the Content Policy API without changing the API, regardless of breakage
3) there are no cases where (!requestingContext && loadingPrincipal !=null && loadingPrincipal!=triggeringPrincipal)

Then:
We should pass triggeringPrincipal as requestingPrincipal
We should continue to pass requestingContext, and let consumers derive loadingPrincipal from the context.

To test 3, I added an assert in nsDocShell::DoURILoad() right before we call NS_NewChannelInternal and pushed to try to see what happens:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9b5b4e9e04

In the meantime, I am going to proceed with the implementation laid out in comment 18.  And we will tweak as necessary once Jonas and Boris are back from vacation.
(Assignee)

Comment 26

3 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #25)
> 3) there are no cases where (!requestingContext && loadingPrincipal !=null
> && loadingPrincipal!=triggeringPrincipal)
> 
> To test 3, I added an assert in nsDocShell::DoURILoad() right before we call
> NS_NewChannelInternal and pushed to try to see what happens:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9b5b4e9e04
> 
Inspecting try, we hit the assertion in browser/base/content/test/general/browser_page_style_menu.js.  We have a case where we don’t have a context, loadingPrincipal is null and does not equal triggeringPrincipal.  triggeringPrincipal is system.  This happens because mScriptGlobal is missing from DoURILoad().  mScriptGlobal no longer existed after a call to nsDocShell::Stop() here https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10268.  More detailed logs can be found here https://pastebin.mozilla.org/8855516

There is another failure that I didn’t look at closely, but it also has this issue because mScriptGlobal no longer exists by the time loadInfo is created:
devtools/client/tilt/test/browser_tilt_05_destruction-url.js
(Assignee)

Comment 27

3 years ago
Created attachment 8703231 [details] [diff] [review]
docshell-implementation-12-31-15.patch

Adding two patches here.  This one is a WIP implementation.  It updates requestingPrincipal, loadingPrincipal, and loadingNode as described in comment 18.  It doesn't touch requestingContext (since the proposal in that comment is to not change that for addon compat) for now.  It doesn't touch triggeringPrincipal as described in comment 12 (that still needs to be done).  I've also added a number of asserts which I will take out later, but are for scenarios that shouldn't occur.

Here is a recent try push with failures.  Most are because of one bug I haven't been able to figure out yet.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa7027614b13

This patch will only apply on top of a second patch that is I just use for debugging purposes.  I will add that patch as well for completeness, but otherwise it can be ignored.

[1] Something goes wrong here https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1981.  We end up in obj-ff-dbg/ipc/ipdl/PNeckoChild.cpp::Write() for PrincipalInfo and a PrincipalInfo with an unknown type.  Still debugging. https://pastebin.mozilla.org/8855638
Attachment #8533260 - Attachment is obsolete: true
(Assignee)

Comment 28

3 years ago
Created attachment 8703232 [details] [diff] [review]
docshell-principal-debug-12-31-15.patch

Debugging patch - full of a bunch of prints, principal and uri extractions, etc.
(Assignee)

Comment 29

3 years ago
Fixed some issues and here is a more recent push to try.  More green than before but still a lot of orange.  I'll post updated patches here soon.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d1e8c1d260&selectedJob=15170223

Spoke to Jonas yesterday about requestingContext yesterday.  For TYPE_DOCUMENT loads in e10s, requestingContext will end up being null anyway.  So preserving behavior for just a few releases isn't necessarily worthwhile, and adds more to the confusion around requestingContext and loadingNode.  So instead requestingContext and loadingNode will be identical:

requestingContext passed to Content Policies:
* null for top level loads (TYPE_DOCUMENT)
* mScriptGlobal->GetFrameElementInternal() for TYPE_SUBDOCUMENT.  Add an assertion that current and parent docshell types match.

requestingPrincipal passed to Content Policies:
* if aOwner exists, use it for top level loads (TYPE_DOCUMENT); this is the trigger.  If aOwner doesn't exist, fall back to aReferrer if it exists.  Otherwise pass nothing (null but not nullPrincipal)
* use GetFrameElementInternal for subdocuments (TYPE_SUBDOCUMENT). This should give you the parent documents principal.  Add an assertion that current and parent docshell types match.

loadInfo->loadingNode
* null for top level loads (TYPE_DOCUMENT)
* mScriptGlobal->GetFrameElementInternal() for TYPE_SUBDOCUMENT.  Add an assertion that current and parent docshell types match.
* No more use of GetExtantDoc()

loadInfo->loadingPrincipal
* loadInfo->loadingNode ? loadInfo->loadingNode->NodePrincipal : null (null not nullprincipal)
* This will result in null for TYPE_DOCUMENT loads and should always result in an actual principal for TYPE_SUBDOCUMENT.
* Don't fall back to triggeringPrincipal as we do today.    We should never have a subdocument with no parent.
Sorry for the lag here, I was on vacation....

(In reply to Jonas Sicking (:sicking) from comment #21)
> If you think that can change requestingPrincipal to also match
> comment 12, without too much addon breakage

My gut feeling is we can, but I can't guarantee that.  I guess I can live with leaving requestingPrincipal as-is for the moment and then trying to update it in a separate patch.

I do strongly feel that if aOwner exists we should use it here, period.  I'm OK with that being a followup, as long as it happens.

(In reply to Tanvi Vyas [:tanvi] from comment #22)
>Or I can do i now if we aren't worried about breakage.

I'm OK with doing this in a separate patch from the general loadingPrincipal/triggeringPrincipal changes, but I think we should do it, even in non-e10s mode....

>I would like requestingPrincipal = loadingPrincipal.

No, we want requestingPrincipal == triggeringPrincipal.  Imo.  Sounds like you agree with me, looking at the end of comment 22.

(In reply to Tanvi Vyas [:tanvi] from comment #23)
>The nsISHEntry's are created in LoadEntry() from an nsSHistory index.

Created or gotten?  I'd expect them to be _gotten_ from the session history.

>The prevEntry and nextEntry created do not have owners.

That seems.. broken.  nsDocShell::AddToSessionHistory definitely passes an owner to nsISHEntry::Create.  Again, maybe it's null.  If so then we should fix that, as I said in comment 20.  For example, looks like OnLoadingSite passes null to OnNewURI for the owner, whereas ideally it would pass the triggering principal of the channel.  There's certainly regression risk here (e.g. we should look at the code that _gets_ owners from history entries and make sure it's OK with owner meaning "triggering principal" as opposed to "force this principal".  But I think that's generally the right direction to go in: if we're going to redo a load, we need to store all the state from the original load so we can redo it properly.

> but I don't follow what all these different cases are for

Most of them are for stupid cases like API entrypoints that just take a string.  For Gecko-triggered loads, at that point "owner" should never be null.  If it's ever null there, those are bugs we should fix.

(In reply to Tanvi Vyas [:tanvi] from comment #24)
> LoadPage creates an nsSHEntry from nsISupports* aPageDescriptor, which has no owner.

Gets, not creates.  Again, this seems like the same issue where session history is not storing enough information to actually properly redo the load, and we should simply fix that brokenness.

(In reply to Tanvi Vyas [:tanvi] from comment #26)
> We have a case where we don’t have a context, loadingPrincipal is null and does not equal
> triggeringPrincipal. 

So did mScriptGlobal _become_ null or is it _still_ null?  If mScriptGlobal _became_ null, that means this docshell has been torn down, and we shouldn't be doing any loading in it at all.  If mScriptGlobal hasn't been set up yet, we should set it up (the fact that a docshell doesn't start off with an mScriptGlobal but creates it lazily is rather silly in the first place).

(In reply to Tanvi Vyas [:tanvi] from comment #29)
>requestingPrincipal passed to Content Policies:
>* use GetFrameElementInternal for subdocuments (TYPE_SUBDOCUMENT).

I still think this is wrong, per above.

And per above, I think that any cases in which aOwner is null are basically buggy and we should fix them as much as possible, so ideally we can stop playing games with aReferrer fallbacks and whatnot.
(Assignee)

Comment 31

3 years ago
Created attachment 8706050 [details] [diff] [review]
docshell-principal-clean-01-09-16.patch

Updated patches (which does not take Boris' latest comment 30 into account).
* requestingPrincipal is loader for TYPE_SUBDOCUMENT and trigger for TYPE_DOCUMENT.
* requestingContext and loadingNode null for TYPE_DOCUMENT (we no longer use GetExtantDoc for loadingNode) and outer window's frameelementinternal for subdocument
* loadingPrincipal is taken directly from loadingNode, if loadingNode doesn't exist, it's null.
* Also add a bunch of asserts in places; if triggered we may have some bugs.


Push to try has debugging code in it, but patches here do not.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c5bc798529
Attachment #8703231 - Attachment is obsolete: true
Attachment #8703232 - Attachment is obsolete: true
(Assignee)

Comment 32

3 years ago
Created attachment 8706051 [details] [diff] [review]
loadinfo-updates-01-09-16.patch

Updating loadInfo so loadingPrincipal and loadingNode can be null for TYPE_DOCUMENT loads.
(Assignee)

Comment 33

3 years ago
Created attachment 8706052 [details] [diff] [review]
loadinfo-updates-01-09-16B.patch
Attachment #8706051 - Attachment is obsolete: true
Attachment #8706052 - Flags: feedback?(mozilla)
Comment on attachment 8706052 [details] [diff] [review]
loadinfo-updates-01-09-16B.patch

Review of attachment 8706052 [details] [diff] [review]:
-----------------------------------------------------------------

The way I understand the current proposal is that the loadingPrincipal can be null for TYPE_DOCUMENT loads and in such a case the triggeringPrincipal needs to be non-null - which sounds reasonable. The logic implemented here for the loadInfo seems reasonable and correct, just adding some nits, but overall looks pretty good to me.

I suppose we also need to check those callsites [1] to make sure they can't create a TYPE_DOCUMENT loadInfo, but should be OK as far as I can tell.

One last thought: we need code within nsContentSecurityManager to allow the special handling of TYPE_DOCUMENT. Right now we rely on the fact that the loadingPrincipal is never null within the loadInfo. Actually, that might be true for other consumers as well - we need to investigate [2]

[1] http://mxr.mozilla.org/mozilla-central/search?string=new%2Bmozilla%3A%3ALoadInfo
[2] http://mxr.mozilla.org/mozilla-central/search?string=info-%3ELoadingPrincipal%28%29&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

::: dom/security/nsCSPService.cpp
@@ +356,5 @@
>    nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  if (!loadInfo->LoadingPrincipal() && nsIContentPolicy::TYPE_DOCUMENT) {
> +    // Top level loads are not subject to a csp
> +    return NS_OK;
> +  }

I think it's just easiest if we slightly refactor subjectToCSP and add the contentPolicyType as a second argument and then move that code:
http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#136
into subjectToCSP and we are good - Easy fix!

::: ipc/glue/BackgroundUtils.cpp
@@ +223,5 @@
> +    requestingPrincipalInfo = requestingPrincipalInfoTemp;
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  } else {
> +    requestingPrincipalInfo = mozilla::void_t();
> +  }

I think no need for the tmp, you could simplify to:

OptionalPrincipalInfo requestingPrincipalInfo = mozilla::void_t();
if (aLoadInfo->LoadingPrincipal()) {
  rv = PrincipalToPrincipalInfo(aLoadInfo->LoadingPrincipal(),
                                &requestingPrincipalInfo);
    NS_ENSURE_SUCCESS(rv, rv);
}

@@ +280,5 @@
> +  if (loadInfoArgs.requestingPrincipalInfo().type() == OptionalPrincipalInfo::Tvoid_t) {
> +    requestingPrincipal = nullptr;
> +  } else {
> +    requestingPrincipal = PrincipalInfoToPrincipal(loadInfoArgs.requestingPrincipalInfo(), &rv);
> +  }

nsCOMPtrs are initalized and set to nullptr - in other words you could simplify that code and just use:

if (loadInfoArgs.requestingPrincipalInfo().type() != OptionalPrincipalInfo::Tvoid_t) {
  requestingPrincipal = PrincipalInfoToPrincipal(loadInfoArgs.requestingPrincipalInfo(), &rv);
  NS_ENSURE_SUCCESS(rv, rv);
}

::: netwerk/base/LoadInfo.cpp
@@ +97,5 @@
> +    // what if that is system?  Does systemPrincipal have originattributes?
> +    // can we pass them in from the docshell in the call to NS_NewChannelInternal?
> +    const PrincipalOriginAttributes attrs = BasePrincipal::Cast(mTriggeringPrincipal)->OriginAttributesRef();
> +    mOriginAttributes.InheritFromDocToNecko(attrs);
> +  }

That is a good question - I don't think using the originAttributes for the triggeringPricnipal is what we want here.

::: netwerk/base/nsIOService.cpp
@@ +851,5 @@
>  
> +    // TYPE_DOCUMENT loads don't require a loadingNode or principal, but other
> +    // types do.
> +    if (aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT || 
> +        aLoadingNode || aLoadingPrincipal) {

nit: just to be consistent, flip the checks around here as well:
if (aLoadingNode || aLoadingPrincipal ||
    aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT) {...
Attachment #8706052 - Flags: feedback?(mozilla) → feedback+
(Assignee)

Comment 35

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #30)
> (In reply to Tanvi Vyas [:tanvi] from comment #26)
> > We have a case where we don’t have a context, loadingPrincipal is null and does not equal
> > triggeringPrincipal. 
> 
> So did mScriptGlobal _become_ null or is it _still_ null?  If mScriptGlobal
> _became_ null, that means this docshell has been torn down, and we shouldn't
> be doing any loading in it at all.  

mScriptGlobal becomes null - https://pastebin.mozilla.org/8856613


> If mScriptGlobal hasn't been set up yet,
> we should set it up (the fact that a docshell doesn't start off with an
> mScriptGlobal but creates it lazily is rather silly in the first place).
> 

I've added a call to EnsureScriptEnvironment here to ensure that mScriptGlobal exists the first time we use it in InternalLoad - https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9589
(Assignee)

Comment 36

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #30)
> (In reply to Tanvi Vyas [:tanvi] from comment #23)
> >The nsISHEntry's are created in LoadEntry() from an nsSHistory index.
> 
> Created or gotten?  I'd expect them to be _gotten_ from the session history.
>
You are right, they are gotten from the history entry index.  And the prev and next entries that are gotten when a reload takes place don't have owners attached to them.
 
> >The prevEntry and nextEntry created do not have owners.
> 
> That seems.. broken.  nsDocShell::AddToSessionHistory definitely passes an
> owner to nsISHEntry::Create.  Again, maybe it's null.  If so then we should
> fix that, as I said in comment 20.  
Yes, you are right.  It looks like I need to trace back further to AddToSessionHistory calls, which sometimes uses a nullptr ...

> For example, looks like OnLoadingSite
> passes null to OnNewURI for the owner, whereas ideally it would pass the
> triggering principal of the channel.  
or is called by functions that pass it a null owner.

I will dig into this further.
> mScriptGlobal becomes null - https://pastebin.mozilla.org/8856613

Huh.  We should figure out why (e.g. beforeunload event removing the iframe from the page?) and probably bail out at that point or something...
(Assignee)

Updated

3 years ago
See Also: → bug 1240246
(Assignee)

Updated

3 years ago
See Also: → bug 1240258
(Assignee)

Comment 38

3 years ago
Lots of issues and bugs are lumped into this bug, so I've attempted to separate them out.  In this bug, we will only do number 1 below.

1) Change loadingNode, loadingPrincipal, requestingContext
* We can start with just these changes in this bug, so that we can unblock some e10s bugs
* Since we are passing null loadingnodes and principals for type_document, we should add a couple assertions (in content policy API and loadinfo constructors)
* Since we want mOuterWindow in loadInfo, we need to create a new loadInfo constructor that takes an outerWindow.  Then we need to change the new channel calls to ones that take a loadInfo instead of the individual parameters.  The loadInfo constructor should assert that if type=DOCUMENT then loadingNode==null and mOuterWindow != null.

The patches here need to be updated to account for passing the mOuterWindow, since we had not considered that before.


2) mScriptGlobal becomes null.  I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1240246 for this.

3) After much debate, I’ve conceded that requestingPrincipal should be the loads “trigger” and not the context in which the load is taking place.  Content Policy consumers should be able to get the context in which the load is taking place by getting the principal associated with the requestingContext (as long as the requestingContext is a node).  Hence, passing the trigger provides more information to the policies.  In the case of TYPE_DOCUMENT loads, we don’t have a requestingContext, but the fact that the load is TYPE_DOCUMENT should tell the Content Policy all it needs to know - it is being loaded as a top level page.

Moreover, the behavior of requestingPrincipal (the principal passed to content policies) is not consistent today.  This is because there are many problems with the “owner” used to derive requestingPrincipal (see 4 below for more on this).  Hence, policies that depend on it are likely already broken.

We should document this clearly in the Content Policy API.

4) There are many cases where the owner passed to InternalLoad is wrong or missing.  I’ve filed bug https://bugzilla.mozilla.org/show_bug.cgi?id=1240258 for this.  We need to fix this in order to get the right requestingPrincipal and triggeringPrincipal.

5) In the general case, triggeringPrincipal will equal the aOwner passed into DoURILoad().  We will set it to something else for the cases described in comment 12.  We can use already filed bug https://bugzilla.mozilla.org/show_bug.cgi?id=1181370 to track the changes to triggeringPrincipal and requestingPrincipal.
(Assignee)

Updated

3 years ago
Summary: Setting correct loadingPrincipal and triggeringPrincipal in docshell → Set correct loadingPrincipal, loadingNode, and requestingContext in docshell
(Assignee)

Comment 39

3 years ago
If we make requestingPrincipal and loadingNode null in TYPE_DOCUMENT cases, we will lose the mOuterWindow in loadInfo.

So we need to create a new loadInfo constructor that takes <loadingNode, outerwindow, triggeringPrincipal, ContentPolicyType>.  It should assert that loadingNode is null only when we have TYPE_DOCUMENT.  Then pass the loadInfo object directly when creating a new channel.

Also, we need to change NewSrcdocChannel and InitSrcdoc so that they take a loadInfo, instead of each loadInfo parameter separately:
https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10633
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/viewsource/nsViewSourceChannel.cpp#96
(Assignee)

Comment 40

3 years ago
(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #39) 
> So we need to create a new loadInfo constructor that takes <loadingNode,
> outerwindow, triggeringPrincipal, ContentPolicyType>.  It should assert that
> loadingNode is null only when we have TYPE_DOCUMENT.
It shoudl also assert that the only content types that use this constructor are type_document and type_subdocument.

>  Then pass the loadInfo
> object directly when creating a new channel.

Updated

3 years ago
Blocks: 1242871
Tanvi, what's the minimum that we need to do here for e10s, and is there any way I can help out? From comment 38, it sounds like the issue with the outer window ID is all that remains to be able to check something in. I can certainly help out with that.

I looked over the patches and they seem reasonably close. Have you had a chance to push to try?
Flags: needinfo?(tanvi)
(Assignee)

Comment 43

3 years ago
(In reply to Bill McCloskey (:billm) from comment #41)
> Tanvi, what's the minimum that we need to do here for e10s, and is there any
> way I can help out? From comment 38, it sounds like the issue with the outer
> window ID is all that remains to be able to check something in. I can
> certainly help out with that.
> 
> I looked over the patches and they seem reasonably close. Have you had a
> chance to push to try?

I spoke to Bill and I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1248089.
Flags: needinfo?(tanvi)
(Assignee)

Comment 44

3 years ago
Created attachment 8719091 [details] [diff] [review]
loadinfo-updates-02-12-16.patch

Thanks Christoph for the review!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #34)
> Comment on attachment 8706052 [details] [diff] [review]
> loadinfo-updates-01-09-16B.patch
> 
> Review of attachment 8706052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/
> search?string=new%2Bmozilla%3A%3ALoadInfo
> [2]
> http://mxr.mozilla.org/mozilla-central/search?string=info-
> %3ELoadingPrincipal%28%29&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&
> tree=mozilla-central
> 
I still need to do this audit.


> ::: ipc/glue/BackgroundUtils.cpp
> @@ +223,5 @@
> > +    requestingPrincipalInfo = requestingPrincipalInfoTemp;
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +  } else {
> > +    requestingPrincipalInfo = mozilla::void_t();
> > +  }
> 
> I think no need for the tmp, you could simplify to:
> 
> OptionalPrincipalInfo requestingPrincipalInfo = mozilla::void_t();
> if (aLoadInfo->LoadingPrincipal()) {
>   rv = PrincipalToPrincipalInfo(aLoadInfo->LoadingPrincipal(),
>                                 &requestingPrincipalInfo);
>     NS_ENSURE_SUCCESS(rv, rv);
> }
> 
PrincipalToPrincipalInfo takes a PrincipalInfo, not an OptionalPrincipalInfo, which is why we need the temp.


> ::: netwerk/base/LoadInfo.cpp
> @@ +97,5 @@
> > +    // what if that is system?  Does systemPrincipal have originattributes?
> > +    // can we pass them in from the docshell in the call to NS_NewChannelInternal?
> > +    const PrincipalOriginAttributes attrs = BasePrincipal::Cast(mTriggeringPrincipal)->OriginAttributesRef();
> > +    mOriginAttributes.InheritFromDocToNecko(attrs);
> > +  }
> 
> That is a good question - I don't think using the originAttributes for the
> triggeringPricnipal is what we want here.
> 
OriginAttribute question still remains; I will talk to Jonas during next Wednesday's meeting.

Other comments are addressed.
Attachment #8706052 - Attachment is obsolete: true
Attachment #8719091 - Flags: review?(mozilla)
Comment on attachment 8719091 [details] [diff] [review]
loadinfo-updates-02-12-16.patch

Review of attachment 8719091 [details] [diff] [review]:
-----------------------------------------------------------------

Looks pretty good, I would like to look at it one more time before you get the r+, but I think we already have what we want.

::: dom/security/nsCSPService.cpp
@@ +46,5 @@
> +subjectToCSP(nsIURI* aURI, nsIPrincipal* loadingPrincipal, nsContentPolicyType aContentType) {
> +  // Top level loads are not subject to a csp
> +  if (!loadingPrincipal && aContentType == nsIContentPolicy::TYPE_DOCUMENT) {
> +    return false;
> +  }

Just move that complete block in here:

  // These content types are not subject to CSP content policy checks:
  // TYPE_CSP_REPORT -- csp can't block csp reports
  // TYPE_REFRESH    -- never passed to ShouldLoad (see nsIContentPolicy.idl)
  // TYPE_DOCUMENT   -- used for frame-ancestors
  if (aContentType == nsIContentPolicy::TYPE_CSP_REPORT ||
    aContentType == nsIContentPolicy::TYPE_REFRESH ||
    aContentType == nsIContentPolicy::TYPE_DOCUMENT) {
    return NS_OK;
  }

No need for the loadingPrincipal argument since TYPE_DOCUMENT (having or not having a loadingPricnipal) is not subject to CSP.

@@ +294,5 @@
>    nsresult rv = newChannel->GetURI(getter_AddRefs(newUri));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  rv = oldChannel->GetLoadInfo(getter_AddRefs(loadInfo));

Nit:
nsCOMPtr<nsILoadInfo> loadInfo = oldChannel->GetLoadInfo();

::: ipc/glue/BackgroundUtils.cpp
@@ +223,5 @@
> +    requestingPrincipalInfo = requestingPrincipalInfoTemp;
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  } else {
> +    requestingPrincipalInfo = mozilla::void_t();
> +  }

Ah, I see. Well we could update PrincipalToPrincipalInfo to take an OptionalPrincipalInfo, but then we also want to throw an error if we use it for the TriggeringPrincipal. So, some kind of trade-off is needed :-(

Potentially we can keep what you have for now, but you could slightly simplify to:
OptionalPrincipalInfo requestingPrincipalInfo = mozilla::void_t();

and remove the else-branch and move the NS_ENSURE_SUCCESS(rv, rv) right after the rv =

::: netwerk/base/LoadInfo.cpp
@@ +42,5 @@
>    , mEnforceSecurity(false)
>    , mInitialSecurityCheckDone(false)
>    , mIsThirdPartyContext(true)
>  {
> +  MOZ_ASSERT(mLoadingPrincipal || aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT);

Nit: maybe add a comment on top indicating that we either have a loadingPrincipal and that 'only' if the TYPE is document there is no need for a loadingPrincipal, but in that case we need a TriggeringPrincipal.

@@ +88,5 @@
>      mUpgradeInsecurePreloads = aLoadingContext->OwnerDoc()->GetUpgradeInsecurePreloads();
>    }
>  
> +  if (mLoadingPrincipal) {
> +    const PrincipalOriginAttributes attrs = BasePrincipal::Cast(mLoadingPrincipal)->OriginAttributesRef();

Nit: maybe separate the assignment to fit into 80 chars:
const PrincipalOriginAttributes attrs =
  BasePrincipal::Cast(mLoadingPrincipal)->OriginAttributesRef();

@@ +97,5 @@
> +    // what if that is system?  Does systemPrincipal have originattributes?
> +    // can we pass them in from the docshell in the call to NS_NewChannelInternal?
> +    const PrincipalOriginAttributes attrs = BasePrincipal::Cast(mTriggeringPrincipal)->OriginAttributesRef();
> +    mOriginAttributes.InheritFromDocToNecko(attrs);
> +  }

Wouldn't it make more sense as:

if (aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT) {
  const PrincipalOriginAttributes triggeringAttrs =
     BasePrincipal::Cast(mTriggeringPrincipal)->OriginAttributesRef();
  mOriginAttributes.InheritFromDocToNecko(triggeringAttrs);
}
else {
  const PrincipalOriginAttributes loadingAttrs =
     BasePrincipal::Cast(mLoadingPrincipal)->OriginAttributesRef();
  mOriginAttributes.InheritFromDocToNecko(loadingAttrs);
}


I can't answer the questions about the originAttributes of the systemPrincipal, probably good advised to consult sicking in our next meeting.
Attachment #8719091 - Flags: review?(mozilla)
(Assignee)

Comment 46

3 years ago
> I can't answer the questions about the originAttributes of the
> systemPrincipal, probably good advised to consult sicking in our next
> meeting.

Talked to Jonas.  To get the originAttributes, we need to use the passed in window to get the docshell and get the attributes off of that.

When updating the loadinfo constructor to add the window, create a new constructor instead of changing the existing one.  So duplicate this constructor https://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#27 and leave the others as they are.
(Assignee)

Comment 47

2 years ago
Created attachment 8721545 [details] [diff] [review]
loadinfo-updates-02-19-16.patch

WIP patch.  I still need to get OriginAttributes using the OuterWindow in the new LoadInfo constructor.
Attachment #8719091 - Attachment is obsolete: true
(Assignee)

Comment 48

2 years ago
Created attachment 8721546 [details] [diff] [review]
newchannel-updates-02-19-16.patch

Patch from Blake.

Adds NS_NewInputStreamChannelInternal needed for docshell changes.  Updates
NewSrcDocChannel to take loadinfo object instead of individual loadinfo members.
(Assignee)

Comment 49

2 years ago
Try push which includes a new nsDocShell patch.  Not putting the patch up here because its full of debugging code.  Lets see how try goes first before I remove the debugging bits and post here.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=af2e60bf6e78
(Assignee)

Comment 50

2 years ago
Created attachment 8722787 [details] [diff] [review]
loadinfo-updates-02-23-16.patch
Attachment #8721545 - Attachment is obsolete: true
Attachment #8722787 - Flags: review?(mozilla)
Attachment #8722787 - Flags: review?(jonas)
Comment on attachment 8722787 [details] [diff] [review]
loadinfo-updates-02-23-16.patch

Review of attachment 8722787 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why the nsCSPService.cpp changes are in this patch. They don't seem wrong, but how are they related to the other changes?

::: dom/security/nsCSPService.cpp
@@ +49,5 @@
> +  // TYPE_REFRESH    -- never passed to ShouldLoad (see nsIContentPolicy.idl)
> +  // TYPE_DOCUMENT   -- used for frame-ancestors
> +  if (aContentType == nsIContentPolicy::TYPE_CSP_REPORT ||
> +    aContentType == nsIContentPolicy::TYPE_REFRESH ||
> +    aContentType == nsIContentPolicy::TYPE_DOCUMENT) {

Indent the second and third line so that it lines up after the parenthesis of the first line. I.e. the 'aContent's should be under each other.

if (... ||
    ... ||
    ...) {

@@ -242,1 @@
>      return NS_OK;

Why remove this check?

::: ipc/glue/BackgroundUtils.cpp
@@ +216,5 @@
>  
>    nsresult rv = NS_OK;
> +  OptionalPrincipalInfo requestingPrincipalInfo = mozilla::void_t();
> +  if (aLoadInfo->LoadingPrincipal()) {
> +    PrincipalInfo requestingPrincipalInfoTemp;

Nit: Can we name this 'loadingPrincipal*' rather than 'requestingPrincipal*'?

::: netwerk/base/LoadInfo.cpp
@@ +132,5 @@
> +     called by both constructors, but let me know if you disagree, and i can
> +     create one */
> +  // if the load is sandboxed, we can not also inherit the principal
> +  if (mSecurityFlags & nsILoadInfo::SEC_SANDBOXED) {
> +    mSecurityFlags ^= nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;

Can we assert that both of these aren't set in http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#16

Even better might be to move that function to nsLoadInfo.cpp and call from both ctors.

@@ +140,5 @@
> +  mInnerWindowID = inner ? inner->WindowID() : 0;
> +  mOuterWindowID = aOuterWindow->WindowID();
> +
> +  nsCOMPtr<nsPIDOMWindowOuter> parent = aOuterWindow->GetScriptableParent();
> +  mParentOuterWindowID = parent->WindowID();

Won't this always be null in e10s builds? Seems like you need at least a nullcheck here (unless I'm wrong). But we might also want to make sure that e10s and non-e10s builds behave the same and always make mParentOuterIndowId null.

@@ +150,5 @@
> +  mUpgradeInsecureRequests = false;
> +
> +  // get the docshell from the outerwindow, and then get the originattributes
> +  nsCOMPtr<nsIDocShell> docShell = aOuterWindow->GetDocShell();
> +  if (docShell) {

This should never be null. So change this to MOZ_ASSERT(docShell);

::: netwerk/base/LoadInfo.h
@@ +53,5 @@
>             nsINode* aLoadingContext,
>             nsSecurityFlags aSecurityFlags,
>             nsContentPolicyType aContentPolicyType);
>  
> +  // constructor used for TYPE_DOCUMENT loads where we don'thave a loadingNode

space after "don't"
Attachment #8722787 - Flags: review?(jonas) → review+
Comment on attachment 8722787 [details] [diff] [review]
loadinfo-updates-02-23-16.patch

Review of attachment 8722787 [details] [diff] [review]:
-----------------------------------------------------------------

I don't understand why the nsCSPService.cpp changes are in this patch. They don't seem wrong, but how are they related to the other changes?

::: dom/security/nsCSPService.cpp
@@ +49,5 @@
> +  // TYPE_REFRESH    -- never passed to ShouldLoad (see nsIContentPolicy.idl)
> +  // TYPE_DOCUMENT   -- used for frame-ancestors
> +  if (aContentType == nsIContentPolicy::TYPE_CSP_REPORT ||
> +    aContentType == nsIContentPolicy::TYPE_REFRESH ||
> +    aContentType == nsIContentPolicy::TYPE_DOCUMENT) {

Indent the second and third line so that it lines up after the parenthesis of the first line. I.e. the 'aContent's should be under each other.

if (... ||
    ... ||
    ...) {

@@ -242,1 @@
>      return NS_OK;

Why remove this check?

::: ipc/glue/BackgroundUtils.cpp
@@ +216,5 @@
>  
>    nsresult rv = NS_OK;
> +  OptionalPrincipalInfo requestingPrincipalInfo = mozilla::void_t();
> +  if (aLoadInfo->LoadingPrincipal()) {
> +    PrincipalInfo requestingPrincipalInfoTemp;

Nit: Can we name this 'loadingPrincipal*' rather than 'requestingPrincipal*'?

::: netwerk/base/LoadInfo.cpp
@@ +132,5 @@
> +     called by both constructors, but let me know if you disagree, and i can
> +     create one */
> +  // if the load is sandboxed, we can not also inherit the principal
> +  if (mSecurityFlags & nsILoadInfo::SEC_SANDBOXED) {
> +    mSecurityFlags ^= nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;

Can we assert that both of these aren't set in http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#16

Even better might be to move that function to nsLoadInfo.cpp and call from both ctors.

@@ +140,5 @@
> +  mInnerWindowID = inner ? inner->WindowID() : 0;
> +  mOuterWindowID = aOuterWindow->WindowID();
> +
> +  nsCOMPtr<nsPIDOMWindowOuter> parent = aOuterWindow->GetScriptableParent();
> +  mParentOuterWindowID = parent->WindowID();

Won't this always be null in e10s builds? Seems like you need at least a nullcheck here (unless I'm wrong). But we might also want to make sure that e10s and non-e10s builds behave the same and always make mParentOuterIndowId null.

@@ +150,5 @@
> +  mUpgradeInsecureRequests = false;
> +
> +  // get the docshell from the outerwindow, and then get the originattributes
> +  nsCOMPtr<nsIDocShell> docShell = aOuterWindow->GetDocShell();
> +  if (docShell) {

This should never be null. So change this to MOZ_ASSERT(docShell);

::: netwerk/base/LoadInfo.h
@@ +53,5 @@
>             nsINode* aLoadingContext,
>             nsSecurityFlags aSecurityFlags,
>             nsContentPolicyType aContentPolicyType);
>  
> +  // constructor used for TYPE_DOCUMENT loads where we don'thave a loadingNode

space after "don't"
Comment on attachment 8722787 [details] [diff] [review]
loadinfo-updates-02-23-16.patch

Review of attachment 8722787 [details] [diff] [review]:
-----------------------------------------------------------------

Jonas is right, we should not have duplicate code in both constructors; please add a function that is bound to the local scope of LoadInfo.cpp and is called from both constructors.
For the nsCSPService.cpp changes we could file a separate bug and land them right away.
Attachment #8722787 - Flags: review?(mozilla) → review+
Once this lands, looks like it need uplift to 46 along with 1113196.
Tracking so we make sure to get all the pieces in place for e10s staged rollout on beta.
status-firefox46: --- → affected
tracking-firefox46: --- → +
(Assignee)

Comment 55

2 years ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #54)
> Once this lands, looks like it need uplift to 46 along with 1113196.
> Tracking so we make sure to get all the pieces in place for e10s staged
> rollout on beta.

Hi Liz,
This won't land for FF 47 and will not get uplifted to 46.
OK, thanks Tanvi. 
Blake, should this be marked for e10s triage?
Flags: needinfo?(mrbkap)
This doesn't need to be marked for e10s triage. Bug 1113196 covered the upliftable parts of this work that were needed for e10s.
Flags: needinfo?(mrbkap)
Untracking, thanks for the clarifications.
tracking-firefox46: + → -
Whiteboard: [domsecurity-active]
(Assignee)

Updated

2 years ago
Depends on: 1259678
(Assignee)

Comment 59

2 years ago
Created attachment 8734635 [details] [diff] [review]
loadinfo-updates-03-24-16B.patch

Some parts of the now obsoleted patch were landed with bug 1113196.  Rebased.

Pushed to try along with the CSP changes in bug 1259678 and docshell changes I'm about to post.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c59261e8761c


(In reply to Jonas Sicking (:sicking) from comment #52)
> Comment on attachment 8722787 [details] [diff] [review]
> loadinfo-updates-02-23-16.patch
> ::: ipc/glue/BackgroundUtils.cpp
> @@ +216,5 @@
> >  
> >    nsresult rv = NS_OK;
> > +  OptionalPrincipalInfo requestingPrincipalInfo = mozilla::void_t();
> > +  if (aLoadInfo->LoadingPrincipal()) {
> > +    PrincipalInfo requestingPrincipalInfoTemp;
> 
> Nit: Can we name this 'loadingPrincipal*' rather than 'requestingPrincipal*'?
>
I did that where I could in this file, but it exists in a handful of other files: http://mxr.mozilla.org/mozilla-central/search?string=requestingPrincipalInfo.  Insteading of adding those changes here, we should do it in a separate bug.


> ::: netwerk/base/LoadInfo.cpp
> @@ +132,5 @@
> > +     called by both constructors, but let me know if you disagree, and i can
> > +     create one */
> > +  // if the load is sandboxed, we can not also inherit the principal
> > +  if (mSecurityFlags & nsILoadInfo::SEC_SANDBOXED) {
> > +    mSecurityFlags ^= nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
> 
> Can we assert that both of these aren't set in
> http://mxr.mozilla.org/mozilla-central/source/dom/security/
> nsContentSecurityManager.cpp#16
> 
> Even better might be to move that function to nsLoadInfo.cpp and call from
> both ctors.
> 
Let's also do in a followup bug.
Attachment #8721546 - Attachment is obsolete: true
Attachment #8722787 - Attachment is obsolete: true
Attachment #8734635 - Flags: review?(mozilla)
Attachment #8734635 - Flags: review?(jonas)
(Assignee)

Comment 60

2 years ago
Created attachment 8734636 [details] [diff] [review]
docshell-principal-changes-03-24-16.patch

* requestingContext passed to nsIContentPolicy and loadingNode passed to loadInfo null for TYPE_DOCUMENT and outer window's frameelementinternal for subdocuments
* loadingPrincipal is taken directly from loadingNode, if loadingNode doesn't exist, it's null.
* requestingPrincipal passed to nsIContentPolicy unchanged
* triggeringPrincipal passed to loadInfo unchanged
* Plus lots of debug code, which I will take out after we have a green try
Attachment #8706050 - Attachment is obsolete: true
Comment on attachment 8734635 [details] [diff] [review]
loadinfo-updates-03-24-16B.patch

Review of attachment 8734635 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Two minor nits below. Would be great if you could do test-fixes as separate patches.

::: netwerk/base/LoadInfo.cpp
@@ +173,5 @@
>  
> +  // get the docshell from the outerwindow, and then get the originattributes
> +  nsCOMPtr<nsIDocShell> docShell = aOuterWindow->GetDocShell();
> +  MOZ_ASSERT(docShell);
> +  const DocShellOriginAttributes attrs = 

Fix space after '='

::: netwerk/ipc/NeckoChannelParams.ipdlh
@@ +26,5 @@
>  //-----------------------------------------------------------------------------
>  
>  struct LoadInfoArgs
>  {
> +  OptionalPrincipalInfo requestingPrincipalInfo;

Name this 'loadingPrincipalInfo'
Comment on attachment 8734635 [details] [diff] [review]
loadinfo-updates-03-24-16B.patch

Review of attachment 8734635 [details] [diff] [review]:
-----------------------------------------------------------------

Changes look fine. A general question though: I suppose we need both constructors of LoadInfo() to be applicable to TYPE_DOCUMENT loads, right? Or should TYPE_DOCUMENT loads always use the constructor providing the |nsPIDOMWindowOuter| argument? If only one should be called, then we could add an assertion in the other constructor that contentpolicytype != TYPE_DOCUMENT. Not sure if that is applicable though.
Attachment #8734635 - Flags: review?(mozilla) → review+
(Assignee)

Comment 63

2 years ago
Fixed a few tests locally and reposting to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b82b1057ca0

Now is a good time for me to start the audit mentioned in comment 34 to find and update places that depend on a loadingPrincipal:
http://mxr.mozilla.org/mozilla-central/search?string=info-%3ELoadingPrincipal%28%29&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
(Assignee)

Updated

2 years ago
Depends on: 1260876
(Assignee)

Comment 64

2 years ago
Created attachment 8737409 [details] [diff] [review]
loadinfo-updates-04-01-16.patch

Carrying over r+ from Jonas and Christoph.

(In reply to Christoph Kerschbaumer [:ckerschb, back on April 11th] from comment #62)
> Comment on attachment 8734635 [details] [diff] [review]
> loadinfo-updates-03-24-16B.patch
> 
> Review of attachment 8734635 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes look fine. A general question though: I suppose we need both
> constructors of LoadInfo() to be applicable to TYPE_DOCUMENT loads, right?
> Or should TYPE_DOCUMENT loads always use the constructor providing the
> |nsPIDOMWindowOuter| argument? If only one should be called, then we could
> add an assertion in the other constructor that contentpolicytype !=
> TYPE_DOCUMENT. Not sure if that is applicable though.

TYPE_DOCUMENT loads should only use the constructor providing the |nsPIDOMWindowOuter| argument.  So I've added an assertion to the other constructor that contentPolicyType != TYPE_DOCUMENT.

(In reply to Jonas Sicking (:sicking) from comment #61)
> ::: netwerk/base/LoadInfo.cpp
> @@ +173,5 @@
> >  
> > +  // get the docshell from the outerwindow, and then get the originattributes
> > +  nsCOMPtr<nsIDocShell> docShell = aOuterWindow->GetDocShell();
> > +  MOZ_ASSERT(docShell);
> > +  const DocShellOriginAttributes attrs = 
> 
> Fix space after '='
> 
Fixed

> ::: netwerk/ipc/NeckoChannelParams.ipdlh
> @@ +26,5 @@
> >  //-----------------------------------------------------------------------------
> >  
> >  struct LoadInfoArgs
> >  {
> > +  OptionalPrincipalInfo requestingPrincipalInfo;
> 
> Name this 'loadingPrincipalInfo'
As stated in Comment 59, the name exists in another of other files: http://mxr.mozilla.org/mozilla-central/search?string=requestingPrincipalInfo.  I changed it where I could, and I think we should leave the rest to a followup.
Attachment #8734635 - Attachment is obsolete: true
Attachment #8737409 - Flags: review+
(Assignee)

Comment 65

2 years ago
Created attachment 8737410 [details] [diff] [review]
docshell-principal-changes-04-01-16.patch

A couple of to do's around asserts left.

I'll ask Jonas to review first before passing to bz.
Attachment #8734636 - Attachment is obsolete: true
Attachment #8737410 - Flags: review?(jonas)
(Assignee)

Comment 66

2 years ago
Created attachment 8737411 [details] [diff] [review]
create-nullprincipal-without-loadingPrincipal-04-01-16.patch
Attachment #8737411 - Flags: review?(jonas)
(Assignee)

Comment 67

2 years ago
Created attachment 8737413 [details] [diff] [review]
simplecontentpolicy-test-fix.patch
Attachment #8737413 - Flags: review?(wmccloskey)
(Assignee)

Comment 68

2 years ago
Created attachment 8737415 [details] [diff] [review]
loadingPrincipal-ChannelClassifier.cpp
Attachment #8737415 - Flags: review?(francois)
(Assignee)

Comment 69

2 years ago
Created attachment 8737416 [details] [diff] [review]
loadingPrincipal-ContentSecurityManager-04-01-16.patch

Not sure if this is the right way of handling TYPE_DOCUMENT cases in ContentSecurityManager.  Maybe we should use triggeringPrincipal in some of these TYPE_DOCUMENT cases?  We can't quite rely on triggeringPrincipal for TYPE_DOCUMENT yet though, so I'm not sure that would help.  Please take a look and see if these checks are okay to skip for TYPE_DOCUMENT.
Attachment #8737416 - Flags: review?(mozilla)
Attachment #8737416 - Flags: review?(jonas)
(Assignee)

Comment 70

2 years ago
Created attachment 8737419 [details] [diff] [review]
loadingPrincipal-PerformanceChecks-04-01-16.patch

I'm not sure if these performance checks apply to top level loads.  Boris and Valentin, please advise.  Thank you!
Attachment #8737419 - Flags: feedback?(valentin.gosu)
Attachment #8737419 - Flags: feedback?(bzbarsky)
(Assignee)

Comment 71

2 years ago
Created attachment 8737421 [details] [diff] [review]
test-fixes-04-01-16.patch

Here are some more test fixes, along with some open questions.  Please advise on the questions called out in the comments.
Attachment #8737421 - Flags: review?(mozilla)
Attachment #8737421 - Flags: review?(jonas)
Attachment #8737415 - Flags: review?(francois) → review+
(Assignee)

Comment 72

2 years ago
Comment on attachment 8737410 [details] [diff] [review]
docshell-principal-changes-04-01-16.patch

I'll fix up the to do items related to asserts and repost.
Attachment #8737410 - Flags: review?(jonas)
(Assignee)

Comment 73

2 years ago
Created attachment 8737424 [details] [diff] [review]
create-nullprincipal-without-loadingPrincipal-04-01-16B.patch
Attachment #8737411 - Attachment is obsolete: true
Attachment #8737411 - Flags: review?(jonas)
Attachment #8737424 - Flags: review?(jonas)
(Assignee)

Comment 74

2 years ago
Created attachment 8737432 [details] [diff] [review]
test-fixes-04-01-16B.patch
Attachment #8737421 - Attachment is obsolete: true
Attachment #8737421 - Flags: review?(mozilla)
Attachment #8737421 - Flags: review?(jonas)
Attachment #8737432 - Flags: review?(mozilla)
Attachment #8737432 - Flags: review?(jonas)
(Assignee)

Comment 75

2 years ago
Created attachment 8737436 [details] [diff] [review]
loadinfo-updates-04-01-16B.patch
Attachment #8737409 - Attachment is obsolete: true
Attachment #8737436 - Flags: review+
Comment on attachment 8737416 [details] [diff] [review]
loadingPrincipal-ContentSecurityManager-04-01-16.patch

Review of attachment 8737416 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: dom/security/nsContentSecurityManager.cpp
@@ +466,5 @@
>    if (cookiePolicy == nsILoadInfo::SEC_COOKIES_SAME_ORIGIN) {
>      nsIPrincipal* loadingPrincipal = loadInfo->LoadingPrincipal();
>  
> +    // TYPE_DOCUMENT loads don't have a loadingPrincipal
> +    if (loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT) {

Don't make this change. We should never load documents with nsILoadInfo::SEC_COOKIES_SAME_ORIGIN since we wouldn't have anything to do same-origin checks with.

Instead just add
MOZ_ASSERT(loadInfo->GetExternalContentPolicyType() !=
           nsIContentPolicy::TYPE_DOCUMENT);
Attachment #8737416 - Flags: review?(jonas) → review+
(Assignee)

Comment 77

2 years ago
Created attachment 8737443 [details] [diff] [review]
loadingPrincipal-ContentSecurityManager-04-01-16B.patch

(In reply to Jonas Sicking (:sicking) from comment #76)
> Comment on attachment 8737416 [details] [diff] [review]
> loadingPrincipal-ContentSecurityManager-04-01-16.patch
> 
> Review of attachment 8737416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that fixed.
> 
> ::: dom/security/nsContentSecurityManager.cpp
> @@ +466,5 @@
> >    if (cookiePolicy == nsILoadInfo::SEC_COOKIES_SAME_ORIGIN) {
> >      nsIPrincipal* loadingPrincipal = loadInfo->LoadingPrincipal();
> >  
> > +    // TYPE_DOCUMENT loads don't have a loadingPrincipal
> > +    if (loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT) {
> 
> Don't make this change. We should never load documents with
> nsILoadInfo::SEC_COOKIES_SAME_ORIGIN since we wouldn't have anything to do
> same-origin checks with.
> 
> Instead just add
> MOZ_ASSERT(loadInfo->GetExternalContentPolicyType() !=
>            nsIContentPolicy::TYPE_DOCUMENT);

Fixed.  carrying over Jonas's r+ and flagging Christoph.
Attachment #8737416 - Attachment is obsolete: true
Attachment #8737416 - Flags: review?(mozilla)
Attachment #8737443 - Flags: review?(mozilla)
(Assignee)

Updated

2 years ago
Attachment #8737443 - Flags: review+
Comment on attachment 8737413 [details] [diff] [review]
simplecontentpolicy-test-fix.patch

Review of attachment 8737413 [details] [diff] [review]:
-----------------------------------------------------------------

This seems bad. There needs to be some way to find out what <browser> element a top-level load is associated with.
Attachment #8737413 - Flags: review?(wmccloskey) → review-
Comment on attachment 8737432 [details] [diff] [review]
test-fixes-04-01-16B.patch

Review of attachment 8737432 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good modulo comments below.

::: docshell/base/nsDocShell.cpp
@@ +1586,5 @@
>    uint32_t loadType = LOAD_NORMAL;
> +  // sicking / ckerschb / bz:
> +  // Is this the loadingPrincipal or the triggeringPrincipal?  Since it is
> +  // coming from owner, it is likely the trigger.  But we are passing it
> +  // as the loader to NS_NewInputStreamChannel

I don't know the answer to this one.

::: dom/workers/ServiceWorkerEvents.cpp
@@ +255,5 @@
>  
>      int16_t decision = nsIContentPolicy::ACCEPT;
> +    // ckerschb -
> +    // Since requestingPrincipal is supposed to equal triggeringPrincipal,
> +    // should we be passing TriggeringPrincipal() here instead?

Is it really true that we generally should use the triggeringPrincipal?

nsContentSecurityManager.cpp uses LoadingPrincipal(). And from a security perspective it feels like that generally makes more sense since it's the LoadingPrincipal which gets access to the response.

I think document loading is an exception. There it makes more sense to pass TriggeringPrincipal() since the LoadingPrincipal() doesn't actually get access to the resulting resource.

::: netwerk/base/nsNetUtil.cpp
@@ +1326,5 @@
> +    // Question for sicking - do we need to call CheckMayLoad on the
> +    // triggeringPrincipal in this case?  Do calls to NS_HasBeenCrossOrigin
> +    // only apply to subresources?  I would think that they apply to top level
> +    // channels as well.
> +    if (loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT) {

We should just assert that this function is not called for TYPE_DOCUMENT loads. It doesn't really make sense to ask if those have been "cross-origin" since there's no origin to compare to.

So just add
MOZ_ASSERT(loadInfo->GetExternalContentPolicyType() !=
           nsIContentPolicy::TYPE_DOCUMENT);

and revert this change.

@@ +2275,3 @@
>        bool crossOriginNavigation =
>          (aLoadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT) &&
> +        (!aChannelResultPrincipal->Equals(aLoadInfo->TriggeringPrincipal()));

I don't know the specs here well enough to comment.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1348,5 @@
> +  // sicking to review
> +  MOZ_ASSERT(principal &&
> +             originalLoadInfo->GetExternalContentPolicyType() !=
> +               nsIContentPolicy::TYPE_DOCUMENT,
> +             "preflights do not apply to toplevel loads so principal should exist");

I'd say "Should not do CORS loads for toplevel loads, so a loadingPrincipal should always exist."
Attachment #8737432 - Flags: review?(jonas) → review+
Comment on attachment 8737419 [details] [diff] [review]
loadingPrincipal-PerformanceChecks-04-01-16.patch

Review of attachment 8737419 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsPerformance.cpp
@@ +152,5 @@
> +    // browser/base/content/test/general/browser_identity_UI.js
> +    // devtools/client/inspector/markup/test/browser_markup_links_06.js
> +    // bz or valentin to advise
> +    //MOZ_ASSERT(false, "TYPE_DOCUMENT loads should not go through CheckAllowedOrigin()");
> +    return false;

I don't think we need to assert.
It's not that the method isn't going to be called for TYPE_DOCUMENT, but that an entry of that type isn't going to be added to the performance object, so we can simply return false, and not worry about it.

@@ +154,5 @@
> +    // bz or valentin to advise
> +    //MOZ_ASSERT(false, "TYPE_DOCUMENT loads should not go through CheckAllowedOrigin()");
> +    return false;
> +  }
> +  

Trailing whitespace.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2915,5 @@
>      nsCOMPtr<nsILoadInfo> loadInfo;
>      GetLoadInfo(getter_AddRefs(loadInfo));
> +    // TYPE_DOCUMENT loads don't have a loadingPrincipal, so we can't set
> +    // AllRedirectsPassTimingAllowCheck on them.  valentin and bz to review
> +    if (loadInfo && loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT) {

Looks good to me.
Attachment #8737419 - Flags: feedback?(valentin.gosu) → feedback+
Comment on attachment 8737419 [details] [diff] [review]
loadingPrincipal-PerformanceChecks-04-01-16.patch

1) nsPerformanceTiming::CheckAllowedOrigin is only called from nsPerformanceTiming::nsPerformanceTiming and then only if aHttpChannel is non-null.  The comments in nsPerformanceTiming::nsPerformanceTiming say that aHttpChannel is null for document loads, though I assume it means TYPE_DOCUMENT, not all document loads....

So we should just be able to assert in nsPerformanceTiming::CheckAllowedOrigin that the load coming through is never TYPE_DOCUMENT, as far as I can tell.

2) The SetAllRedirectsPassTimingAllowCheck call in SetupReplacementChannel only matters for channels for which we will call GetAllRedirectsPassTimingAllowCheck() which once again is only the cases when aHttpChannel is non-null in nsPerformanceTiming::nsPerformanceTiming.  So skipping the SetAllRedirectsPassTimingAllowCheck for TYPE_DOCUMENT is perfectly reasonable.
Attachment #8737419 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 82

2 years ago
Created attachment 8738724 [details] [diff] [review]
test-fixes-04-06-16.patch

(In reply to Jonas Sicking (:sicking) from comment #79)
> Comment on attachment 8737432 [details] [diff] [review]
> test-fixes-04-01-16B.patch
> ::: dom/workers/ServiceWorkerEvents.cpp
> @@ +255,5 @@
> >  
> >      int16_t decision = nsIContentPolicy::ACCEPT;
> > +    // ckerschb -
> > +    // Since requestingPrincipal is supposed to equal triggeringPrincipal,
> > +    // should we be passing TriggeringPrincipal() here instead?
> 
> Is it really true that we generally should use the triggeringPrincipal?
> 
> nsContentSecurityManager.cpp uses LoadingPrincipal(). And from a security
> perspective it feels like that generally makes more sense since it's the
> LoadingPrincipal which gets access to the response.
> 
> I think document loading is an exception. There it makes more sense to pass
> TriggeringPrincipal() since the LoadingPrincipal() doesn't actually get
> access to the resulting resource.
> 
Hmm.  I'm not sure about this.  But we can work this out in a followup.  From comment 38:
3) After much debate, I’ve conceded that requestingPrincipal should be the loads “trigger” and not the context in which the load is taking place.  Content Policy consumers should be able to get the context in which the load is taking place by getting the principal associated with the requestingContext (as long as the requestingContext is a node).  Hence, passing the trigger provides more information to the policies.  In the case of TYPE_DOCUMENT loads, we don’t have a requestingContext, but the fact that the load is TYPE_DOCUMENT should tell the Content Policy all it needs to know - it is being loaded as a top level page.

I addressed the rest of your comments and have an updated patch.  Carrying over r+ from Jonas and r? to ckerschb.
Attachment #8737432 - Attachment is obsolete: true
Attachment #8737432 - Flags: review?(mozilla)
Attachment #8738724 - Flags: review?(mozilla)
(Assignee)

Updated

2 years ago
Attachment #8738724 - Flags: review+
(Assignee)

Comment 83

2 years ago
Created attachment 8738727 [details] [diff] [review]
loadingPrincipal-PerformanceChecks-04-06-16.patch

Per below, it seems that Valentin and Boris disagree on whether we need the assert in nsPerformanceTiming::CheckAllowedOrigin.  Since the assert fails a couple mochtiests and since Valentin says that the method could be called on TYPE_DOCUMENT loads, I'm removing it.  And flagging for review this time instead of feedback.  Thanks!

(In reply to Valentin Gosu [:valentin] from comment #80)
> Comment on attachment 8737419 [details] [diff] [review]
> loadingPrincipal-PerformanceChecks-04-01-16.patch
> 
> Review of attachment 8737419 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsPerformance.cpp
> @@ +152,5 @@
> > +    // browser/base/content/test/general/browser_identity_UI.js
> > +    // devtools/client/inspector/markup/test/browser_markup_links_06.js
> > +    // bz or valentin to advise
> > +    //MOZ_ASSERT(false, "TYPE_DOCUMENT loads should not go through CheckAllowedOrigin()");
> > +    return false;
> 
> I don't think we need to assert.
> It's not that the method isn't going to be called for TYPE_DOCUMENT, but
> that an entry of that type isn't going to be added to the performance
> object, so we can simply return false, and not worry about it.
> 


(In reply to Boris Zbarsky [:bz] from comment #81)
> Comment on attachment 8737419 [details] [diff] [review]
> loadingPrincipal-PerformanceChecks-04-01-16.patch
> 
> 1) nsPerformanceTiming::CheckAllowedOrigin is only called from
> nsPerformanceTiming::nsPerformanceTiming and then only if aHttpChannel is
> non-null.  The comments in nsPerformanceTiming::nsPerformanceTiming say that
> aHttpChannel is null for document loads, though I assume it means
> TYPE_DOCUMENT, not all document loads....
> 
> So we should just be able to assert in
> nsPerformanceTiming::CheckAllowedOrigin that the load coming through is
> never TYPE_DOCUMENT, as far as I can tell.
>
Attachment #8737419 - Attachment is obsolete: true
Attachment #8738727 - Flags: review?(valentin.gosu)
Attachment #8738727 - Flags: review?(bzbarsky)
(Assignee)

Comment 84

2 years ago
Created attachment 8738729 [details] [diff] [review]
docshell-principal-changes-04-06-16.patch

Fixed todo items
(Assignee)

Updated

2 years ago
Attachment #8737410 - Attachment is obsolete: true
(Assignee)

Comment 85

2 years ago
Created attachment 8738734 [details] [diff] [review]
docshell-principal-changes-04-06-16B.patch

Here are the changes to nsDocShell.  Generally ready for review, but there are two open questions in there for bz.
Attachment #8738729 - Attachment is obsolete: true
Attachment #8738734 - Flags: review?(jonas)
Attachment #8738734 - Flags: review?(bzbarsky)
> Since the assert fails a couple mochtiests and since Valentin says that the method could be
> called on TYPE_DOCUMENT loads, I'm removing it.

If that method can be called for TYPE_DOCUMENT loads, then clearly some of the comments in that class are lying.  What tests is that assert failing on?  What does the callstack look like?
Flags: needinfo?(tanvi)
(Assignee)

Comment 87

2 years ago
Created attachment 8738744 [details] [diff] [review]
docshell-principal-changes-04-06-16C.patch

Fixing a typo.
Attachment #8738734 - Attachment is obsolete: true
Attachment #8738734 - Flags: review?(jonas)
Attachment #8738734 - Flags: review?(bzbarsky)
Flags: needinfo?(tanvi)
Attachment #8738744 - Flags: review?(jonas)
Attachment #8738744 - Flags: review?(bzbarsky)
Flags: needinfo?(tanvi)
(Assignee)

Comment 88

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #86)
> > Since the assert fails a couple mochtiests and since Valentin says that the method could be
> > called on TYPE_DOCUMENT loads, I'm removing it.
> 
> If that method can be called for TYPE_DOCUMENT loads, then clearly some of
> the comments in that class are lying.  What tests is that assert failing on?
> What does the callstack look like?

https://pastebin.mozilla.org/8866668
https://pastebin.mozilla.org/8866669
Flags: needinfo?(tanvi)
(Assignee)

Comment 89

2 years ago
try looks pretty good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7723d931dff7

Pending items:

* xpcshell test failures - this is because we create a TYPE_DOCUMENT load in javascript and use the wrong loadInfo constructor.  I've discussed a work around with Jonas and will post a new patch with that.

* netwerk/test/mochitests/test_origin_attributes_conversion.html and netwerk/test/mochitests/test_signed_web_packaged_app_origin.html failing - one of these tests will be removed and the other will be fixed by bug 1260876

* unexpected pass for a web platform test - when I run this test locally without my patches, it passes.  So I'm not sure if I need to do anything about this.

* work out with Boris and Valentin what to do about TYPE_DOCUMENT loads going through nsPerformanceTiming::CheckAllowedOrigin

* work with Bill and Blake to determine if its okay that requestingContext is set to null in the docshell patch and if we can update the simplecontentpolicy test (currently r-'ed)

* remaining reviews
> https://pastebin.mozilla.org/8866668

OK, so that shows that the comments about aHttpChannel being null or not are misleading at best.

What that comment is _trying_ to say, I think, is something like this:

  // The aHttpChannel argument is null if this nsPerformanceTiming object is being used
  // for navigation timing (which is only relevant for documents).  It has a non-null
  // value if this nsPerformanceTiming object is being used for resource timing, which
  // can include both document loads, both toplevel and in subframes, and resources
  // linked from a document.

Or something like that.  Please check with Valentin to make sure the comment ends up reflecting reality.
Flags: needinfo?(tanvi)
Comment on attachment 8738727 [details] [diff] [review]
loadingPrincipal-PerformanceChecks-04-06-16.patch

>+  // With bug 1105556, principal will no longer exist for TYPE_DOCUMENT loads.

loadingPrincipal, not triggeringPrincipal, right?

I don't think there's a point in mentioning the bug number.  Just say:

// TYPE_DOCUMENT loads have no loadingPrincipal.  And that's OK, because we
// never actually need to have a performance timing entry for TYPE_DOCUMENT
// loads.

assuming that's actually what's going on.  Is that right, though?  Note that we have known bugs about not exposing the right things via performance.getEntriesByName("document")/performance.getEntriesByType("navigation"); we should make sure that whatever we do here does not preclude fixing those bugs...

r=me if you fix this comment, double-check with Valentin whether we should return true or false in the TYPE_DOCUMENT case (or whether we should change things so that this code is not reached in that case!), get those other comments updated to reflect reality.
Attachment #8738727 - Flags: review?(bzbarsky) → review+
Attachment #8738727 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8738744 [details] [diff] [review]
docshell-principal-changes-04-06-16C.patch

>   // Use nsPIDOMWindow since we _want_ to cross the chrome boundary if needed

This comment should go away, since the code it referred to is gone, no?

>+    // The below comment no longer appears to be true since we only use this
>+    // for frames. Okay to remove comment?-->

Yes, the comment is no longer true and should go.

>+      /  Should this code be moved up before the call to content policies?

This is basically the "browser UI loads data: URI in the browser, it should inherit the current page's principal" case, right?

I honestly don't think it matters too much what we do with content policies there...

>+  //   is our mScriptGlobal. We pass null for laodingPrincipal in this case.

"loadingPrincipal".

>+      // This is a top-level load and mScriptGlobal's frame element is null,

No, it's NOT a toplevel load, right?  The old comment you deleted said "If this isn't a top-level load" in this case.  Please do pu the "If" back too, since you kept the corresponding "then".

>+  // get triggeringPrincipal.  This code should be updated by bug 1181370.

"Get" at start of sentence.

>+  // until then, we cannot rely on the triggeringPrincipal for TYPE_DOCUMENT

"Until".


r=me
Attachment #8738744 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 93

2 years ago
Created attachment 8739705 [details] [diff] [review]
loadingPrincipal-PerformanceChecks-04-09-16.patch

I spoke to Valentin and he said to update the comments per Boris' suggestions, and r+'ed the patch.  He said returning false is the right thing to do for TYPE_DOCUMENT loads.  Comments are updated.  Carrying over r+'s.

(In reply to Boris Zbarsky [:bz] from comment #91)
> Just say:
> 
> // TYPE_DOCUMENT loads have no loadingPrincipal.  And that's OK, because we
> // never actually need to have a performance timing entry for TYPE_DOCUMENT
> // loads.
> 
> assuming that's actually what's going on.  Is that right, though?  Note that
> we have known bugs about not exposing the right things via
> performance.getEntriesByName("document")/performance.
> getEntriesByType("navigation"); we should make sure that whatever we do here
> does not preclude fixing those bugs...
> 
I'm not sure what Boris is referring to with these other known bugs.  But Valentin didn't mention making any other changes and said returning false is the right thing to do for TYPE_DOCUMENT loads.  Valentin, if there is something more to do here, please let me know.
Attachment #8738727 - Attachment is obsolete: true
Flags: needinfo?(tanvi)
Attachment #8739705 - Flags: review+
(In reply to Tanvi Vyas [:tanvi] from comment #93)
> > we have known bugs about not exposing the right things via
> > performance.getEntriesByName("document")/performance.
> > getEntriesByType("navigation"); we should make sure that whatever we do here
> > does not preclude fixing those bugs...
> > 
> I'm not sure what Boris is referring to with these other known bugs.  But
> Valentin didn't mention making any other changes and said returning false is
> the right thing to do for TYPE_DOCUMENT loads.  Valentin, if there is
> something more to do here, please let me know.

I don't think I know about these bugs. If Boris were to post the bug numbers I think I could take a look.
(Assignee)

Comment 95

2 years ago
Created attachment 8739709 [details] [diff] [review]
docshell-loadinfo-principal-changes-04-09-16.patch

Updated the docshell code per Boris' comments.  carrying over r+.  And I'll also r? Jonas.

I've also separated out the single patch into two separate patches:
* nsDocShell::InternalLoad changes to ContentPolicy parameters
* nsDocShell::DoURILoad changes to LoadInfo parameters
Attachment #8738744 - Attachment is obsolete: true
Attachment #8738744 - Flags: review?(jonas)
Attachment #8739709 - Flags: review+
(Assignee)

Comment 96

2 years ago
Created attachment 8739710 [details] [diff] [review]
docshell-contentpolicy-changes-04-09-16.patch

Jonas and I are having some discussions with Bill about the parameter changes to ContentPolicy in nsDocShell::InternalLoad.  Depending on what we decide, we may need to move this patch and the simplecontentpolicy-test-fix patch into a separate bug.

For now, carrying over bz's r+ and r? to Jonas.
Attachment #8739710 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8739709 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Attachment #8739710 - Flags: review?(jonas)
(Assignee)

Comment 97

2 years ago
Created attachment 8739715 [details] [diff] [review]
xpcshell-test-fixes-04-09-16.patch

3 xpcshell tests create TYPE_DOCUMENT channels via javascript and go through nsIOService.  IOService uses the main LoadInfo constructor.  Since we have added an assert to ensure that constructor doesn't get called for TYPE_DOCUMENT loads, those xpcshell tests fail.  Jonas advised to set a preference in those xpcshell tests and check it in the LoadInfo constructor.  This patch does that.  Jonas, what do you think?
Attachment #8739715 - Flags: review?(jonas)
(Assignee)

Comment 98

2 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #97)
> Created attachment 8739715 [details] [diff] [review]
> xpcshell-test-fixes-04-09-16.patch
> 
> 3 xpcshell tests create TYPE_DOCUMENT channels via javascript and go through
> nsIOService.  IOService uses the main LoadInfo constructor.  Since we have
> added an assert to ensure that constructor doesn't get called for
> TYPE_DOCUMENT loads, those xpcshell tests fail.  Jonas advised to set a
> preference in those xpcshell tests and check it in the LoadInfo constructor.
> This patch does that.  Jonas, what do you think?

Because the skipContentTypeCheck is only updated in debug builds, I'm getting an unused variable error in non-debug builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e2662132319&selectedJob=19225667

Jonas, is there a way around this, or should I write the code in a different way?
Flags: needinfo?(jonas)
> I don't think I know about these bugs. If Boris were to post the bug numbers I think I could take a look.

Um... they were discussed at length on public-web-perf, so I assumed you knew about them.  Looks like they were not filed, though.  I've gone ahead and filed bug 1263722.  The upshot is that there should be a PerformanceEntry for the document load itself.
Oh, and that PerformanceEntry is a PerformanceResourceTiming (and a PerformanceNavigationTiming), which is why it's relevant here, since afaict it will need to hold on to the nsPerformanceTiming for the document.
(Assignee)

Comment 101

2 years ago
Comment on attachment 8739710 [details] [diff] [review]
docshell-contentpolicy-changes-04-09-16.patch

Spoke to Jonas and I need to make some changes.  So taking off the r?
Attachment #8739710 - Flags: review?(jonas)
(Assignee)

Comment 102

2 years ago
(In reply to Tanvi Vyas [:tanvi] from comment #98)
> (In reply to Tanvi Vyas [:tanvi] from comment #97)
> > Created attachment 8739715 [details] [diff] [review]
> > xpcshell-test-fixes-04-09-16.patch
> > 
> > 3 xpcshell tests create TYPE_DOCUMENT channels via javascript and go through
> > nsIOService.  IOService uses the main LoadInfo constructor.  Since we have
> > added an assert to ensure that constructor doesn't get called for
> > TYPE_DOCUMENT loads, those xpcshell tests fail.  Jonas advised to set a
> > preference in those xpcshell tests and check it in the LoadInfo constructor.
> > This patch does that.  Jonas, what do you think?
> 
> Because the skipContentTypeCheck is only updated in debug builds, I'm
> getting an unused variable error in non-debug builds:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=0e2662132319&selectedJob=19225667
> 
> Jonas, is there a way around this, or should I write the code in a different
> way?

I just need to move the variable declaration into the ifdef debug.
Flags: needinfo?(jonas)
(Assignee)

Comment 103

2 years ago
Created attachment 8740528 [details] [diff] [review]
xpcshell-test-fixes-04-12-16.patch
Attachment #8739715 - Attachment is obsolete: true
Attachment #8739715 - Flags: review?(jonas)
Attachment #8740528 - Flags: review?(ckerschb)
Comment on attachment 8739709 [details] [diff] [review]
docshell-loadinfo-principal-changes-04-09-16.patch

Review of attachment 8739709 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +10634,3 @@
>    nsCOMPtr<nsIPrincipal> loadingPrincipal;
> +
> +  if (aContentPolicyType != nsIContentPolicy::TYPE_DOCUMENT) {

Nit, it's just easier to read

if (X) {
  do stuff if X
} else {
  do stuff if not X
}

than

if (!X) {
  do stuff if !X
} else {
  do stuff if not !X
}


So I'd do
if (TYPE_DOCUMENT) {
  // type document case
} else {
  // frame case
}

or

if (TYPE_IFRAME || TYPE_FRAME) {
  // frame case
} else {
  // type document case
}


Up to you though.
Attachment #8739709 - Flags: review?(jonas) → review+
(Assignee)

Comment 105

2 years ago
Comment on attachment 8739710 [details] [diff] [review]
docshell-contentpolicy-changes-04-09-16.patch

After discussion with Bill and Jonas, obsoleting this patch and moving changes to requestingContext to bug 1264137
Attachment #8739710 - Attachment is obsolete: true
Attachment #8739710 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8737413 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Summary: Set correct loadingPrincipal, loadingNode, and requestingContext in docshell → Set correct loadingPrincipal and loadingNode in docshell
Comment on attachment 8737443 [details] [diff] [review]
loadingPrincipal-ContentSecurityManager-04-01-16B.patch

Review of attachment 8737443 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM,r=me
Attachment #8737443 - Flags: review?(ckerschb) → review+
Comment on attachment 8738724 [details] [diff] [review]
test-fixes-04-06-16.patch

Review of attachment 8738724 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: docshell/base/nsDocShell.cpp
@@ +1586,5 @@
>    uint32_t loadType = LOAD_NORMAL;
> +  // bz / ckerschb:
> +  // Is this the loadingPrincipal or the triggeringPrincipal?  Since it is
> +  // coming from owner, it is likely the trigger.  But we are passing it
> +  // as the loader to NS_NewInputStreamChannel

I don't fully understand the question. As far as I understand there is only one version of NS_NewInputStreamChannel which takes a loadingPrincipal which is equivalent to the triggeringPrincipal. Do you just want to rename requestingPrincipal to triggeringPrincipal?

::: netwerk/base/nsNetUtil.cpp
@@ +2258,5 @@
>        // Please note that cross origin top level navigations are not subject
>        // to upgrade-insecure-requests, see:
>        // http://www.w3.org/TR/upgrade-insecure-requests/#examples
> +      // Compare the principal we are navigating to (aChannelResultPrincipal)
> +      // with the referring/triggering Principal.  ckerschb to review

jit: don't forget to remove the 'ckerschb to review' from the comment before landing.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1347,5 @@
>    nsCOMPtr<nsIPrincipal> principal = originalLoadInfo->LoadingPrincipal();
> +  MOZ_ASSERT(principal &&
> +             originalLoadInfo->GetExternalContentPolicyType() !=
> +               nsIContentPolicy::TYPE_DOCUMENT,
> +             "Should not do CORS loads for top-level loads, so a loadingPrincipal should always exist.");

nit: 80 char line overflows.

::: netwerk/test/mochitests/mochitest.ini
@@ +40,5 @@
>  [test_idn_redirect.html]
>  [test_origin_attributes_conversion.html]
>  skip-if = e10s || buildapp != 'browser'
>  [test_about_blank_to_signed_web_packaged_app.html]
> +skip-if = e10s || buildapp != 'browser' || os == 'win' || os == 'linux' || os == 'mac' #disabling until removed in Bug 1260876

just curious, any particular reason you are not using skip-if = true?
You are disabling the test everywhere, or not?
Attachment #8738724 - Flags: review?(ckerschb) → review+
Comment on attachment 8740528 [details] [diff] [review]
xpcshell-test-fixes-04-12-16.patch

Review of attachment 8740528 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8740528 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 109

2 years ago
Created attachment 8741109 [details] [diff] [review]
docshell-loadinfo-principal-changes-04-13-16.patch

Updated nits per Jonas' suggestions.  Carrying over r+ from Jonas and Boris.
Attachment #8739709 - Attachment is obsolete: true
Attachment #8741109 - Flags: review+
(Assignee)

Comment 110

2 years ago
Created attachment 8741157 [details] [diff] [review]
test-fixes-04-13-16.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #107)
> Comment on attachment 8738724 [details] [diff] [review]
> test-fixes-04-06-16.patch
> 
> Review of attachment 8738724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +1586,5 @@
> >    uint32_t loadType = LOAD_NORMAL;
> > +  // bz / ckerschb:
> > +  // Is this the loadingPrincipal or the triggeringPrincipal?  Since it is
> > +  // coming from owner, it is likely the trigger.  But we are passing it
> > +  // as the loader to NS_NewInputStreamChannel
> 
> I don't fully understand the question. As far as I understand there is only
> one version of NS_NewInputStreamChannel which takes a loadingPrincipal which
> is equivalent to the triggeringPrincipal. Do you just want to rename
> requestingPrincipal to triggeringPrincipal?
> 

I see what you are saying.  loadingPrincipal=triggeringPrincipal in this case, so what we call it won't make a difference in the logic.  Technically, this is coming from the owner so it is the triggeringPrincipal.  It also falls back to system.  But if we pass it is to the "loadingPrincipal" parameter in NS_NewInputStreamChannel() then that is also confusing.  So I'm going to leave this as is right now and remove the comment.

> ::: netwerk/protocol/http/nsCORSListenerProxy.cpp
> @@ +1347,5 @@
> >    nsCOMPtr<nsIPrincipal> principal = originalLoadInfo->LoadingPrincipal();
> > +  MOZ_ASSERT(principal &&
> > +             originalLoadInfo->GetExternalContentPolicyType() !=
> > +               nsIContentPolicy::TYPE_DOCUMENT,
> > +             "Should not do CORS loads for top-level loads, so a loadingPrincipal should always exist.");
> 
> nit: 80 char line overflows.
> 
I can't fix this without breaking the MOZ_ASSERT macro.

> ::: netwerk/test/mochitests/mochitest.ini
> @@ +40,5 @@
> >  [test_idn_redirect.html]
> >  [test_origin_attributes_conversion.html]
> >  skip-if = e10s || buildapp != 'browser'
> >  [test_about_blank_to_signed_web_packaged_app.html]
> > +skip-if = e10s || buildapp != 'browser' || os == 'win' || os == 'linux' || os == 'mac' #disabling until removed in Bug 1260876
> 
> just curious, any particular reason you are not using skip-if = true?
> You are disabling the test everywhere, or not?

Yes you are right.  But the test has now been removed in bug 1260876.

Updated the comments and carrying over r+ from Jonas and Christoph
Attachment #8738724 - Attachment is obsolete: true
Attachment #8741157 - Flags: review+
(Assignee)

Comment 111

2 years ago
Try looks pretty good, so I'm going to land this.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0080dbea7eba
Jonas, Tanvi: following up on our conversation yesterday. For toplevel loads we want the triggeringPrincipal to be the codebaseprincipal of the typed in URL, right? (Unless it's data:, javascript:, etc).

Isn't that basically what this code [1] was doing modulo the fact that it was named the loadingPrincipal. Potentially we should just bring back that piece of code and massage it to fit our needs. E.g. only create a new principal if loading TYPE_DOCUMENT. What do you think?


[1] https://hg.mozilla.org/mozilla-central/diff/573ce84e7547/docshell/base/nsDocShell.cpp#l1.100
Flags: needinfo?(tanvi)
Flags: needinfo?(jonas)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #114)
> Jonas, Tanvi: following up on our conversation yesterday. For toplevel loads
> we want the triggeringPrincipal to be the codebaseprincipal of the typed in
> URL, right? (Unless it's data:, javascript:, etc).

Only if the load is coming from someplace other than a webpage. If a webpage triggers a top-level load, for example using window.open, or using <a target=_blank> or <a target=_top>, we want the triggeringPrincipal to be that webpage.

For non-webpage-initiated toplevel loads we want the triggeringPrincipal to be a codebase principal, a NullPrincipal or a system principal as per comment 12.

I don't have an opinion about how to accomplish this.
Flags: needinfo?(jonas)
(Assignee)

Comment 116

2 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #114)
> Jonas, Tanvi: following up on our conversation yesterday. For toplevel loads
> we want the triggeringPrincipal to be the codebaseprincipal of the typed in
> URL, right? (Unless it's data:, javascript:, etc).
> 
> Isn't that basically what this code [1] was doing modulo the fact that it
> was named the loadingPrincipal. Potentially we should just bring back that
> piece of code and massage it to fit our needs. E.g. only create a new
> principal if loading TYPE_DOCUMENT. What do you think?
> 
> 
> [1]
> https://hg.mozilla.org/mozilla-central/diff/573ce84e7547/docshell/base/
> nsDocShell.cpp#l1.100

As Jonas said, we want to createCodebasePrincipal in certain situations, but not all.  If we can determine whether or not we are in a codebaseprincipal scenario within DoURILoad, then we could call createcodebaseprincipal directly there.

One way we might be able to implement is to rely on the owner.  If there is an owner, then this page load was definitely triggered by another page, and we can use owner as triggeringPrincipal.  If we don't have an owner, then we know the user typed the url or clicked on it from chrome (i.e. history or bookmarks).  In that case, we can check the scheme and apply the heuristics in comment 12.  BUT - I am not confident we can rely on owner in this way.  Does an owner always exist if a navigation is triggered by another page?  Is owner always missing for typed or bookmarked urls?  I know you have been making some changes that might help answer this.
Flags: needinfo?(tanvi)
(In reply to Tanvi Vyas [:tanvi] from comment #12)
> * The code that handles aLoadType == LOAD_NORMAL_EXTERNAL likely does
> everything we need to safely treat things like desktop shortcuts and
> commandline options as normal bookmarks.

We do _not_ want to treat external loads like bookmarks. There are things we trust when the user has bookmarked it that could well be malicious when passed in from an external entity. "commandline" options include being launched from other programs (potentially web connected, like a chat program or a downloaded document). For one obvious example javascript: "bookmarklets" are OK, and forbidden from external.
You need to log in before you can comment on or make changes to this bug.