1289097 - Investigate TriggeringPrincipal for toplevel docshell loads where URL is typed into address bar

Status: NEW

(Core :: DOM: Security, defect, P3)

Description

[Christoph Kerschbaumer [:ckerschb]]

A URL that is typed into the address bar (which loads as a toplevel load) should try to create a principal from the typed in URL and use that as the triggeringPrincipal. (LoadingPrincipal should be a nullptr in that case.)

Currently, when typing 'mozilla.org' into the address bar we get:

LOADINFO in docshell {
  aURI: http://mozilla.org/
  loadingPrincipal: nullptr
  triggeringPrincipal: NullPrincipal
  contentPolicyType: 6
}

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Here is a (simplified) stacktrace of the codepath we are going through when typing http://www.mozilla.org/ into the address bar:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffe9a27dee in nsDocShell::DoURILoad (this=0x7fffc2f27800, aURI=0x7fffc35e0000, aOriginalURI=0x0, aLoadReplace=false, aReferrerURI=0x0, 
    aSendReferrer=true, aReferrerPolicy=0, aOwner=0x7fffc3aa3d60, aTypeHint=0x0, aFileName=..., aPostData=0x0, aHeadersData=0x0, aFirstParty=true, 
    aDocShell=0x0, aRequest=0x7fffffff60d0, aIsNewWindowTarget=false, aBypassClassifier=false, aForceAllowCookies=false, aSrcdoc=..., aBaseURI=0x0, 
    aContentPolicyType=6) at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10834
10834	        MOZ_ASSERT(false, "do'h");
(gdb) bt
#0  0x00007fffe9a27dee in nsDocShell::DoURILoad (this=0x7fffc2f27800, aURI=0x7fffc35e0000, aOriginalURI=0x0, aLoadReplace=false, aReferrerURI=0x0, 
    aSendReferrer=true, aReferrerPolicy=0, aOwner=0x7fffc3aa3d60, aTypeHint=0x0, aFileName=..., aPostData=0x0, aHeadersData=0x0, aFirstParty=true, 
    aDocShell=0x0, aRequest=0x7fffffff60d0, aIsNewWindowTarget=false, aBypassClassifier=false, aForceAllowCookies=false, aSrcdoc=..., aBaseURI=0x0, 
    aContentPolicyType=6) at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10834
#1  0x00007fffe9a26e23 in nsDocShell::InternalLoad (this=0x7fffc2f27800, aURI=0x7fffc35e0000, aOriginalURI=0x0, aLoadReplace=false, aReferrer=0x0, 
    aReferrerPolicy=0, aOwner=0x7fffc3aa3d60, aFlags=0, aWindowTarget=0x7fffd372f560 u"", aTypeHint=0x0, aFileName=..., aPostData=0x0, aHeadersData=0x0, 
    aLoadType=1, aSHEntry=0x0, aFirstParty=true, aSrcdoc=..., aSourceDocShell=0x0, aBaseURI=0x0, aDocShell=0x0, aRequest=0x0)
    at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10581
#2  0x00007fffe9a04454 in nsDocShell::LoadURI (this=0x7fffc2f27800, aURI=0x7fffc35e0000, aLoadInfo=0x7fffc4f9db30, aLoadFlags=2883584, aFirstParty=true)
    at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:1565
#3  0x00007fffe9a0fde1 in nsDocShell::LoadURIWithOptions (this=0x7fffc2f27800, aURI=0x7fffc4bdba60 u"http://www.mozilla.org/", 
    aTriggeringPrincipal=0x7fffc4b0f670, aLoadFlags=0, aReferringURI=0x0, aReferrerPolicy=0, aPostStream=0x0, aHeaderStream=0x0, aBaseURI=0x0)
    at /home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:4828
#4  0x00007fffe554f9e5 in NS_InvokeByIndex (that=0x7fffc2f279b0, methodIndex=9, paramCount=8, params=0x7fffffff6ab8)
    at /home/ckerschb/moz/mc/xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:182
...


(gdb) call DumpJSStack()
[Thread 0x7fffccd77700 (LWP 6416) exited]
[Thread 0x7fffbc2ff700 (LWP 6441) exited]
0 _loadURIWithFlags(browser = [object XULElement], uri = "http://www.mozilla.org/", params = [object Object]) ["chrome://browser/content/browser.js":841]
    this = [object ChromeWindow]
1 loadURIWithFlags(aURI = "http://www.mozilla.org/", aTriggeringPrincipal = [object Object], aFlags = undefined, aReferrerURI = undefined, aCharset = undefined, aPostData = undefined) ["chrome://browser/content/tabbrowser.xml":6913]
    this = [object XULElement]
