Closed
Bug 1492207
Opened 6 years ago
Closed 6 years ago
Favicon loads show up in resource timing
Categories
(Core :: Networking, defect, P3)
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)
(deleted),
video/quicktime
|
Details |
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.
Updated•6 years ago
|
Group: firefox-core-security → network-core-security
Component: General → Networking
Product: Firefox → Core
Comment 1•6 years ago
|
||
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
status-firefox62:
--- → unaffected
status-firefox63:
--- → affected
status-firefox-esr60:
--- → unaffected
status-geckoview62:
--- → unaffected
tracking-firefox63:
--- → +
tracking-firefox64:
--- → +
Ever confirmed: true
Flags: needinfo?(dtownsend)
Keywords: regression
Version: 64 Branch → 63 Branch
Comment 2•6 years ago
|
||
Can anyone explain what this means?
Comment 3•6 years ago
|
||
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)
Reporter | ||
Comment 4•6 years ago
|
||
(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)
Reporter | ||
Comment 5•6 years ago
|
||
Comment 6•6 years ago
|
||
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.
Reporter | ||
Comment 7•6 years ago
|
||
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.
Comment 8•6 years ago
|
||
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
Comment 9•6 years ago
|
||
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.
Comment 10•6 years ago
|
||
(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)
Comment 11•6 years ago
|
||
We could definitely hide the favicon load from resource timing. That's not a security issue, though.
Comment 12•6 years ago
|
||
(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.
Comment 13•6 years ago
|
||
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]
Comment 14•6 years ago
|
||
Boris, per comment 11, should can I unhide this? Thanks.
Flags: needinfo?(bzbarsky)
Updated•6 years ago
|
Group: network-core-security
Comment 17•6 years ago
|
||
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
Updated•6 years ago
|
tracking-firefox63:
+ → ---
tracking-firefox64:
+ → ---
Blocks: tab-unloading
Comment 18•6 years ago
|
||
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)
Comment 19•6 years ago
|
||
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)
Comment 20•6 years ago
|
||
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.
Description
•