Closed Bug 1492207 Opened 6 years ago Closed 6 years ago

Favicon loads show up in resource timing

Categories

(Core :: Networking, defect, P3)

63 Branch
defect

Tracking

()

RESOLVED INVALID
Tracking Status
geckoview62 --- unaffected
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- wontfix
firefox64 --- fix-optional

People

(Reporter: chromium.khalil, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(1 file)

Steps to reproduce: 1. Go to bug 1408990 2. Cmd + click right on https://attack.shhnjk.com/resource_check.html 2. Wait for 3 seconds Actual results: Cross-origin URL after redirect is leaked. note: I can only repro this on Nightly version.
Group: firefox-core-security → network-core-security
Component: General → Networking
Product: Firefox → Core
INFO: Last good revision: 52baefdfbeca055fccb7518db3fd51ceea0649dc INFO: First bad revision: 40ed437da7ae4407ece2ad47cd8fe3c7943f9eb9 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=52baefdfbeca055fccb7518db3fd51ceea0649dc&tochange=40ed437da7ae4407ece2ad47cd8fe3c7943f9eb9
Blocks: 1453751
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Ever confirmed: true
Flags: needinfo?(dtownsend)
Keywords: regression
Version: 64 Branch → 63 Branch
Can anyone explain what this means?
I'm not following either. The page that's loaded is <https://attack.shhnjk.com/resource_check.html>. The browser then does a load of <https://attack.shhnjk.com/favicon.ico> and this is done in the context of the page. The page sees that this URL was loaded (via resource timing API). There is no cross-origin anything going on here. Maybe we should hide the favicon load from resource timing API, but it's not exposing cross-site information. Note that the testcase also does a load of an iframe, which does redirect bits, etc. But none of that is being exposed, as far as I can tell. Looking in the web console: > performance.getEntriesByType("resource").map((e) => e.name) Array [ "https://t.co/sklJNLqqjJ", "https://attack.shhnjk.com/favicon.ico" ] which is not exposing any information the site did not already have. Khalil, am I just missing something?
Flags: needinfo?(chromium.khalil)
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3) > I'm not following either. > > The page that's loaded is <https://attack.shhnjk.com/resource_check.html>. > The browser then does a load of <https://attack.shhnjk.com/favicon.ico> and > this is done in the context of the page. The page sees that this URL was > loaded (via resource timing API). > > There is no cross-origin anything going on here. Maybe we should hide the > favicon load from resource timing API, but it's not exposing cross-site > information. > > Note that the testcase also does a load of an iframe, which does redirect > bits, etc. But none of that is being exposed, as far as I can tell. > Looking in the web console: > > > performance.getEntriesByType("resource").map((e) => e.name) > > Array [ "https://t.co/sklJNLqqjJ", "https://attack.shhnjk.com/favicon.ico" > ] > > which is not exposing any information the site did not already have. > > Khalil, am I just missing something? But the alert shows "https://t.co/sklJNLqqjJ" not https://attack.shhnjk.com/favicon.ico!
Flags: needinfo?(chromium.khalil)
Attached video Untitled.mov (deleted) —
Huh, https://attack.shhnjk.com/favicon.ico is the only one I was seeing. And the regression range I found was when that testcase went from doing nothing at all (which was the same as Chrome's behavior) to popping up the alert dialog with the favicon.ico URL.
When I load https://attack.shhnjk.com/resource_check.html directly, I can only see the alert dialog with https://attack.shhnjk.com/favicon.ico, not like with using CMD + click right on https://attack.shhnjk.com/resource_check.html as in the attached video.
I can reproduce this with the t.co result and previous with the favicon result. It changed to t.co with https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=370fae337b8df463b3ae1ebb8665728d24f5601b&tochange=4073ae1a277ff00d40609dd62afc5cc60a705a10. Which is still me :s
That page does two loads, effectively. One is: <iframe src="https://t.co/sklJNLqqjJ"> The other is the favicon load. The alert is showing the URL of whatever the "second" load was, since it's grabbing the array of all performance entries and showing the name of the thing at [1]. Depending on how the favicon code races with the HTML parser and stuff (which might depend on background tab status or not?), maybe it could be "https://t.co/sklJNLqqjJ" or it could be "https://attack.shhnjk.com/favicon.ico". But in either case, both URLs are in performance.getEntriesByType("resource") and that's the right behavior. Neither one is leaking information, because the page _already_ knows it's loading the "https://t.co/sklJNLqqjJ" URL: it's right there in the page source. What would leak information is if the "https://www.bing.com/?secret" URL that "https://t.co/sklJNLqqjJ" loads were present in performance.getEntriesByType("resource") somewhere. But that's not happening, as far as I can tell. You should really just alert something like performance.getEntriesByType("resource").map((e) => e.name) to see the full list of everything the page is seeing. (And fwiw, the alert in the testcase is always showing "https://attack.shhnjk.com/favicon.ico" for me so far, no matter how I load the page... but that's not really relevant to the above discussion.) As far as I can see, there is no bug here.
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #9) > The alert is showing the URL of whatever the "second" load was. Almost certainly before bug 1453751 landed favicon loads would not show up in the resource timing APIs since their requests weren't associated with the document's loadgroup properly. Now I'm not surprised that they do since that association is happening properly. Whether favicon loads should show up in the resource timing API I don't know. It should be noted that the favicon load that is showing up here isn't one that the page technically asked for, there is no icon link in the page, it's just a favicon we guess at based on the URL. As far as I know there is no spec saying that browsers should do this guessing it's just something IE started a long time ago and everyone else follows suit. Maybe there's an argument that these icons in particular shouldn't show up in the timing API. I wonder what chrome does here.
Flags: needinfo?(dtownsend)
We could definitely hide the favicon load from resource timing. That's not a security issue, though.
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11) > We could definitely hide the favicon load from resource timing. That's not > a security issue, though. Agreed. It would probably be trivial to do too since we tag them with a content policy type of TYPE_INTERNAL_IMAGE_FAVICON.
Please feel free to raise the priority if this is more serious issue. It's somewhat hard to make an assessment for me by reading the comments.
Priority: -- → P3
Whiteboard: [necko-triaged]
Boris, per comment 11, should can I unhide this? Thanks.
Flags: needinfo?(bzbarsky)
I believe so, yes.
Flags: needinfo?(bzbarsky)
Group: network-core-security
Resummarizing to make it clear what issue is really left here.
Summary: Resource Timing API leaks URL after subframe navigation (repro issue 1408990) → Favicon loads show up in resource timing

So should we actually hide favicon loads from the resource timing API or is this a wontfix? I can't really tell from reading the specs.

Flags: needinfo?(bzbarsky)

I can't either. The relevant specs are ... <sigh>.

https://html.spec.whatwg.org/#rel-icon says that the fetch should be done from the document. So arguably at least in the <link rel="icon"> case it should in fact show up in resource timing.

Flags: needinfo?(bzbarsky)

Actually, looks like all requests made by a non-null client should be included. For both <link> and /favicon.ico cases the requests are required to use the document's relevant settings object as the client.

So I'm going to claim that including them is probably what we should do. Feel free to reopen if this seems wrong.

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: