Make sure favicons are governed by CSP

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 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)

Comment hidden (empty)
(Reporter)

Updated

2 years ago
Blocks: 1231788
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
(Reporter)

Comment 1

2 years ago
Created attachment 8783865 [details] [diff] [review]
favicon_csp_mochitest.patch

Here is a WIP mochitest illustrating the problem.
(Reporter)

Comment 2

2 years ago
Created attachment 8802902 [details] [diff] [review]
favicon_csp_mochitest.patch

Just rebased the test.
Attachment #8783865 - Attachment is obsolete: true
(Reporter)

Comment 3

2 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?
Flags: needinfo?(tihuang)

Comment 4

2 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?
Flags: needinfo?(tihuang)
(Reporter)

Comment 5

2 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.
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)
(Reporter)

Comment 7

2 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.
Flags: needinfo?(ckerschb)
Created attachment 8807587 [details]
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.
Keywords: dev-doc-needed
Duplicate of this bug: 1363031

Updated

10 months ago
Duplicate of this bug: 1428050

Updated

8 months ago
See Also: → bug 1433700
(Assignee)

Comment 11

7 months ago
I think I can see how to fix this.
Assignee: nobody → dtownsend
(Assignee)

Updated

7 months ago
Blocks: 1433700
(Assignee)

Comment 12

7 months 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.
Attachment #8802902 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Depends on: 1453751
(Assignee)

Comment 19

7 months 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.
Flags: needinfo?(ckerschb)
(Reporter)

Comment 20

7 months 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.
Flags: needinfo?(ckerschb)
(Assignee)

Comment 21

7 months 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?
Flags: needinfo?(ckerschb)
(Reporter)

Comment 22

7 months 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!
Flags: needinfo?(ckerschb)
(Assignee)

Comment 23

5 months ago
Fixed by bug 1453751
(Assignee)

Updated

5 months ago
Status: NEW → RESOLVED
Last Resolved: 5 months ago
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. :)
Keywords: dev-doc-complete → dev-doc-needed
Fixed that. Now docs are really complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.