Christoph Kerschbaumer [:ckerschb] Reporter
	Description • 8 years ago

      No description provided.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 1 • 8 years ago

Here is a WIP mochitest illustrating the problem.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 2 • 8 years ago

Just rebased the test.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 3 • 8 years ago

Tim, now that we have landed Bug 1277803 I would have hoped it also fixes this bug, but it doesn't. Most likely because we don't serialize CSP between child and parent. What's surprising is though is that I don't get a call to the ContentSecurityManager which is called in asyncOpen2() for all network loads. All I see is the following:

doContentSecurityCheck {
  channelURI: http://mochi.test:8888/favicon.ico
  loadingPrincipal: http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp
  triggeringPrincipal: http://mochi.test:8888/tests?autorun=1&consoleLevel=INFO&manifestFile=tests.json&dumpOutputDirectory=%2Ftmp
  contentPolicyType: 41
  securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS, SEC_ALLOW_CHROME,
  initalSecurityChecksDone: no
  enforceSecurity: no
}

But that is not the *.ico I load from within the test. I would have at least expected a call to load URI: file_favicon.ico.

Do you happen to know what the problem might be here?

	Tim Huang[:timhuang]
	Comment 4 • 8 years ago

No, I have no idea about this. I suppose that everything goes through the asyncOpen2 will call into doContentSecurityCheck(). And for Favicon loads, they should use the asyncOpen2. Could it becasue the XUL uses the system principal, so it skips this check. But the favicon request from the Places is not using the system principal, it should go through the doContentSecurityCheck().

Does this happen after Bug 1277803 landed?

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 5 • 8 years ago

