1182569 - Use asyncOpen2() for docshell loads (nsJSProtocolHandler and nsURILoader)

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Once we are ready for docshell we have to update that callsite within the same changeset:
http://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsURILoader.cpp#825

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Before we can start looking at docshell we have to make sure we are using the right principals, hence I marked bug 1105556 as blocking this one.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_nsuriloader.patch

Comment 4

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

Given that this is only called by docshell, I think it might make more sense to change this along with the docshell code.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8664048 [details] [diff] [review]
bug_1182569_nsuriloader.patch

(In reply to Jonas Sicking (:sicking) from comment #4)
> Given that this is only called by docshell, I think it might make more sense
> to change this along with the docshell code.

Making a good point there. For some reason I thought this code is also called from nsObjectLoadingContent, but that was uriLoader->OpenChannel. Unflagging for now!

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_use_asyncOpen2_for_docshell_loads.patch

First WIP patch to see what we have to change when converting docshell to rely on asyncOpen2.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=965a3239e73d

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bff6b3e90e5

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c0f384b851

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Gijs, when using asyncOpen2() a few tests, like e.g. docshell/test/chrome/test_bug789773.xul break. Reason is that before converting docshell to rely on asyncOpen2() we haven't called CheckLoadURIWIthPrincipal.

For test_bug789773.xul in particular we call CheckLoadURIWithPrincipal using
> principal: chrome://mochitests/content/chrome/docshell/test/chrome/test_bug789773.xul
> uri: about:mozilla

which then blocks the load because about:mozilla does not use 'nsIAboutModule::MAKE_LINKABLE'. So, we could either replace those tests to use 'about:license' instead of 'about:mozilla' or we could make about:mozilla 'linkable'.

I saw that you made 'about:license' linkable (Bug 1273936), so I was wondering if you happen to know if there is a particular reason why 'about:mozilla' is not linkable or if I can just file a bug to update that?

Comment 13

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #12)
> Gijs, when using asyncOpen2() a few tests, like e.g.
> docshell/test/chrome/test_bug789773.xul break. Reason is that before
> converting docshell to rely on asyncOpen2() we haven't called
> CheckLoadURIWIthPrincipal.
> 
> For test_bug789773.xul in particular we call CheckLoadURIWithPrincipal using
> > principal: chrome://mochitests/content/chrome/docshell/test/chrome/test_bug789773.xul
> > uri: about:mozilla
> 
> which then blocks the load because about:mozilla does not use
> 'nsIAboutModule::MAKE_LINKABLE'. So, we could either replace those tests to
> use 'about:license' instead of 'about:mozilla' or we could make
> about:mozilla 'linkable'.

This doesn't make sense. chrome:// mochitests should be privileged and should be able to link to about:mozilla . They're not web content. Is the JS here not privileged for some reason?

I would prefer not to make about:mozilla linkable because every linkable about: page is basically attack surface. In principle there's no particular reason why the web needs to be able to link to *any* about: pages.

Do we know why the test is using about:mozilla as a test page? Is it just because relying on some other page is easier than having the test supply a helper file to use, specific to the test?

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #13)
> This doesn't make sense. chrome:// mochitests should be privileged and
> should be able to link to about:mozilla . They're not web content. Is the JS
> here not privileged for some reason?

Privileged in what way? The test calls window.open("about:mozilla",...) where we then call CheckLoadURIWithPrincipal before opening the window, that seems correct to me.
 
> I would prefer not to make about:mozilla linkable because every linkable
> about: page is basically attack surface. In principle there's no particular
> reason why the web needs to be able to link to *any* about: pages.

Right, I agree. So converting the tests to use about:license is probably the better choice.

> Do we know why the test is using about:mozilla as a test page? Is it just
> because relying on some other page is easier than having the test supply a
> helper file to use, specific to the test?

I don't know why those tests (in particular this test) is relying on about:mozilla.

Comment 15

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > This doesn't make sense. chrome:// mochitests should be privileged and
> > should be able to link to about:mozilla . They're not web content. Is the JS
> > here not privileged for some reason?
> 
> Privileged in what way? The test calls window.open("about:mozilla",...)
> where we then call CheckLoadURIWithPrincipal before opening the window, that
> seems correct to me.

The JS code that's calling window.open is privileged (like, say, firefox browser code, and unlike code from www.example.com). It should have the system principal. That should bypass the "can I link to this" check. Doesn't it?

> > I would prefer not to make about:mozilla linkable because every linkable
> > about: page is basically attack surface. In principle there's no particular
> > reason why the web needs to be able to link to *any* about: pages.
> 
> Right, I agree. So converting the tests to use about:license is probably the
> better choice.

Well no, if it really doesn't matter what the tests is opening, the test should provide its own support-file accessed over http://.

> > Do we know why the test is using about:mozilla as a test page? Is it just
> > because relying on some other page is easier than having the test supply a
> > helper file to use, specific to the test?
> 
> I don't know why those tests (in particular this test) is relying on
> about:mozilla.

If in doubt, ask the person who wrote it (seems to be bholley).

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Bobby, please have a look at comment 12 before continuing to read this comment.

Initially [1] you started out using "about:home" which then got updated to "about:mozilla". For the test, does it matter to use an about: page? If so, could we update to use "about:license" which uses nsIAboutModule::MAKE_LINKABLE, or should we rather update the test to provide it's own support file as Gijs suggests within comment 15?
Please note that there are more tests that use "about:mozilla" which now breaks as we convert docshell to rely on asyncOpen2(). My suggestion is to update all those tests to use "about:license" instead, but I would like to hear your opinion as well.

[1] https://hg.mozilla.org/mozilla-central/diff/1612f075c42d/docshell/test/chrome/test_bug789773.xul#l1.62
[2] https://hg.mozilla.org/mozilla-central/rev/c6a9dce2248a#l1.30

Comment 17

[Bobby Holley (:bholley)]

I think about:license should be ok. I didn't fully page in the situation described in bug 789773 comment 50, but it looks like the original testcase was loading mozilla.org, so it seems unlikely that the precise URL matters that much.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I think we still have this backwards.  Why do we have a non-system principal for a thing loaded from chrome://mochitests/content/chrome/docshell/test/chrome/test_bug789773.xul ?  That part seems pretty busted to me...  Until we figure that out, I don't think we should worry about what exact URL is passed to window.open() here.

Comment 19

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #18)
> I think we still have this backwards.  Why do we have a non-system principal
> for a thing loaded from
> chrome://mochitests/content/chrome/docshell/test/chrome/test_bug789773.xul ?
> That part seems pretty busted to me...  Until we figure that out, I don't
> think we should worry about what exact URL is passed to window.open() here.

As usual Boris gets to the crux of it better than I can - but this is why I said this whole thing doesn't make sense in comment #13. The test shouldn't be subject to content restrictions on loading about:whatever from chrome://whatever. If we have to update a bunch of tests to load other things, what are the odds the patch is actually breaking something we, uh, shouldn't be breaking? I mean, that or the tests are "weird" somehow, but we should *know* which of those two it is.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> I think we still have this backwards.  Why do we have a non-system principal
> for a thing loaded from
> chrome://mochitests/content/chrome/docshell/test/chrome/test_bug789773.xul ?

Here are the details when running test_bug789773.xul and loading "about:mozilla" the triggeringPrincipal is still the SystemPrincipal at this point [1] and mItemType != typeChrome (1 != 0), hence we enter the if-branch and do |triggeringPrincipal = nullptr|.

Later within [2] we care a principal from the referrer. Thats how we end up with a principal of chrome://mochitests/content/chrome/docshell/test/chrome/test_bug789773.xul within CheckLoadURIWithPrincipal.

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK, thanks. So the real issue is that we're opening a non-chrome window, right?  Is that what the test really wants to do, or should it be opening a chrome window instead?

But in general, the docshell code is broken there if it's really nulling out the triggering principal, it seems to me.  The intent of this code was to prevent inheritance of the system principal, not to cause CheckLoadURI checks downstream to fail.  So the real fix should be to propagate the correct triggering principal through to where we create the loadInfo but change which principal we use for inheritance to be something that we munge to not equal the triggering principal on this codepath.  

Or at least I think that should be the right fix.  I don't have a good feel for what things examine the triggeringPrincipal of the loadinfo and what they really expect in the various cases this code is trying to handle...

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #21)
> OK, thanks. So the real issue is that we're opening a non-chrome window,
> right?  Is that what the test really wants to do, or should it be opening a
> chrome window instead?

That is a question for Bobby. Bobby, what do we want to be tested here?
https://dxr.mozilla.org/mozilla-central/source/docshell/test/chrome/test_bug789773.xul#58


> But in general, the docshell code is broken there if it's really nulling out
> the triggering principal, it seems to me.  The intent of this code was to
> prevent inheritance of the system principal, not to cause CheckLoadURI
> checks downstream to fail.  So the real fix should be to propagate the
> correct triggering principal through to where we create the loadInfo but
> change which principal we use for inheritance to be something that we munge
> to not equal the triggering principal on this codepath.  

That sounds reasonable to me, I'll take a closer look tomorrow.

Comment 23

[Bobby Holley (:bholley)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> (In reply to Boris Zbarsky [:bz] from comment #21)
> > OK, thanks. So the real issue is that we're opening a non-chrome window,
> > right?  Is that what the test really wants to do, or should it be opening a
> > chrome window instead?
> 
> That is a question for Bobby. Bobby, what do we want to be tested here?
> https://dxr.mozilla.org/mozilla-central/source/docshell/test/chrome/
> test_bug789773.xul#58

I can't tell without paging in the details of bug 789773 comment 50, which I'd rather not do. Absent someone doing that, I would rather change the testcase behavior as little as possible.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_asyncopen2.patch

Attaching a WIP patch of what I got so far:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe35632798d8


Changes from Bug 1297338 seem crucial to get fixed before we can move forward with this bug.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Now that Bug 1297338 landed we should also see some improvements within this Bug:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b31fc9ae9634

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Jonathan, Benjamin, a long time ago you introduced a testcase for <embed src="..." newstream="true" ...> [1]. It seems newstream="true" is not used anywhere else in the codebase [2]. To be honest, I am not entirely sure what's it used for exactly - I tried to find documentation about newstream="true" [3,4], but I couldn't find any information that would help me move forward here.

It seems test_pluginstream_newstream.html is the last failing testcase before we can convert docshell to rely on AsyncOpen2() [see comment 25 for try run]. Question: Is that testcase still relevant, or is newstream="true" an artifact of some legacy support?

The reason the test is failing is because of a CheckLoadURIWithPrincipal security check [5] - see detailed values of important arguments:

> doContentSecurityCheck {
>  channelURI: file:///tmp/testframe
>  loadingPrincipal: http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_pluginstream_newstream.html
>  triggeringPrincipal: http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_pluginstream_newstream.html
>  principalToInherit: http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_pluginstream_newstream.html
>  contentPolicyType: 29
>  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
>  initalSecurityChecksDone: no
>  enforceSecurity: no
> }

In other words a triggeringPrincipal of "http://mochi.test:8888" tries to access a 'file:' which is not allowed. It seems we haven't performed a CheckLoadURIWithPrincipal for this codepath before converting docshell to asyncOpen2().

Any suggestions on how to move forward?

[1] http://hg.mozilla.org/mozilla-central/rev/d1328e8b7e4d#l3.30
[2] https://dxr.mozilla.org/mozilla-central/search?q=newstream%3D%22true%22&redirect=false
[3] http://www.w3schools.com/tags/tag_embed.asp
[4] https://developer.mozilla.org/en/docs/Web/HTML/Element/embed
[5] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#104

Comment 27

[Benjamin Smedberg]

"newstream" is passed on to the test plugin, and so this is a test of the NPAPI functioning correctly. See this chain:

http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/dom/plugins/test/testplugin/nptest.cpp#971
http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/dom/plugins/test/testplugin/nptest.cpp#481
which calls NPN_NewStream which ends up here:
http://searchfox.org/mozilla-central/rev/767e1e9b118269f957ca1817138472146278a29e/dom/plugins/base/nsNPAPIPlugin.cpp#831

So it seems that your changes break the NPAPI function NPN_NewStream.

https://developer.mozilla.org/en-US/Add-ons/Plugins/Reference/NPN_NewStream

I don't really understand what "doContentSecurityCheck" does, so I'm not clear on what exactly you're testing. I also don't know for certain what security context the new stream page should have: I'd imagine that either it should be same-origin with the plugin or a unique null principal, but it's not well-documented.

And we only have to live with this for another release: I'm pretty sure Flash never uses this API and so we can rip it out in 53.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #27)
> And we only have to live with this for another release: I'm pretty sure
> Flash never uses this API and so we can rip it out in 53.

Thanks Benjamin for the speedy response, but what does that mean exactly? Does that mean I can remove the test within this bug? Or are you suggesting I should wait with landing this bug to FF53, which I don't really want of course :-)

Comment 29

[Benjamin Smedberg]

I think the test is correct and you broke NPN_NewStream and either need to fix it or wait.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Boris, running test_pluginstream_newstream.html fails because a codebasePrincipal of "http://mochi.test:8888" tries to access a "file:///" URI (see important arguments and stacktrace underneath). It seems the problem initiates within nsDocShell::OnLinkClickSync() [1]. I wonder if Bug 1297338 should have updated the triggeringPrincipal. I don't fully understand what OnLinkClickSync() does, but potentially the triggeringPrincipal should have been the SystemPrincipal and the principalToInherit should have been 'aContent->NodePrincipal()'?

If so, I would file a follow up for Bug 1297338 to get that fixed - thanks!

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

%---snip---

calling NPN_NewStream...File URL = file:///tmp/testframe
return value 0
1024 bytes written, total 1024
doContentSecurityCheck {
  channelURI: file:///tmp/testframe
  loadingPrincipal: http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_pluginstream_newstream.html
  triggeringPrincipal: http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_pluginstream_newstream.html
  principalToInherit: http://mochi.test:8888/tests/dom/plugins/test/mochitest/test_pluginstream_newstream.html
  contentPolicyType: 29
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
  initalSecurityChecksDone: no
  enforceSecurity: no
}
Assertion failure: false, at /home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:552
#01: nsContentSecurityManager::doContentSecurityCheck(nsIChannel*, nsCOMPtr<nsIStreamListener>&) (/home/ckerschb/moz/mc/dom/security/nsContentSecurityManager.cpp:552)
#02: nsBaseChannel::AsyncOpen2(nsIStreamListener*) (/home/ckerschb/moz/mc/netwerk/base/nsBaseChannel.cpp:702)
#03: nsURILoader::OpenURI(nsIChannel*, unsigned int, nsIInterfaceRequestor*) (/home/ckerschb/moz/mc/uriloader/base/nsURILoader.cpp:819)
#04: nsDocShell::DoChannelLoad(nsIChannel*, nsIURILoader*, bool) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:11315)
#05: nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, nsIURI*, bool, unsigned int, nsIPrincipal*, nsIPrincipal*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, nsAString_internal const&, nsIURI*, unsigned int) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:11129)
#06: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:10587)
#07: nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, char16_t const*, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, nsIDocShell**, nsIRequest**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:9994)
#08: nsDocShell::OnLinkClickSync(nsIContent*, nsIURI*, char16_t const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, nsIDocShell**, nsIRequest**) (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:13981)
#09: OnLinkClickEvent::Run() (/home/ckerschb/moz/mc/docshell/base/nsDocShell.cpp:13749)
#10: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:1082 (discriminator 1))

Comment 31

[Boris Zbarsky [:bzbarsky]]

> I don't fully understand what OnLinkClickSync() does

Handles a link click.

> but potentially the triggeringPrincipal should have been the SystemPrincipal

Well, that would have matched our old behavior, kinda, for purposes of CheckLoadURI checks.  But it would have been a bald-faced lie for other purposes (e.g. content policy).

I assume the caller of OnLinkClick() (which is what posts the runnable that later runs OnLinkClickSync()) in this case is nsPluginInstanceOwner::GetURL in this case, right?  And presumably it's coming through with aDoCheckLoadURIChecks == false, because otherwise the existing CheckLoadURIWithPrincipal check there would have forbidden the load.  

What is the caller of GetURL?  Is it nsPluginStreamToFile::Write or nsPluginStreamToFile::Close?

And more to the point, why should this file:// access be allowed at all?  That's a question for Benjamin, probably...  It looks like this came up before, in bug 1182543 comment 16, and was kind of hacked around back then with this aDoCheckLoadURIChecks thing.

Is the idea that we streamed this data from somewhere the plugin _is_ allowed to get it from, saved it to disk, and now want to navigate to that file?  If so, we should explicitly be setting the triggeringPrincipal for this navigation to system, I agree.  But that shouldn't apply to _all_ link navigations.  Most simply, maybe OnLinkClick/OnLinkClickSync need a "triggering principal override" or something.  That's assuming we want to keep using OnLinkClick in nsPluginInstanceOwner::GetURL, and keep using GetURL in nsPluginStreamToFile.  It seems a bit weird to me to pretend like the load is coming from the plugin loading content in this situation, but maybe I don't really understand this newstream API...

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #31)
> And more to the point, why should this file:// access be allowed at all? 
> That's a question for Benjamin, probably...  It looks like this came up
> before, in bug 1182543 comment 16, and was kind of hacked around back then
> with this aDoCheckLoadURIChecks thing.

Benjamin, if you get a chance, could you answers some of Boris' questions from Comment 31? Thanks for your help!

Comment 33

[Benjamin Smedberg]

Short answser: no I can't answer these questions.

Long answer: The documented API surface is that the plugin streams us some data and we display it to the user. The fact that this is a file stream is at best an implementation detail. I don't know, and I don't know if anyone knows, what the security properties/origin of the displayed page are actually supposed to be. I don't know if any plugin actually use this API, or what they might use it for.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #31)
> And more to the point, why should this file:// access be allowed at all? 
> That's a question for Benjamin, probably...  It looks like this came up
> before, in bug 1182543 comment 16, and was kind of hacked around back then
> with this aDoCheckLoadURIChecks thing.

Yes, you are right Boris, aDoCheckLoadURIChecks flag was used to skip security checks. Since Benjamin mentioned the NPN_NewStream API is going to be ripped out in FF53 (See comment 27) I think we can just use that flag and use the SystemPrincipal as the triggeringPrincipal in that case. Once the API got ripped out we can revert that change. Basically, we use that only as a temporary hack so we can convert docshell to asyncOpen2() without waiting for the removal of the API. I went ahead and implemented that which now causes test_pluginstream_newstream.html to pass again.

Finally we should have a green TRY run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=369eee49f853aa12ec380443544c9d0994bd1f3c

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part1.patch

Hi Smaug, thanks for helping me getting those changesets reviewed. Overall goal is to change docshell loads to rely on AscyncOpen2() :-)

A few notes that might be helpful:
*) Instead of creating a new LoadInfo within nsJSProtocolHandler, I think it's preferable to pass aLoadInfo from nsJSProtocolHandler::NewChannel2 along to the init() function of the actual new channel.
*) AsyncOpen2() performs the removed ContentPolicy check from nsDocshell, but there is one important difference. Currently we eventually return 'NS_ERROR_CONTENT_BLOCKED_SHOW_ALT' from Internalload(). The new world, where we perform contentpolicy checks within ASyncopen2() we can't propagate that information back up so InternalLoad() could return NS_ERROR_CONTENT_BLOCKED_SHOW_ALT. Honestly I don't think there is an easy way to accomplish that. I am most certainly sure we wanna keep that behavior, right? Any suggestions on how we could accomplish that?

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch

