1492207 - Favicon loads show up in resource timing

Status: RESOLVED INVALID

(Core :: Networking, defect, P3)

Description

[Khalil Zhani]

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.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 52baefdfbeca055fccb7518db3fd51ceea0649dc
INFO: First bad revision: 40ed437da7ae4407ece2ad47cd8fe3c7943f9eb9
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=52baefdfbeca055fccb7518db3fd51ceea0649dc&tochange=40ed437da7ae4407ece2ad47cd8fe3c7943f9eb9

Comment 2

[Dave Townsend [:mossop]]

Can anyone explain what this means?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Khalil Zhani]

(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!

Comment 5

[Khalil Zhani]

Attachment: Untitled.mov

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 7

[Khalil Zhani]

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

[Dave Townsend [:mossop]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Dave Townsend [:mossop]]

(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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

We could definitely hide the favicon load from resource timing.  That's not a security issue, though.

Comment 12

[Dave Townsend [:mossop]]

(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

[Honza Bambas (:mayhemer)]

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.

Comment 14

[Andrew McCreight [:mccr8]]

Boris, per comment 11, should can I unhide this? Thanks.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I believe so, yes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Resummarizing to make it clear what issue is really left here.

Comment 18

[Dave Townsend [:mossop]]

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.

Comment 19

[Boris Zbarsky [: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.

Comment 20

[Dave Townsend [:mossop]]

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.