Make sure favicons are governed by CSP

RESOLVED FIXED

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: ckerschb, Assigned: mossop)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(7 attachments, 2 obsolete attachments)

No description provided.
Blocks: csp-w3c-3
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Posted patch favicon_csp_mochitest.patch (obsolete) — Splinter Review
Here is a WIP mochitest illustrating the problem.
Posted patch favicon_csp_mochitest.patch (obsolete) — Splinter Review
Just rebased the test.
Attachment #8783865 - Attachment is obsolete: true
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?
Flags: needinfo?(tihuang)
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?
Flags: needinfo?(tihuang)
(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.
Flags: needinfo?(fbraun)
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?
Flags: needinfo?(fbraun) → needinfo?(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.
Flags: needinfo?(ckerschb)
Posted file 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.
Duplicate of this bug: 1363031
Duplicate of this bug: 1428050
See Also: → 1433700
I think I can see how to fix this.
Assignee: nobody → dtownsend
Blocks: 1433700
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.
Attachment #8802902 - Attachment is obsolete: true
Depends on: 1453751
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.
Flags: needinfo?(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.
Flags: needinfo?(ckerschb)
(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?
Flags: needinfo?(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!
Flags: needinfo?(ckerschb)
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Somehow I translated this to CORS repeatedly in my head. I need to fix this up; will do tomorrow. Clearly too sleepy tonight. :)
Fixed that. Now docs are really complete.
You need to log in before you can comment on or make changes to this bug.