For toplevel loads we have to use the triggeringPrincipal as the principal we pass to content policies. Please note that the triggeringPrincipal is the principal we currently pass to contentPolicies within docshell. Further we have to potentially fix up the URI for security checks, like in the case of wyciwcyg URIs or potentially we have to query the nested URI in case of view-source.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

As mentioned in comment 34, the NPN_NewStream() API is going to be removed in FF53, unti then I suppose we can remove the workaround of skipping security checks like proposed in the patch. Please note that currently we do not perform security checks if 'aDoCheckLoadURIChecks' is false. Within the patch I just propagate that flag all do way to the place where we call InternalLoad and based on that flag we perform a load using the SystemPrincipal to bypass security checks.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part4_test_updates.patch

@Billm: Can you have a look at dom/base/test/file_simplecontentpolicy.js. I assume that assertion never worked completely correct right? Most likely because docshell code returned earlier. Within test_simplecontentpolicy.html I see |["DOCUMENT", true]| where the second argument in the array denotes whether it's a toplevel load or not.

@Smaug: A while ago we discussed tests within dom/browser-element/mochitest/priority and why we need to update to a full blown URL, because otherwise they would load with a chrome URL (if I remember correctly) which then would be forbidden because the toplevel URL is http. And within test_bug813906 the onload() handler is not called again if the frame was blocked. I assume that should never have been the case in the first place.