(In reply to Tim Huang[:timhuang] from comment #4)
> Does this happen after Bug 1277803 landed?

Yeah, that happens after Bug 1277803 landed. I am out all week, maybe Freddy can take an initial look why contentPolicies are not consulted for the favicon load within the testcase.

Freddy, if you are too busy, that's fine too, I'll have a look once I am back from CCS.

	Frederik Braun [:freddy]
	Comment 6 • 8 years ago

I tried looking into this, but I am not sure how you are debugging this (e.g., looking at your output comment 3).
Care to elaborate?

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 7 • 8 years ago

(In reply to Frederik Braun [:freddyb] from comment #6)
> I tried looking into this, but I am not sure how you are debugging this
> (e.g., looking at your output comment 3).
> Care to elaborate?

Hey Freddy, by now we should be using asyncOpen2() for all subresource loads, which also includes any kind of image loads. So I would have thought that favicion loads also call ContentSecurityManager::DoContentSecurityChecks() for favicons, but apparently that's not happening. We need to find out why.

	Frederik Braun [:freddy]
	Comment 8 • 8 years ago

I could reproduce with the test and can attach a stack trace.
The load comes from nsImageBoxFrame, which is using nsContentUtils::LoadImage and then imgLoader::LoadImage.

nsImageBoxFrame _tries_ to get a loadingPrincipal from an element but falls back to the SystemPrincipal if it can not. Investigating.

	Dave Townsend [:mossop] Assignee
	Comment 11 • 6 years ago

I think I can see how to fix this.

	Dave Townsend [:mossop] Assignee
	Comment 12 • 6 years ago

Comment on attachment 8802902 [details] [diff] [review]
favicon_csp_mochitest.patch

This doesn't work anymore. In particular it uses a page in an inner frame with a favicon, but we only load favicons for top-level documents.

	Dave Townsend [:mossop] Assignee
	Comment 19 • 6 years ago

I've ended up fixing this bug with the patch in bug 1453751. Going to leave these patches here since the first two might still be useful, if you want to be able to use the CSP in the main process.

But I have a question about how CSP applies to favicons. The spec makes it clear that <link> tags that load icons are governed by img-src. But when Firefox doesn't see a favicon defined in a page we guess and load "/favicon.ico".

Should that guessed load be governed by the CSP?
Perhaps more generally, what should the loadingPrincipal and triggeringPrincipal of that load be?
If we choose to use CSP for it should we report violations?

My current patch in bug 1453751 uses the content page's principal as the loading and triggering principals for the load so we respect CSP and report a bunch of new violations in tests that aren't expecting them so I need to either change something about the above or fix the tests to ignore favicon CSP violations.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 20 • 6 years ago

(In reply to Dave Townsend [:mossop] from comment #19)
> I've ended up fixing this bug with the patch in bug 1453751.
Ha, before I joined Mozilla I always wanted to have favicons being governed by CSP correctly - thank you sir!

> But I have a question about how CSP applies to favicons. The spec makes it
> clear that <link> tags that load icons are governed by img-src. But when
> Firefox doesn't see a favicon defined in a page we guess and load
> "/favicon.ico".

Yes, indeed!

> Should that guessed load be governed by the CSP?
Ideally yes!

> Perhaps more generally, what should the loadingPrincipal and
> triggeringPrincipal of that load be?

The document's principal should be loadingPrincipal.

> If we choose to use CSP for it should we report violations?
I would say yes.
 
> My current patch in bug 1453751 uses the content page's principal as the
> loading and triggering principals for the load so we respect CSP

Yes, that is the correct principal to use.

> and report
> a bunch of new violations in tests that aren't expecting them so I need to
> either change something about the above or fix the tests to ignore favicon
> CSP violations.

So, that clearly indicates your patch works as expected. I would imagine fixing the tests is what we have to do. Are lots failing? I mean, can we just update some tests manually? Alternatively we could set some kind of pref I guess.

	Dave Townsend [:mossop] Assignee
	Comment 21 • 6 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20)
> > and report
> > a bunch of new violations in tests that aren't expecting them so I need to
> > either change something about the above or fix the tests to ignore favicon
> > CSP violations.
> 
> So, that clearly indicates your patch works as expected. I would imagine
> fixing the tests is what we have to do. Are lots failing? I mean, can we
> just update some tests manually? Alternatively we could set some kind of
> pref I guess.

I have a lot of failures but I don't know how many of them are this specific issue just yet. We also do have a pref to turn off this favicon guessing behaviour so that may be appropriate in some cases.

The web-platform tests are the ones that I know are failing here, this test for example see's a failure because the favicon load is blocked by the default-src setting: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/form-action/form-action-src-default-ignored.sub.html

Since the spec doesn't really document what to do about this case is it likely changes to those tests would be accepted or do you think I should just turn off this behaviour for the web-platform tests?

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 22 • 6 years ago

(In reply to Dave Townsend [:mossop] from comment #21)
> Since the spec doesn't really document what to do about this case is it
> likely changes to those tests would be accepted or do you think I should
> just turn off this behaviour for the web-platform tests?

I am fine either way. Personally I think it makes sense to just flip that pref for the entire CSP web platform test folder, but i could also see changes to wpt tests would be accepted. As I said, I am fine either way.

Thanks for doing this!

	Dave Townsend [:mossop] Assignee
	Comment 23 • 6 years ago

Fixed by bug 1453751

	Eric Shepherd [:sheppy]
	Comment 24 • 6 years ago

Updated documentation:

https://developer.mozilla.org/en-US/docs/Learn/HTML/Introduction_to_HTML/The_head_metadata_in_HTML
https://developer.mozilla.org/en-US/docs/Web/HTML/CORS_enabled_image#Storing_an_image_from_a_foreign_origin
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link

Firefox 63 for developers

	Eric Shepherd [:sheppy]
	Comment 25 • 6 years ago

Somehow I translated this to CORS repeatedly in my head. I need to fix this up; will do tomorrow. Clearly too sleepy tonight. :)

	Eric Shepherd [:sheppy]
	Comment 26 • 6 years ago

Fixed that. Now docs are really complete.