Make sure favicons are governed by CSP

NEW
Unassigned

Status

()

Core
DOM: Security
P3
normal
a year ago
7 months ago

People

(Reporter: ckerschb, Unassigned)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-backlog1])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Updated

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

Comment 1

a year ago
Created attachment 8783865 [details] [diff] [review]
favicon_csp_mochitest.patch

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

Comment 2

a year ago
Created attachment 8802902 [details] [diff] [review]
favicon_csp_mochitest.patch

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

Comment 3

a year 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)
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

a year 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

a year 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
You need to log in before you can comment on or make changes to this bug.