2 loadURIWithFlags(aURI = "http://www.mozilla.org/", aTriggeringPrincipal = [object Object]) ["chrome://browser/content/tabbrowser.xml":4008]
    this = [object XULElement]
3 openLinkIn(url = "http://www.mozilla.org/", where = "current", params = [object Object]) ["chrome://browser/content/utilityOverlay.js":362]
    this = [object ChromeWindow]
4 openUILinkIn(url = "http://www.mozilla.org/", where = "current", aAllowThirdPartyFixup = [object Object]) ["chrome://browser/content/utilityOverlay.js":199]
    this = [object ChromeWindow]
5 handleCommand/continueOperation/loadCurrent() ["chrome://browser/content/urlbarBindings.xml":398]
6 continueOperation() ["chrome://browser/content/urlbarBindings.xml":448]
    this = [object XULElement]
7 handleCommand/<(response = http://www.mozilla.org/,,false) ["chrome://browser/content/urlbarBindings.xml":376]
8 _canonizeURL/<(data = [object Object]) ["chrome://browser/content/urlbarBindings.xml":528]
9 Handler.prototype.process() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":937]
    this = [object Object]
10 this.PromiseWalker.walkerLoop() ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":816]
    this = [object Object]
11 bound walkerLoop() ["self-hosted":894]
    this = [object Object]
12 bound bound walkerLoop() ["self-hosted":894]
    this = null
13 this.PromiseWalker.scheduleWalkerLoop/<(undefined) ["resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js":750]

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1289097_update_triggering_address_bar.patch

Smaug, after several discussions Tanvi, Jonas, and myself (hopefully also bz) agreed that we should try to create a Principal using the typed in URL and use that principal as the triggeringPrincipal (see also [1,2]) for toplevel loads. In other words the loadInfo when typing http://mozilla.org/ into the address bar should look like this:

LOADINFO in docshell {
  aURI: http://mozilla.org/
  loadingPrincipal: nullptr
  triggeringPrincipal: http://mozilla.org/
  contentPolicyType: 6
}

Here are a couple of questions which I hope you can help me answer:
1) Is extending loadURIWithOptions() with |aTriggeringPrincipal| argument something we might consider? (please note that we are going to replace owner with triggeringPrincipal within bug 1286472).

2) For the specific example described in comment 0, is the attached patch, and in particular the code within browser/base/content/utilityOverlay.js something we might consider?

3) Applying the attached patch, typing http://mozilla.org/ into the address bar would generate the right triggeringPrincipal but then that principal would be overwritten here [3]. That whole block starting at line 1482 seems complicated and fragile. Do you think it's even possible to push the required logic into each callsite? Or is there any other 'correct' possibility that LOAD_FLAGS_DISALLOW_INHERIT_OWNER is not going to overwrite the triggeringPrincipal (owner) with a nullPrincipal?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1181370#c7
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1181370#c35
[3] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1508

Comment 3

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

"Typed URL"? What if there is uri fixup happening? Would the "Typed URL" be created after that?
Looks like loadURIWithOptions is doing fixup, so why would you pass any new principal before that?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #3)
> "Typed URL"? What if there is uri fixup happening? Would the "Typed URL" be
> created after that?
> Looks like loadURIWithOptions is doing fixup, so why would you pass any new
> principal before that?

Well, that was basically the main question. Is it even worth creating a principal and pass it as owner (triggeringprincipal) to docshell if it's fixed up within docshell anyway? Reading between the lines of your answer, it's not worth it, right?

Comment 5

[Tanvi Vyas[:tanvi]]

Chris, is this bug supposed to include the below parts of comment 7 https://bugzilla.mozilla.org/show_bug.cgi?id=1181370#c7?  I don't think you can split the below up by much.

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #7)
> * 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.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas - behind on reviews [:tanvi] from comment #5)
> Chris, is this bug supposed to include the below parts of comment 7
> https://bugzilla.mozilla.org/show_bug.cgi?id=1181370#c7?  I don't think you
> can split the below up by much.

Yes, this bug will fix up the triggeringPrincipal for http, data, javascript, etc. loads.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1289097_triggeringprincipal_address_bar_tests.patch

Gijs, I am trying to craft a test to investigate the triggeringPrincipal for toplevel loads. I am not super familiar with all these BrowserTestUtils functions and was wondering if you could provide some feedback on what I got so far. The test seems to work, but I am not sure if that is the state of the art of writing browser tests. Additionally it would be great if we could also test chrome://, resource:// and ftp:// schemes, but I don't know how feasible that is. 

In general, is that the way to simulate a user typing a URL into the address bar or is there a better way to accomplish this?

