1297156 - Make sure favicons are governed by CSP

Status: RESOLVED FIXED

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: favicon_csp_mochitest.patch

Here is a WIP mochitest illustrating the problem.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: favicon_csp_mochitest.patch

Just rebased the test.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 4

[Tim Huang[:timhuang]]

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?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 6

[Frederik Braun [:freddy]]

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?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 8

[Frederik Braun [:freddy]]

Attachment: stack.txt

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.

Comment 11

[Dave Townsend [:mossop]]

I think I can see how to fix this.

Comment 12

[Dave Townsend [:mossop]]

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.

Comment 13

[Dave Townsend [:mossop]]

Attachment: Bug 1297156: Include content security policy when structured cloning principals.

When JavaScript sends an nsIPrincipal to the main process we use the structured
clone algorithm to serialize it. Currently this discards any content security
policy on the principal. This change includes that information so the CSP
directives can be seen in the main process.

Review commit: https://reviewboard.mozilla.org/r/235834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235834/

Comment 14

[Dave Townsend [:mossop]]

Attachment: Bug 1297156: Fix the naming of the triggering principal argument in nsContentUtils.

nsContentUtils::LoadImage accepts an argument labelled as aLoadingPrincipal
but we actually pass in triggering principals and it passes it along to a
method that accepts a triggering principal so this renames the argument.

Review commit: https://reviewboard.mozilla.org/r/235836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235836/

Comment 15

[Dave Townsend [:mossop]]

Attachment: Bug 1297156: Check favicon loads from the system principal against the content policy.

When a system principled xul:image element attempts to load anything we
currently bypass content policy checks. This change makes us continue to respect
the content policy when loading favicons.

Review commit: https://reviewboard.mozilla.org/r/235838/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235838/

Comment 16

[Dave Townsend [:mossop]]

Attachment: Bug 1297156: Always use the content security policy from the triggering principal for favicon loads.

Normally we use the CSP defined by the principal of the node loading something.
In the case of a xul:image element loading a favicon we need to use the
triggering principal.

Review commit: https://reviewboard.mozilla.org/r/235840/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235840/

Comment 17

[Dave Townsend [:mossop]]

Attachment: Bug 1297156: Fire an error event to xul:image elements when the attempt to load an image fails synchronously.

When the attempt to load an image returns a failure we need to dispatch an
error event to the xul element.

Review commit: https://reviewboard.mozilla.org/r/235842/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235842/

Comment 18

[Dave Townsend [:mossop]]

Attachment: Bug 1297156: Test that favicon loads are correctly blocked by content security policies.

Review commit: https://reviewboard.mozilla.org/r/235844/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235844/

Comment 19

[Dave Townsend [:mossop]]

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.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 21

[Dave Townsend [:mossop]]

(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?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

(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!

Comment 23

[Dave Townsend [:mossop]]

Fixed by bug 1453751

Comment 24

[Eric Shepherd [:sheppy]]

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

Comment 25

[Eric Shepherd [:sheppy]]

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

Comment 26

[Eric Shepherd [:sheppy]]

Fixed that. Now docs are really complete.