Comment 39

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

Comment on attachment 8803063 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part1.patch


>   // build up a channel for this stream.
>   nsCOMPtr<nsIChannel> channel;
>   nsresult rv = NS_NewInputStreamChannel(getter_AddRefs(channel),
>                                          uri,
>                                          aStream,
>                                          triggeringPrincipal,
>-                                         nsILoadInfo::SEC_NORMAL,
>+                                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
Why this change?


>-    // XXXbz would be nice to know the loading principal here... but we don't
>-    nsCOMPtr<nsIPrincipal> requestingPrincipal = aTriggeringPrincipal;
>-    if (!requestingPrincipal && aReferrer) {
>-      rv =
>-        CreatePrincipalFromReferrer(aReferrer, getter_AddRefs(requestingPrincipal));
>-      NS_ENSURE_SUCCESS(rv, rv);
>-    }
>-
>-    int16_t shouldLoad = nsIContentPolicy::ACCEPT;
>-    rv = NS_CheckContentLoadPolicy(contentType,
>-                                   aURI,
>-                                   requestingPrincipal,
>-                                   requestingContext,
>-                                   EmptyCString(),  // mime guess
>-                                   nullptr,  // extra
>-                                   &shouldLoad);
>-
>-    if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {
>-      if (NS_SUCCEEDED(rv) && shouldLoad == nsIContentPolicy::REJECT_TYPE) {
>-        return NS_ERROR_CONTENT_BLOCKED_SHOW_ALT;
>-      }
I can't actually find any use for this, but it does actually look like a useful error message.

InternalLoad calls DoURILoad which calls NS_NewChannelInternal and such. Why can't you propagate NS_ERROR_CONTENT_BLOCKED_SHOW_ALT from
DoURILoad? Necko should propagate some useful error message if content policy was rejected. If not, that is a bug to fix.

>   nsLoadFlags loadFlags = mDefaultLoadFlags;
>-  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;
>+  nsSecurityFlags securityFlags =
>+    nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;
So, why this?

Comment 40

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

Comment on attachment 8803064 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch


>-  MOZ_ASSERT(skipContentTypeCheck ||
>-             loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT,
>-             "calling NS_HasBeenCrossOrigin on a top level load");
>+  if (loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT) 
>+    // top level load can not be cross site
>+    return false;
>+  }
Why to get rid of the MOZ_ASSERT, even though the comment still says the assertion should be ok.
The assertion put back (and keep also the 'if'), r+

Or explain why we break the variant. But if we do, the comment is wrong.
hmm.., so r- after all.

Comment 41

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

Comment on attachment 8803066 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

>+  // if we need to skip security checks, we pass the systemPrincipal
>+  // as the triggeringPrincipal, else we us the node's principal.
>+  nsCOMPtr<nsIPrincipal> triggeringPrincipal =
>+    aDoCheckLoadURIChecks ? aContent->NodePrincipal()
>+                          : nsContentUtils::GetSystemPrincipal();i
I'm really not happy to pass system principal to InternalLoad. So easy to get things wrong in InternalLoad, and any code it uses.

I'd rather pass the triggering principal (let it be system principal in some plugin case) to OnLinkClick and if that is not passed, then
use aContent->NodePrincipal(). That would limit this all weirdness to plugins only. 
Would still need to verify it doesn't allow more evil stuff than expected.

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #39)
> >-                                         nsILoadInfo::SEC_NORMAL,
> >+                                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
> Why this change?
...
> >-  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;
> >+  nsSecurityFlags securityFlags =
> >+    nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;
> So, why this?

Hey Olli, is the question in both cases why using exactly SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL or why removing SEC_NORMAL?
If the question is why removing SEC_NORMAL, then the answer is because SEC_NORMAL was introduced as a placeholder initially when we started to attach loadInfos to all channels. In this second pass where we are going over all callsites again to actually open all channels using asyncOpen2(), we have to pass one of the five available securityflags within loadInfo [1].

If the question is, why we are using SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, then my answer is because data: loads should be allowed and the resulting resource should get a freshly created NullPrincipal.

Does that answer your question?

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#50

Comment 43

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

(In reply to Christoph Kerschbaumer [:ckerschb (@CCS)] from comment #42)

> If the question is, why we are using SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
> then my answer is because data: loads should be allowed and the resulting
> resource should get a freshly created NullPrincipal.
doesn't that change the current behavior?

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #43)
> > If the question is, why we are using SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
> > then my answer is because data: loads should be allowed and the resulting
> > resource should get a freshly created NullPrincipal.
> doesn't that change the current behavior?

I wouldn't think so, but since you are asking I suppose I could be missing something? Can you be more explicit about the problem you are seeing?

Comment 45

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

I'm not seeing anything, I'm asking you whether the behavior changes. Do we end up creating null principal more often comparing to the current setup?

Comment 46

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8803070 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part4_test_updates.patch

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

::: dom/base/test/file_simplecontentpolicy.js
@@ +38,5 @@
>                         extra)
>    {
>      // Remember last content type seen for the test url
>      if (contentLocation.spec.endsWith(urlSuffix)) {
> +      assert.ok(frame === browserElement || isTopLevel, "correct <browser> element");

Please just remove this line. It's not testing anything useful.

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part1.patch

Smaug, I argue that SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is the security flag to use which persists current behavior. I further updated patch2 to (potentially) return NS_ERROR_CONTENT_BLOCKED_SHOW_ALT.

Comment 48

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch

I think we can't keep the assertion within NS_HasBeenCrossOrigin. In the old world we did not use asyncOpen2() for docshell loads, hence NS_HasBeenCrossOrigin has never been called for toplevel loads. Toplevel loads can not be cross-origin, right? Hence I updated the code to return false for TYPE_DOCUMENT. Agreed?

Further I updated the contentPolicy check return type within nsContentSecurityManager. In the old world, contetPolicy checks were performed within ::InternalLoad(). In the new world the chain looks like this: InternalLoad() -> DoURILoad -> DoChannelLoad() -> aURILoader->OpenURI() -> asyncOpen2(). So simply returning the new error propagates NS_ERROR_CONTENT_BLOCKED_SHOW_ALT back up to InternalLoad().

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

I agree that we should rather use aTriggeringPrincipal as an argument which can be passed explicitly in case some callsites want to use a custom triggeringPrincipal.

Comment 50

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part4_test_updates.patch

Carrying over r+ from smaug and billm.

Comment 51

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

(In reply to Christoph Kerschbaumer [:ckerschb (limited availability)] from comment #48)
>Toplevel
> loads can not be cross-origin, right? 
cross-origin between which origins? Whoever is triggering the load and new load of course can be cross-origin.

Comment 52

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

Comment on attachment 8806091 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part1.patch

> 
>   nsLoadFlags loadFlags = mDefaultLoadFlags;
>-  nsSecurityFlags securityFlags = nsILoadInfo::SEC_NORMAL;
>+  nsSecurityFlags securityFlags =
>+    nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL;

Ahaa, the important part of the code is right after the lines visible in the patch.
securityFlags |= nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
And we have contradicting documentation about SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL vs SEC_FORCE_INHERIT_PRINCIPAL


>-nsresult nsJSChannel::Init(nsIURI *aURI)
>+nsresult nsJSChannel::Init(nsIURI *aURI, nsILoadInfo *aLoadInfo)
nit, * goes with the type. Want to fix also nsIURI*

Comment 53

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

Comment on attachment 8806091 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part1.patch

er, not sure yet.

I need to understand how NS_HasBeenCrossOrigin deals with subdocuments.

Comment 54

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

Comment on attachment 8806092 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch

So I've tried and failed to understand why 
nsIContentPolicy::TYPE_DOCUMENT is special cased in NS_HasBeenCrossOrigin but nsIContentPolicy::TYPE_SUBDOCUMENT isn't.
And I don't understand the comment there either "top level load can not be cross site". We explicitly disable the cross origin check.

Did you test iframe doing redirects`to cross-domain sites?

Comment 55

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

Comment on attachment 8806093 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

Er, wait. What happens if data url is used there with plugin stuff, or about:blank.

Comment 56

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

Comment on attachment 8806093 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

Ah, it is used as triggering principal, not as principal to inherit.

But why do we need this? which security check does this prevent, where?

Comment 57

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

Comment on attachment 8806093 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

This will pass the NS_CheckContentLoadPolicy check we currently have in nsDocShell::InternalLoad. Why is that ok?

Comment 58

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

(In reply to Olli Pettay [:smaug] from comment #56)
> But why do we need this? which security check does this prevent, where?

Currently we bypass CheckLoadURIWithPrincipal within nsPluginInstanceOwner.cpp if !aDoCheckLoadURIChecks. We should keep the same behavior and bypass CheckLoadURIWithPrincipal within ASyncOpen2. (Test test_pluginstream_newstream.html would be failing otherwise). 

> This will pass the NS_CheckContentLoadPolicy check we currently have in
> nsDocShell::InternalLoad. Why is that ok?

Initially I wanted to use the SystemPrincipal to bypass CheckLoadURIWithPrincipal but I agree that this would also skip ContentPolicy checks, which should still be called. In this new version I am creating a codebasePrincipal which will also succeed CheckLoadURIWithPrincipal but at the same time does not bypass contentpolicy checks. I agree it's a little hacky, but given comment 27 from bsmedberg. The functionality is going to be ripped out of Firefox in version 53 anyway.

Comment 59

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch

(In reply to Olli Pettay [:smaug] from comment #54)
> So I've tried and failed to understand why 
> nsIContentPolicy::TYPE_DOCUMENT is special cased in NS_HasBeenCrossOrigin
> but nsIContentPolicy::TYPE_SUBDOCUMENT isn't.
> And I don't understand the comment there either "top level load can not be
> cross site". We explicitly disable the cross origin check.
> 
> Did you test iframe doing redirects`to cross-domain sites?

I expanded the comment on top to explain in more detail. In short, top level loads do not contain a loadingPrincipal (it's null), hence we can not perform a cross-origin check for top level loads. In more detail, we can't perform a loadingPrincipal->CheckMayLoad(uri, ...) security check for top level loads, hence the comment why such loads can't be cross origin.

Comment 60

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

The NS_HasBeenCrossOrigin function is defined to return true if the channel is loading a URI which is not-same-origin with the loadingPrincipal of the channel.

Since top-level loads don't have a loadingPrincipal [1], that means that NS_HasBeenCrossOrigin for top-level loads don't really make sense.

I don't have an opinion about if that means we should make NS_HasBeenCrossOrigin assert for top-level loads, always return true, or always return false.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#196

Comment 61

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

Comment on attachment 8808606 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

>+  // if security checks (in particular CheckLoadURIWithPrincipal) needs
>+  // to be skipped we are creating a codebasePrincipal to make sure
>+  // that security check succeeds. Please note that we do not want to
>+  // fall back to using the systemPrincipal, because that would also
>+  // bypass ContentPolicy checks which should still be enforced.
>+  nsCOMPtr<nsIPrincipal> triggeringPrincipal;
>+  if (!aDoCheckLoadURIChecks) {
>+    mozilla::PrincipalOriginAttributes attrs;
>+    triggeringPrincipal = BasePrincipal::CreateCodebasePrincipal(uri, attrs);
in order to create more expected principal, shouldn't you get OA from somewhere, like from content->NodePrincipal() and not pass just empty OA.

Comment 62

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

Comment on attachment 8808608 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch


>-
>-  MOZ_ASSERT(skipContentTypeCheck ||
>-             loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT,
>-             "calling NS_HasBeenCrossOrigin on a top level load");
>+  if (loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT) {
>+    // Top level loads can not be cross origin since there is no origin to compare.
>+    // We can not call CheckMayLoad since the LoadingPrincipal for top level loads
>+    // is a nullptr within the loadInfo, hence we can bail early in that case.
>+    return false;
>+  }
But why do we want to check anything for SUBDOCUMENTs? That is unclear to me.
Please explain and improve the comment, or fix.

Comment 63

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

Smaug, as discussed over IRC, we should use the Origin attributes of content->NodePrincipal() for creating the triggeringPrincipal within nsPluginInstanceOwner.cpp

Comment 64

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch

So, agreed, we should also bail out of NS_HasBeenCrossOrigin() for TYPE_SUBDOCUMENT loads. Further, I agree, that function should move into the contentsecurityManager since it's only called from within contentSecuriytManager, but I leave all that work for a follow up bug. Within this bug I really only want to focus on converting docshell to use asyncOpen2().

Comment 65

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=980e65e8ea0137703e3040f1b847554bfb974688

Comment 66

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

Comment on attachment 8809811 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part3_fix_pluginstream.patch

>   int32_t blockPopups =
>     Preferences::GetInt("privacy.popups.disable_from_plugins");
>   nsAutoPopupStatePusher popupStatePusher((PopupControlState)blockPopups);
> 
>+
>+  // if security checks (in particular CheckLoadURIWithPrincipal) needs
>+  // to be skipped we are creating a codebasePrincipal to make sure
>+  // that security check succeeds. Please note that we do not want to
>+  // fall back to using the systemPrincipal, because that would also
>+  // bypass ContentPolicy checks which should still be enforced.
>+  nsCOMPtr<nsIPrincipal> triggeringPrincipal;
>+  if (!aDoCheckLoadURIChecks) {
>+    nsCOMPtr<nsIContent> content = do_QueryReferent(mContent);
you should have content already on stack. just use that variable.

That fixed, r+

Comment 67

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

Comment on attachment 8809812 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part2_sec_checks.patch

> NS_HasBeenCrossOrigin(nsIChannel* aChannel, bool aReport)
> {
>   nsCOMPtr<nsILoadInfo> loadInfo = aChannel->GetLoadInfo();
>   MOZ_RELEASE_ASSERT(loadInfo, "Origin tracking only works for channels created with a loadinfo");
> 
>-#ifdef DEBUG
>-  // Don't enforce TYPE_DOCUMENT assertions for loads
>-  // initiated by javascript tests.
>-  bool skipContentTypeCheck = false;
>-  skipContentTypeCheck = Preferences::GetBool("network.loadinfo.skip_type_assertion");
>-#endif
>-
>-  MOZ_ASSERT(skipContentTypeCheck ||
>-             loadInfo->GetExternalContentPolicyType() != nsIContentPolicy::TYPE_DOCUMENT,
>-             "calling NS_HasBeenCrossOrigin on a top level load");
>+  if (loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_DOCUMENT ||
>+      loadInfo->GetExternalContentPolicyType() == nsIContentPolicy::TYPE_SUBDOCUMENT) {
>+    // Document loads can not be cross origin.
I don't understand this comment. Document loads very much can be cross origin.

I think either the contentpolicytype check needs to go to the callers, or the method name should be changed.
It is very confusing if a method called NS_HasBeenCrossOrigin returns different results based on the contentpolicytype.
I could live with HasBeenCrossOriginNonDocumentChannel. It is long, but this method is called rarely. With that r+

Comment 68

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

Don't make special exceptions in NS_HasBeenCrossSite for TYPE_SUBDOCUMENT. That would be very surprising behavior which is not clear from the function name.

Change the caller if really needed.

Ideally NS_HasBeenCrossSite shouldn't check for TYPE_DOCUMENT either, but rather just check if the loadingPrincipal is null and if so return false. But I'm not sure if we're setting the right loadingPrincipal for TYPE_DOCUMENT loads yet?

Comment 69

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #68)
> Ideally NS_HasBeenCrossSite shouldn't check for TYPE_DOCUMENT either, but
> rather just check if the loadingPrincipal is null and if so return false.

Bailing out of NS_HasBeenCrossSite if the loadingPrincipal is null sounds like the best way forward to me. 

> But I'm not sure if we're setting the right loadingPrincipal for
> TYPE_DOCUMENT loads yet?

For TYPE_DOCUMENT loads the loadingPrincipal is always null.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=6af11348d325d9b199826e0b983d1f885ce26597

Comment 70

[Christoph Kerschbaumer [:ckerschb]]

Shane, Kris, within this bug we are about to convert docshell to rely on asyncOpen2(). Evaluating the latest TRY run (see comment 69) reveals that test browser_ext_webRequest.js is failing. I tried to examine the problem but I can't really figure out what's going on. The test was added within Bug 1318800 together with an update within WebRequest.jsm [1]. Even though I don't know exactly how that all works together I am pretty sure that's where the problem occurs. I was wondering if you could explain what data.isSystemPrincipal is used for and when and why it's needed.

Thanks for your help!

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#627

Comment 71

[Shane Caraveo (:mixedpuppy)]

It is used to prevent system requests from reaching webextensions, every request that goes through webrequest gets checked.  The failure is that the background frame that has system principle is being allowed through.  

Your patch changes the [behavior of] principals in channel.loadinfo, which is fine, but someone will then need to explain how we detect system principal loads, and likely some idl documentation will need updating to explain the correct behavior.

Comment 72

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #70)
> Shane, Kris, within this bug we are about to convert docshell to rely on
> asyncOpen2(). Evaluating the latest TRY run (see comment 69) reveals that
> test browser_ext_webRequest.js is failing. I tried to examine the problem
> but I can't really figure out what's going on. The test was added within Bug
> 1318800 together with an update within WebRequest.jsm [1]. Even though I
> don't know exactly how that all works together I am pretty sure that's where
> the problem occurs. I was wondering if you could explain what
> data.isSystemPrincipal is used for and when and why it's needed.

This is actually happening in the content policy code:

http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/toolkit/modules/addons/WebRequestContent.js#83-84

It looks like this request is coming from HiddenFrame.jsm creating an iframe in the hidden window. For some reason, both the requestOrigin and requestPrincipal are resource://gre-resources/hiddenWindow.html, even though the URL eventually loads with the system principal, so it's not being filtered out.

We should probably filter out resource: request origins anyway, but it seems like something else is wrong here.

Comment 73

[Christoph Kerschbaumer [:ckerschb]]

Smaug, in the old world we used the triggeringPrincipal for contentPolicy checks of TYPE_DOCUMENT as well as TYPE_SUBDOCUMENT within nsdocshell [1]. In the new world we slightly change that behavior and only use the triggeringPrincipal for TYPE_DOCUMENT, but the loadingPrincipal for TYPE_SUBDOCUMENT loads (see DoContentSecurityChecks within [2]).

Even though I think TYPE_DOCUMENT should use the triggeringPrincipal (because the loadingPrincipal for toplevel loads is always a nullptr) and TYPE_SUDDOCUMENT loads should use the loadingPrincipal that slightly changes current behavior. In particular browser_ext_webRequest.js is failing.

@Shane, Kris: The test is failing because of
> 151 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_webRequest.js | unexpected request application/vnd.mozilla.xul+xml;charset=utf-8,<window%20id='win'/> -
The principal for that load in the new world is: resource://gre-resources/hiddenWindow.html

I looked at the test and also at WebRequestContent.js [3] and WebRequest.jsm [4] and tried to figure out what's going on, but unfortunately I don't understand how that all works together. Maybe you can help me out a little.

Within comment 71 Shane mentioned you just need a way to figure out system loads right? A triggeringPrincipal of systemprincipal should provide that information.

Question for all: How should web proceed? Should we keep current behavior and pass the same principal for TYPE_DOCUMENT and SUB_DOCUMENT loads? Should we update the test somehow or do we need yet something else to make this work again correctly?


[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9870
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8809812&action=diff#a/dom/security/nsContentSecurityManager.cpp_sec5
[3] http://searchfox.org/mozilla-central/rev/4f5f9f3222f35fa4635ea96a0c963c130854ef19/toolkit/modules/addons/WebRequestContent.js#83-84
[4] https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/WebRequest.jsm#627

Comment 74

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

Unfortunately I have no idea what a "WebRequest" is.

But, for consistency, wouldn't it make sense to use triggeringPrincipal both for TYPE_DOCUMENT and TYPE_SUBDOCUMENT.
If one needs the loading principal in iframe case, one can get access to it using aContext param of the content policy methods, right?

Comment 75

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #74)
> Unfortunately I have no idea what a "WebRequest" is.
> 
> But, for consistency, wouldn't it make sense to use triggeringPrincipal both
> for TYPE_DOCUMENT and TYPE_SUBDOCUMENT.
> If one needs the loading principal in iframe case, one can get access to it
> using aContext param of the content policy methods, right?

As discussed over IRC, let's be consistent and use the triggeringPrincipal for TYPE_DOCUMENT and TYPE_SUBDOCUMENT for content policy checks.

Comment 76

[Christoph Kerschbaumer [:ckerschb]]

Lots of problems disappeared locally, let's see what TRY thinks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f4d18e8220c3f1cf2c0f43a52eb67a0da7ef6cd

Comment 77

[Shane Caraveo (:mixedpuppy)]

Seems like something was figured out.  BTW also look at Bug 1316256 which was a recent change related to what principal is passed through content policy.

Comment 78

[Christoph Kerschbaumer [:ckerschb]]

Kate, when working on HSTS priming, have you ever encountered those errors:
> UnicodeDecodeError: 'utf8' codec can't decode byte 0xfc in position 46: invalid start byte
> UnicodeDecodeError: 'utf8' codec can't decode byte 0xc1 in position 42: invalid start byte
> TEST-UNEXPECTED-NOTRUN | /mixed-content/blockable/no-opt-in/same-host-http/iframe-tag/top-level/no-redirect/no-opt-in-blocks.https.html | opt_in_method: no-opt-in

These tests are failing (see comment 76, e.g. Linux x64 debug W(8)). I don't know what might cause this problem and I also don't know how to proceed. James' comment [1] indicates that it might be related to HSTS priming. Any ideas?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1307840#c9

Comment 79

[Kate McKinley [:kmckinley, :Kate]]

Yes, priming sends a HEAD request which appears to be decoded incorrectly in by python's BaseHttpServer which causes it to go down the wrong path. If it encounters 'HEAD' instead of 'GET' when it parses the request, it fails and tries to print the error string in the message. Failing would be fine, but the exception caused by the strange encoding of the command string causes the UnicodeDecodeError. If you set security.mixed_content.send_hsts_priming to false for that test, it should be allow the test to pass.

I'm investigating to identify the root cause with a python dev, I'll create a new bug if it is an issue on our side. In the meanwhile, I've created bug 1325680 to re-enable HSTS priming on web-platform tests, and will post and findings there.

Comment 80

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #75)
> As discussed over IRC, let's be consistent and use the triggeringPrincipal
> for TYPE_DOCUMENT and TYPE_SUBDOCUMENT for content policy checks.

With that decision the needinfo for kris has become superfluous.

Comment 81

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part5_disable_hsts_for_wpt.patch

Kate, since this bug converts docshell loads to call asyncOpen2() loads of TYPE_SUBDOCUMENT are now subject to ::MarkLoadInfoForPriming within nsContentSecurityManager.cpp. As dicsussed within comment 79, such loads would now send a HEAD request and the test would fail, hence we have to disable HSTS priming for those wpt tests.

I suppose once bug 1325680 is resolved you can also re-enable HSTS prming for those wpt tests, right?

Comment 82

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b7c4ff92a34b5b15c5eea8365d91b8fe8bc6cb1

Comment 83

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_part5_disable_hsts_for_wpt.patch

Kate, apparently I forgot to add /swap-scheme-redirect/, those tests all look very identical but then they aren't :-(

Anyway, please also see comment 81 for the initial review request.

Comment 84

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1182569_docshell_ascyncopen2_update_cors_wpt_tests.patch

Ben, when running fetch-frame-resource.https.html I get the following error:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://www1.web-platform.test:8443/service-workers/service-worker/resources/fetch-access-control.py?ACAOrigin=https://web-platform.test:8443. (Reason: expected ‘true’ in CORS header ‘Access-Control-Allow-Credentials’).

It seems that we should use credentials for the request. I updated the test accordingly.

Comment 85

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f293a2cd88aec1e579d29ed755a3de03dad16a2d

Comment 86

[Ben Kelly [:bkelly, not reviewing]]

Christoph, can you help me understand what changed here?  Are we sending credentials now when previously we were making these requests anonymously?

Comment 87

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8823092 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_part5_disable_hsts_for_wpt.patch

+1

Yes, this will be included in bug 1325680.

Comment 88

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8823093 [details] [diff] [review]
bug_1182569_docshell_ascyncopen2_update_cors_wpt_tests.patch

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

Christoph and I talked on IRC.  It appears the credentials change is due to the fact we no longer hit our legacy compat code to generate FetchEvent.request.credentials from nsIChannel:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/InternalRequest.cpp#447

Now that we are going through AsyncOpen2() we are correctly setting credentials mode.  And it makes sense credentials is set for a navigation.  We definitely send/receive cookies on navigation.

We should consider landing bug 1272594 after this.

Comment 89

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [:bkelly] from comment #88)
> We should consider landing bug 1272594 after this.

Yep, will certainly land that soon after this bug landed.

Comment 90

[Christoph Kerschbaumer [:ckerschb]]

This should be ready to land now, let's confirm:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=39729bebb798642209e72cfd7a732bb29f4542ca

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5bac3694b6f
Use AsyncOpen2 for docshell loads. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/81f9670a6b33
Update ContentSecurityManager to handle docshell loads. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/417c71ad76ad
Skip security check for plugins using newstream attribute. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa7d7723116
Update tests when converting docshell loads to use AynscOpen2. r=smaug,billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e98992ddb62
Disable hsts priming for loads of type subdocument for wpt tests. r=mckinley
https://hg.mozilla.org/integration/mozilla-inbound/rev/d31df518cb6e
Add credentials mode for fetch-frame-resource.https.html wpt test. r=bkelly

Comment 92

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a5bac3694b6f
https://hg.mozilla.org/mozilla-central/rev/81f9670a6b33
https://hg.mozilla.org/mozilla-central/rev/417c71ad76ad
https://hg.mozilla.org/mozilla-central/rev/efa7d7723116
https://hg.mozilla.org/mozilla-central/rev/6e98992ddb62
https://hg.mozilla.org/mozilla-central/rev/d31df518cb6e