Comment 8

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> A URL that is typed into the address bar (which loads as a toplevel load)
> should try to create a principal from the typed in URL and use that as the
> triggeringPrincipal. (LoadingPrincipal should be a nullptr in that case.)

Why do we want this?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8782109 [details] [diff] [review]
bug_1289097_triggeringprincipal_address_bar_tests.patch

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

::: docshell/test/navigation/browser_test_triggeringprincipal_addressbar.js
@@ +76,5 @@
> +    let tab = gBrowser.addTab(BASE_SITE_URI);
> +    let browser = gBrowser.getBrowserForTab(tab);
> +
> +    yield BrowserTestUtils.browserLoaded(browser);
> +    yield BrowserTestUtils.loadURI(browser, curTest.uri);

No, this just calls loadURI directly on the XUL <browser> element. That could happen through any number of reasons. It's not at all equivalent to location bar loads

TBH, it's not clear to me what the purpose here is and what the scope would be. There's a wiiiide spectrum / grey area of loads that I'm not at all clear this would make sense for.

What about:
- loads triggered by external programs (ie passed to us from the OS)
- loads triggered by bookmarks
- loads triggered by drag/drop from the OS
- loads triggered by drag/drop from Firefox (not sure we can distinguish these!)
- loads triggered by clicking links in the preferences, or certain Firefox menu items
- loads triggered by about:synced-tabs or similar UI for synced tabs / history
- loads triggered by middle clicking links
- loads triggered by right clicking links and opening them in a new tab / private window / container tab
- loads triggered by typing an invalid URI in the location bar (e.g., http://[invalidipv6literal]/ isn't allowed to load about:neterror, so how would that work?)
- loads triggered by some keywords that cause a web search to be executed - is the triggering principal just the same as the URI?

More generally, if the triggering principal is always the same as the URI, that doesn't really seem like it gets you any extra security guarantees or information over just not providing the triggering principal at all. It might also confuse the case of clicking a link from foo.com to open foo.com with opening foo.com from the location bar. Finally, it means that if I can trick the user into using any of these means to open a privileged URI, I might be able to use that to privelege escalate.

IOW, I think this is a huge project and not necessarily a good idea.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

The main idea is that it *might* be interesting for a security policy to know which exact document caused the top-level page load. Since we don't have a triggeringNode within loadinfo, but only a triggeringPrincipal, we (Jonas, Tanvi) agreed that it might be a good idea to generate a codebasePrincipal for top level loads when the user types a URL into the address bar. (see comment 5 for special handling of schemes like data:, javascript:, etc.).

Gijs, you make a valid statement within comment 9 and I personally don't feel that strongly, I would be fine with leaving the triggeringPrincipal as is. Getting all the different cases right might be time consuming and quite an undertaking in the end. Smaug already raised his concerns about changing the the triggeringPrincipal over IRC a while ago when I flagged him for review on the initial patch.

Before moving forward with this bug I would like Tanvi and also Boris (he is on PTO and not accepting ni? at the moment) to weigh in the pros and cons of changing the triggeringPrincipal for top level loads. Potentially we should just convert docshell to rely on AsyncOpen2() [Bug 1182569] without changing the TriggeringPrincipal within this bug.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Smaug, if you have an opinion on Comment 10 (see also comment 9), please feel free to add your thoughts!

Comment 12

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

I think I mostly agree with Gijs. Giving triggeringPrincipal this kind of second meaning is rather confusing.

"The main idea is that it *might* be interesting for a security policy to know which exact document caused the top-level page load."
Well, if triggeringPrincipal is just generated from user input, security policy wouldn't really know the document which caused top-level load, since triggeringPrincipal would mean something else. The principal would have nothing to do with any document.

Comment 13

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

For navigations triggered by webpages having a triggeringPrincipal makes a lot of sense.

For example, if a webpage contains a <a href="..."> or a <form action="...">, or if it calls otherWindow.location = ... or window.open(...), it is very useful to know which page initiated the network request that we're about to do.

So for navigations triggered from webpages the triggeringPrincipal is commonly different from the URI loaded.

Having this information consistently would let us simplify referrer logic (by setting it automatically using the triggeringPrincipal). It would let us simplify CheckLoadURI() security checks (by moving them entirely into AsyncOpen2()). And in the future we could allow addons to implement custom security policies (no one may link to my bank).

So the question is what to use for navigations trigged through other means, for example when the user clicks a bookmark. For bookmarks, the theoretically most correct solution would be to remember the triggeringPrincipal that was used when the bookmarked page was originally loaded. However that would require significant rearchitecturing of the bookmark system, including the places database that we use to hold bookmark information. So that doesn't seem like a realistic solution.

Instead we proposed to generate a triggeringPrincipal which will make the systems which use triggeringPrincipals do the right thing, to the largest extent possible. Hence the proposal to generate a nsIPrincipal from the URI being loaded.

Other proposals welcome. But please keep in mind that triggering principals are not just used for navigations, but also for resource usage, such as when loading images and stylesheets.

Ultimately I think the decision here is owned by Tanvi.

Comment 14

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

(In reply to :Gijs Kruitbosch (away 26-29 incl.) from comment #9)
> What about:
> - loads triggered by external programs (ie passed to us from the OS)
> - loads triggered by drag/drop from the OS
> - loads triggered by drag/drop from Firefox (not sure we can distinguish
> these!)
> - loads triggered by clicking links in the preferences, or certain Firefox
> menu items
> - loads triggered by some keywords that cause a web search to be executed -
> is the triggering principal just the same as the URI?

For all of these, I think that the triggeringPrincipal should be generated from the loaded URI yes.

> - loads triggered by bookmarks
> - loads triggered by about:synced-tabs or similar UI for synced tabs /
> history

Ideally we'd track the triggeringPrincipal along with the bookmarked/history/synced uri. However that seems like an enormous amount of work for only theoretical benefit. So I think it's fine to generate the triggeringPrincipal from the URI here yes.

> - loads triggered by typing an invalid URI in the location bar (e.g.,
> http://[invalidipv6literal]/ isn't allowed to load about:neterror, so how
> would that work?)

I honestly don't know enough about how error pages are implemented to answer this one. What triggeringPrincipal we use here doesn't really matter as long as the error page is loaded. No critical security checks need to happen here anyway.

> - loads triggered by middle clicking links
> - loads triggered by right clicking links and opening them in a new tab /
> private window / container tab

In the cases where we want to send a referrer we need to grab the correct triggeringPrincipal. I.e. the principal of the page that contained the link.

In cases where we don't want to send a referrer, which I imagine at least includes opening in a private window and opening in a container tab, I think generating the triggeringPrincipal from the URI is fine yes.

This might possibly mean that in all of these cases we can generate the triggeringPrincipal from the URI.

> More generally, if the triggering principal is always the same as the URI,
> that doesn't really seem like it gets you any extra security guarantees or
> information over just not providing the triggering principal at all.

It won't always be the same. For all the cases when a webpage can trigger a network load, be that by causing a navigation, or by loading an image, the triggeringPrincipal will be a "real" principal.

> It might also confuse the case of clicking a link from foo.com to open foo.com
> with opening foo.com from the location bar.

I think having a flag on nsILoadInfo indicating of the triggeringPrincipal is "generated" or not could be a good idea. But I don't think we currently have a need for that. Though once we move referrer setting to be done using the triggeringPrincipal, we would indeed need such a flag.

> Finally, it means that if I can
> trick the user into using any of these means to open a privileged URI, I
> might be able to use that to privelege escalate.

Could you detail this concern more. I agree we should not expose ourselves to privilege escalation risks.

> IOW, I think this is a huge project and not necessarily a good idea.

Generating the triggeringPrincipal from the loaded URI in the case where the load comes from "outside the web platform" seems like the simplest solution. The more correct solution of remembering the triggeringPrincipal which was originally used to load a bookmarked page seems like dramatically more work.

If you have simpler solutions I'm all ears.

Comment 15

[:Gijs (he/him)]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #14)
> > - loads triggered by typing an invalid URI in the location bar (e.g.,
> > http://[invalidipv6literal]/ isn't allowed to load about:neterror, so how
> > would that work?)
> 
> I honestly don't know enough about how error pages are implemented to answer
> this one. What triggeringPrincipal we use here doesn't really matter as long
> as the error page is loaded. No critical security checks need to happen here
> anyway.

I don't believe this is true. Our error pages like about:neterror, about:certerror and about:blocked all take URI parameters and inject that content into the page. They are not directly linkable from the normal web (almost all about: pages are unlinkable from the web at this stage, and if anything that protection needs expanding further) to avoid spoofing issues, but obviously we do need to be able to load them when we want to show the user an error page. So there are security checks, and we should not get rid of them.

> > Finally, it means that if I can
> > trick the user into using any of these means to open a privileged URI, I
> > might be able to use that to privelege escalate.
> 
> Could you detail this concern more. I agree we should not expose ourselves
> to privilege escalation risks.

Well, the proposal is to have UI interactions reuse the target URI as the source URI and use that source URI to make security decisions. So if I'm http://foo.com and I can get you to load "a link" via any of the means that we're discussing, we're going to allow you to open that link because links can always refer to themselves (even null principals can do that). Inasmuch as that is a change from the status quo (which I don't know, because I don't know what we do right now) that would be worrisome.

> > IOW, I think this is a huge project and not necessarily a good idea.
> 
> Generating the triggeringPrincipal from the loaded URI in the case where the
> load comes from "outside the web platform" seems like the simplest solution.
> The more correct solution of remembering the triggeringPrincipal which was
> originally used to load a bookmarked page seems like dramatically more work.
> 
> If you have simpler solutions I'm all ears.

What do we do right now (I expect system principal or null?), and why is that not good enough?

The reason I am hesitant is that it would be a significant quantity of work to fix up all the browser entrypoints that load pages right now. There are a lot of those entrypoints, and a number of levels of indirection (also because of e10s), and adding parameters everywhere doesn't strike me as particularly fun or productive. If the net result of "triggering principal is system principal" or "triggering principal is null" should be "triggering principal is a simple principal of the target URI", that seems like a fix we can make in asyncOpen itself rather than in all the consumers.

What's the benefit of making the same changes to all the callers that provide no real additional information to the callee, and might also confuse those cases where the load was *genuinely* triggered by the same page (e.g. meta refresh that triggers reloading its own page) ? I guess I'm also still misunderstanding the reasoning behind this change, see below...

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #13)
> For navigations triggered by webpages having a triggeringPrincipal makes a
> lot of sense.

Right, I agree.

> Having this information consistently would let us simplify referrer logic
> (by setting it automatically using the triggeringPrincipal).

But if I load mozilla.org from the location bar or a bookmark, should the referrer really be mozilla.org ? I don't think any browser implements that behaviour right now, it feels wrong, and I wouldn't be surprised if in some cases it triggered behaviour changes (or at least statistics changes) on real websites. Why would we want to do this?

> It would let us simplify CheckLoadURI() security checks (by moving them entirely into
> AsyncOpen2()).

But why can't we make asyncopen itself make the assumptions you think we should hardcode into all the callers? What's the difference? :-)

> And in the future we could allow addons to implement custom
> security policies (no one may link to my bank).

If we wanted to do this, I think it would be possible orthogonally to these changes by providing extension points into caps as it is, no? How does it relate to these changes?

Comment 16

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

(In reply to :Gijs Kruitbosch (away 26-29 incl.) from comment #15)
> (In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently
> from comment #14)
> > > - loads triggered by typing an invalid URI in the location bar (e.g.,
> > > http://[invalidipv6literal]/ isn't allowed to load about:neterror, so how
> > > would that work?)
> > 
> > I honestly don't know enough about how error pages are implemented to answer
> > this one. What triggeringPrincipal we use here doesn't really matter as long
> > as the error page is loaded. No critical security checks need to happen here
> > anyway.
> 
> I don't believe this is true. Our error pages like about:neterror,
> about:certerror and about:blocked all take URI parameters and inject that
> content into the page. They are not directly linkable from the normal web
> (almost all about: pages are unlinkable from the web at this stage, and if
> anything that protection needs expanding further) to avoid spoofing issues,
> but obviously we do need to be able to load them when we want to show the
> user an error page. So there are security checks, and we should not get rid
> of them.

What I'm saying is that I don't care what principal we use as triggeringPrincipal when loading about:neterror pages. As long as we don't break any existing functionality of course.

So I'm happy to do whatever solution you think we should do for the case when the url can't be parsed as a valid nsIURI.

> > > Finally, it means that if I can
> > > trick the user into using any of these means to open a privileged URI, I
> > > might be able to use that to privelege escalate.
> > 
> > Could you detail this concern more. I agree we should not expose ourselves
> > to privilege escalation risks.
> 
> Well, the proposal is to have UI interactions reuse the target URI as the
> source URI and use that source URI to make security decisions. So if I'm
> http://foo.com and I can get you to load "a link" via any of the means that
> we're discussing, we're going to allow you to open that link because links
> can always refer to themselves (even null principals can do that). Inasmuch
> as that is a change from the status quo (which I don't know, because I don't
> know what we do right now) that would be worrisome.

Status quo is that we don't do any security checks at all. Well, apart from some very low-level necko security checks which forbids certain protocols (like shell://) and certain http ports from being loaded anywhere by anyone. But those checks are not going anywhere. There are no plans to change how they are implemented.

The proposal here is *not* to make any behavioral changes at all. It's just to finish implementing the nsILoadInfo+AsyncOpen2() security infrastructure that we've been putting in place for all other network requests other than docshell loads. This infrastructure has so far made our security checks substantially more reliable and consistently implemented.

If we want to add security policies to docshell loads, it is likely better to do that on top of the nsILoadInfo+AsyncOpen2 infrastructure. But that will require putting that in place for docshell loads.

> > > IOW, I think this is a huge project and not necessarily a good idea.
> > 
> > Generating the triggeringPrincipal from the loaded URI in the case where the
> > load comes from "outside the web platform" seems like the simplest solution.
> > The more correct solution of remembering the triggeringPrincipal which was
> > originally used to load a bookmarked page seems like dramatically more work.
> > 
> > If you have simpler solutions I'm all ears.
> 
> What do we do right now (I expect system principal or null?), and why is
> that not good enough?

See above. What we do right now is "don't do security checks at all". I.e. it's neither null nor system principal. We simply don't call various security checks at all.

Our goal is to have a very small number of code paths which all network loads go through, and which do security checks. This way we are certain that security checks happen for all network loads. Right now we have no such confidence which is why we on a regular basis find security bugs.

Current status is that such a choke point exists and all loads except docshell loads. The goal of this bug is to move us closer to where docshell loads also go through said choke point. But we don't want to change existing behavior in doing so since that can risk breaking important functionality.

> The reason I am hesitant is that it would be a significant quantity of work
> to fix up all the browser entrypoints that load pages right now. There are a
> lot of those entrypoints, and a number of levels of indirection (also
> because of e10s), and adding parameters everywhere doesn't strike me as
> particularly fun or productive. If the net result of "triggering principal
> is system principal" or "triggering principal is null" should be "triggering
> principal is a simple principal of the target URI", that seems like a fix we
> can make in asyncOpen itself rather than in all the consumers.

My hope is that we can put the code for "generate a triggeringPrincipal using the URI" in a central enough location that very few code changes are needed.

In fact, I was hoping that little to no front-end changes would be needed at all, possibly other than search'n'replace type stuff where we'd add some explicit argument to every callsite of a couple of gecko APIs which trigger a docshell navigation.

> > Having this information consistently would let us simplify referrer logic
> > (by setting it automatically using the triggeringPrincipal).
> 
> But if I load mozilla.org from the location bar or a bookmark, should the
> referrer really be mozilla.org ? I don't think any browser implements that
> behaviour right now, it feels wrong, and I wouldn't be surprised if in some
> cases it triggered behaviour changes (or at least statistics changes) on
> real websites. Why would we want to do this?

No, we don't want to change behavior. Including behavior around referrers. We will have to make sure to not set a referrer in the case when we generate a URI. Either by setting some flag in nsILoadInfo indicating that the 

> > It would let us simplify CheckLoadURI() security checks (by moving them entirely into
> > AsyncOpen2()).
> 
> But why can't we make asyncopen itself make the assumptions you think we
> should hardcode into all the callers? What's the difference? :-)

We could leave the triggeringPrincipal as null here indeed. And make asyncopen do the logic we are discussing. But that would mean that any other consumer of nsILoadInfo will also have to duplicate that same logic.

> > And in the future we could allow addons to implement custom
> > security policies (no one may link to my bank).
> 
> If we wanted to do this, I think it would be possible orthogonally to these
> changes by providing extension points into caps as it is, no? How does it
> relate to these changes?

Necko has had a long-standing problem that once we start a network request, there is next to no information on the nsIChannel allowing anyone to look at it and make security decisions regarding if the request should be allowed or not. The nsIChannel has contained no information about why the request was started, by who it was started, what we plan on doing with the result, etc.

You've been able to do some qualified guesses by hoping that the loadGroup.notificationCallbacks object was a docshell, and that the docshell contained enough information that you can determine if the url should be loaded or not. But that still leave you with no way of knowing if the request is due to a navigation, a resource load, an iframe load, etc. Additionally docshell is notoriously finicky when it comes to state management, so it's even hard to know the url of the document that initiated the load.

The goal of this bug is to finish the nsILoadInfo infrastructure so that we can more sanely build security checks on top of that. Including adding extension points to allow addons to perform such checks.

Comment 17

[:Gijs (he/him)]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #16)
> > > > Finally, it means that if I can
> > > > trick the user into using any of these means to open a privileged URI, I
> > > > might be able to use that to privelege escalate.
> > > 
> > > Could you detail this concern more. I agree we should not expose ourselves
> > > to privilege escalation risks.
> > 
> > Well, the proposal is to have UI interactions reuse the target URI as the
> > source URI and use that source URI to make security decisions. So if I'm
> > http://foo.com and I can get you to load "a link" via any of the means that
> > we're discussing, we're going to allow you to open that link because links
> > can always refer to themselves (even null principals can do that). Inasmuch
> > as that is a change from the status quo (which I don't know, because I don't
> > know what we do right now) that would be worrisome.
> 
> Status quo is that we don't do any security checks at all. Well, apart from
> some very low-level necko security checks which forbids certain protocols
> (like shell://) and certain http ports from being loaded anywhere by anyone.
> But those checks are not going anywhere. There are no plans to change how
> they are implemented.

I mean, docshell and higher layers clearly do security checks, but I guess we're saying we don't do checks on a lower level? I'm not sure how many of those higher level checks can be removed, because a website saying "location.href = 'chrome://whatever/'" should get stopped a lot sooner than after unloading the first page and then finding we're not allowed to request the next one. :-)

> The proposal here is *not* to make any behavioral changes at all. It's just
> to finish implementing the nsILoadInfo+AsyncOpen2() security infrastructure
> that we've been putting in place for all other network requests other than
> docshell loads. This infrastructure has so far made our security checks
> substantially more reliable and consistently implemented.
> 
> If we want to add security policies to docshell loads, it is likely better
> to do that on top of the nsILoadInfo+AsyncOpen2 infrastructure. But that
> will require putting that in place for docshell loads.

So I'm confused. Do we currently use these checks for things like window.location = "http://foo.com/"? <meta refresh> tags? <a href> clicks? All of that stuff is handled in DOM and docshell, in some cases with the same APIs that firefox frontend is using. AFAICT all the docshell loadURI APIs already have an nsIDocshellLoadInfo param that has a triggering+loadingPrincipal. Is Firefox frontend just not passing the right things there? Do the values there get ignored?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I mean, docshell and higher layers clearly do security checks

Docshell itself does not (mostly; see below).  Higher layers may.

> because a website saying "location.href = 'chrome://whatever/'"

In the new setup, this would get stopped at the point when docshell calls AsyncOpen.  This is indeed later than now, so we would have done Stop() and whatnot.  But it's before we've unloaded the previous page.

> Do we currently use these checks for things like window.location = "http://foo.com/"?

Yes.  The check is done in nsLocation::CheckURL.

> <meta refresh> tags?

Yes.  The check is done in nsDocShell::SetupRefreshURIFromHeader (which is in docshell, but not on the shared navigation codepath).

> <a href> clicks?

Yes.  The check is done in nsContentUtils::TriggerLink.

> in some cases with the same APIs that firefox frontend is using.

None of the docshell "do a load" APIs do security checks.  All of the above checks are done before calling into the docshell loading path.

> AFAICT all the docshell loadURI APIs already have an nsIDocshellLoadInfo param that has a
> triggering+loadingPrincipal.

Correct.  They are not currently used for security checks (at least immediately; they do get used for some stuff like navigation timing security checks).  Bug 1182569 is about using them for security checks, so we can do the security check in shared code and remove those three security checks mentioned earlier in this comment.

> Is Firefox frontend just not passing the right things there? Do the values there get ignored?

Both.  The Firefox frontend doesn't have a way to pass in the right things, because nsIDocShell.loadURI and nsIDocShell.internalLoad are both [noscript] and the scriptable APIs are nsIWebNavigation.loadURI/loadURIWithOptions, which do not have the relevant arguments....

Comment 19

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

For something like

location.href = 'chrome://whatever/';

we likely will need to keep at least some CheckLoadURI outside of AsyncOpen2(). Not for security reasons, but for web-compat reasons.

It would mean that something like

location.href = 'http://some-domain-that-is-blocked-by-a-security-addon.com/';

would behave differently than 

location.href = 'chrome://...';

But that's probably ok.

Comment 20

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #18)
> > AFAICT all the docshell loadURI APIs already have an nsIDocshellLoadInfo param that has a
> > triggering+loadingPrincipal.
> 
> Correct.  They are not currently used for security checks (at least
> immediately; they do get used for some stuff like navigation timing security
> checks).  Bug 1182569 is about using them for security checks, so we can do
> the security check in shared code and remove those three security checks
> mentioned earlier in this comment.
> 
> > Is Firefox frontend just not passing the right things there? Do the values there get ignored?
> 
> Both.  The Firefox frontend doesn't have a way to pass in the right things,
> because nsIDocShell.loadURI and nsIDocShell.internalLoad are both [noscript]
> and the scriptable APIs are nsIWebNavigation.loadURI/loadURIWithOptions,
> which do not have the relevant arguments....

OK. So are we saying we need to add optional 'in' arguments on these methods so consumers can pass nsIDocshellLoadInfo, and generate a default one (ie == simple target URI principal) within docshell? That would likely mean only very very few browser/ consumers would need changing (ie the ones where we are somehow "redirecting" a load from content code through chrome code to trigger a navigation -- which hopefully all have security checks already...). Though even then, we'll likely need to add forwarding infra because of e10s and RemoteWebNavigation and friends.

Or am I still not 'getting it'? :-)

Comment 21

[Boris Zbarsky [:bzbarsky]]

That sounds like a plausible way forward, yes....

Comment 22

[Tanvi Vyas[:tanvi]]

We are trying to determine whether we need to do this bug before landing bug 1182569, which converts docshell to using AsyncOpen2.  Right now, there are cases where we don't have a triggeringPrincipal (the cases mentioned in this bug and in bug 1240258).  When we don't have a triggeringPrincipal and don't have aReferrer, we fallback to systemPrincipal[1]

What are the security implications of falling back to systemPrincipal for TYPE_DOCUMENT loads?  (For TYPE_SUBDOCUMENT loads we should add an assertion that requires triggeringPrincipal and doesn't allow a fallback to system.)  After the conversion in bug 1182569, loadInfo->triggeringPrincipal will be passed into NS_CheckContentLoadPolicy.  Before the conversion, if there is no triggeringPrincipal or referrer available, we would pass in a nullptr to NS_CheckContentLoadPolicy[2].  Hence, after the conversion, if there is no triggeringPrincipal or referrer available, we will pass in systemPrincipal to NS_CheckContentLoadPolicy instead of nullptr.

I don't think this is an issue for our internal Content Policies, as most of them return early for TYPE_DOCUMENT.  But it may be for external ones in addons.

What are the other implications of falling back to systemPrincipal for triggeringPrincipal for TYPE_DOCUMENT loads?  Are there any, or is the one I mentioned above the only one?

Should we worry about the addon case, or just move forward?

Adding dveditz, who can also take a look at this.

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10811
[2] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9841

Comment 23

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

> What are the security implications of falling back to systemPrincipal

The docs should answer that question quite clearly these days.

https://hg.mozilla.org/mozilla-central/file/tip/netwerk/base/nsILoadInfo.idl#l244

Comment 24

[Daniel Veditz [:dveditz]]

(In reply to Tanvi Vyas [:tanvi] from comment #22)
> I don't think this is an issue for our internal Content Policies, as most of
> them return early for TYPE_DOCUMENT.  But it may be for external ones in addons.

AsyncOpen2 doesn't just check ContentPolicies, it also calls CheckLoadURI. But several add-ons will certainly care if their content policies are skipped for document loads.

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #23)
> > What are the security implications of falling back to systemPrincipal
> 
> The docs should answer that question quite clearly these days.

Indeed they do:
  There will be no security checks on the initial load or any subsequent redirects.
  This means there will be no nsIContentPolicy checks or any CheckLoadURI checks.
  Because of this, never set the triggeringPrincipal to the system principal when
  the URI to be loaded is controlled by a webpage.

"Or external program" I should add. If we don't know where a load came from then system principal is dangerous; it's an unsafe fallback. If we don't know what triggered the load then we need to assume a null principal and fix what breaks. The claim is that docshell currently doesn't do security checks so systemPrincipal is safe, but we're also talking about removing the higher level security checks in favor of new ones (AsyncOpen2) in docshell. Fail closed.

WRT the specific summary of this bug ("where URL is TYPED into address bar") then sure, use the system principal--IFF you know the user typed it. And bookmarks, on the assumption the user put them there. But if we can't tell the difference between a user and an external program then we need to keep security checks at the point where we _can_ tell.

Comment 25

[Tanvi Vyas[:tanvi]]

Chris and I spoke to Dan about what could happen if we skipped CheckLoadURI for a TYPE_DOCUMENT load when the triggeringPrincipal was set to system but the load actually came from web content or an external program.

An attacker could trick the user into saving a malicious file.  If the attacker knew wehre the file was saved (ex: default directory), then web content could window.open that file.  If the file load had systemPrincipal instead of the principal of the malicious web content, then the browser would open the file, which could contain malicious script that is then run by the browser.  An attacker could also use a trick like this help identify a user by trying to determine if a specific file exists on a users machine.

There may be other threats, but this is the attack we thought of.  Dan, please let me know if something is missing here.

I believe Chris already has mochitests that ensure window.open loads have the right triggeringPrincipal.

Dan's suggestion as a path forward to land bug 1182569 (convert docshell) without fixing this bug, is to manually go through the list from Gijs in https://bugzilla.mozilla.org/show_bug.cgi?id=1289097#c9, printing out triggeringPrincipals and ensure that we have a sane triggeringPrincipal in each case.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #25)
> Dan's suggestion as a path forward to land bug 1182569 (convert docshell)
> without fixing this bug, is to manually go through the list from Gijs in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1289097#c9, printing out
> triggeringPrincipals and ensure that we have a sane triggeringPrincipal in
> each case.

That sounds like a good path forward to me. As discussed in our meeting today, I don't have the cycles to look into this bug at the moment. Unassigning myself so someone else can pick it up.