Feed preview's request for favicon.ico should not send Referer

RESOLVED FIXED in Firefox 3 alpha1

Status

()

Firefox
RSS Discovery and Preview
P1
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Pike, Assigned: mano)

Tracking

({fixed1.8.1.1, privacy})

2.0 Branch
Firefox 3 alpha1
fixed1.8.1.1, privacy
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Just setting the image of a menuitem is apparently sending a referer when getting that image, which may be ok in the general context of a xul menu.

In the context of feed preview though, it sends the URL of the users feed to the service, at least when that service manages to not get his favicon cached.

We should fix that, maybe by hooking a loader object in the middle explicitly and use data urls, or just a x-no-referer protocol or so.
http://www.legroom.net/modules.php?op=modload&name=News&file=article&sid=215 implies that this bug should probably be open ...
Should this be fixed for 1.8.1.1 as this is a privacy issue?
Flags: blocking1.8.1.1?
Group: security
Keywords: privacy

Comment 3

11 years ago
This is bad. We should do some kine of announcement that this is a bug and not a desired feature. 
 -> my overloaded plate.
Assignee: nobody → mano
Priority: -- → P1
Target Milestone: --- → Firefox 3 alpha1

Comment 5

11 years ago
now reported at http://www.heise-security.co.uk/news/80310
Created attachment 244253 [details] [diff] [review]
use data uris
Attachment #244253 - Flags: review?(gavin.sharp)

Comment 7

11 years ago
Does this patch also fix the cookie issue, so cookies aren't shared for this request, and so if you have the "ask me every time a site tries to set a cookie" option set you won't see the dialog?

Is it ok to treat all favicon.ico files as image/x-icon, ignoring the mime type sent by the server?  I'm guessing imagelib sniffs anyway.

Comment 8

11 years ago
http://www.legroom.net/modules.php?op=modload&name=News&file=article&sid=215 says:

"I find it very odd that Firefox 2.0 loads the Google favicon, and only the Google favicon, remotely for the Feed Preview page, when the favicons for three other services displayed in the same page are loaded locally."

Do we know why he saw that behavior?  To me it looks like the code loads favicon.ico remotely from all the services.

Updated

11 years ago
Summary: feed preview's favicon should not send referer → Feed preview's request for favicon.ico should not send Referer or cookies

Comment 9

11 years ago
Jesse, I'm the one that posted the information on legroom.  No, I don't know why Google was the only site I saw that exhibited this behavior.  It was confirmed to happen for Yahoo as well according to the Heise article and another Firefox user that had e-mailed me privately over the weekend, but I haven't had the time to look into it any further myself.  It may very well load the icon from all other servers, though for whatever reason in the few test cases I tried on my system I only saw traffic going to Google.

If you'd like I'd be happy to test further and see what I can find.  I see a lot of big names already included on the comments in this bug, though, so I'm not sure how much information I could add at this point, but I'm certainly willing to help.  Just let me know.
*** Bug 359060 has been marked as a duplicate of this bug. ***
Comment on attachment 244253 [details] [diff] [review]
use data uris

>Index: browser/components/feeds/src/FeedWriter.js

>       var ios = Cc["@mozilla.org/network/io-service;1"].
>                 getService(Ci.nsIIOService);
>       for (var i = 0; i < handlers.length; ++i) {
>         menuItem = this._document.createElementNS(XUL_NS, "menuitem");
>         menuItem.className = "menuitem-iconic";
>         menuItem.setAttribute("label", handlers[i].name);
>         menuItem.setAttribute("handlerType", "web");
>         menuItem.setAttribute("webhandlerurl", handlers[i].uri);
>-
>-        var uri = ios.newURI(handlers[i].uri, null, null);
>-        menuItem.setAttribute("image", uri.prePath + "/favicon.ico");
>-
>         handlersMenuPopup.appendChild(menuItem);
>+
>+        // For privacy reasons we cannot set the image attribute directly
>+        // to the icon url, see Bug 358878
>+        var uri = makeURI(handlers[i].uri);
>+        if (uri) 
>+          new iconDataURIGenerator(uri.prePath + "/favicon.ico", menuItem);

ios is now unused (and the one in _getContainer too).

Note that http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp&rev=1.296&mark=3820#3810 asks your notificationCallbacks object for nsIPrompt, which it currently doesn't implement, so an exception is thrown (from QI). Since that code doesn't seem to actually do anything with the prompt (why does SetCookieStringFromHttp have an aPrompt argument that it doesn't use?) you can probably just get away with adding nsIPrompt to your QI to silence the warning.

Copying this code over really sucks, but I can't think of a better solution :(
Attachment #244253 - Flags: review?(gavin.sharp) → review-
Also note that this patch doesn't fix the fact that the cookie is sent with the response - we could fix that using the technique described at http://developer.mozilla.org/en/docs/Creating_Sandboxed_HTTP_Connections#Handling_cookies to avoid receiving or sending cookies for these requests.

Perhaps Darin or biesi could answer the question in comment 11, or provide any suggestions for fixing this another way.
Created attachment 244511 [details] [diff] [review]
patch

Note this does not address the cookies thing.
Attachment #244253 - Attachment is obsolete: true
Attachment #244511 - Flags: review?(gavin.sharp)
Status: NEW → ASSIGNED
And please separate this out to another bug (which also applies to the feed preferences pane, by the way).
Summary: Feed preview's request for favicon.ico should not send Referer or cookies → Feed preview's request for favicon.ico should not send Referer
Attachment #244511 - Flags: review?(gavin.sharp) → review+
Created attachment 244661 [details] [diff] [review]
as checked in

mozilla/browser/components/feeds/src/FeedWriter.js 1.24
Attachment #244511 - Attachment is obsolete: true
Attachment #244661 - Flags: approval1.8.1.1?
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(In reply to comment #12)
> Perhaps Darin or biesi could answer the question in comment 11, or provide any
> suggestions for fixing this another way.

You mean this part?
> Copying this code over really sucks, but I can't think of a better solution :(

Which code is that referring to? (And where was it copied from?)
nsSearchService.js
(In reply to comment #16)
> You mean this part?
> > Copying this code over really sucks, but I can't think of a better solution :(

No, I meant the question about SetCookieStringFromHttp having an aPrompt argument that it doesn't use (though if you have other suggestions for a better solution I'm all ears :). I hadn't realized that it was a public interface, I guess that explains it.
Depends on: 360220
Attachment #244661 - Flags: approval1.8.1.1?
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Created attachment 246221 [details] [diff] [review]
1.8 branch patch

Use "src" attribute and workaround bug 360220.
Attachment #246221 - Flags: review?(gavin.sharp)
Attachment #246221 - Flags: approval1.8.1.1?
Attachment #246221 - Flags: review?(gavin.sharp) → review+

Comment 20

11 years ago
Comment on attachment 246221 [details] [diff] [review]
1.8 branch patch

Approved for 1.8.1 branch, a=jay for drivers.  Please land this asap, thanks!
Attachment #246221 - Flags: approval1.8.1.1? → approval1.8.1.1+
1.8 branch: mozilla/browser/components/feeds/src/FeedWriter.js  1.2.2.26
Keywords: fixed1.8.1.1

Comment 22

11 years ago
Jared B. or anyone else that is able: Could you please help us verify this fix?  Please retest with the latest "-mozilla1.8/" nightly build from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ and let us know if things work better for you.  Please replace "fixed1.8.1.1" with the "verified1.8.1.1" keyword if everything look good.  Thanks!
Depends on: 363318
There's no reason to call splice in the base64 encoder -- it mutates the array gratuitously, and at linear cost, which compounds quadratically.  Walking an index through the array is much, much cheaper.  If this._bytes needs to be truncated at the end of the b64 function, then that can be done once by setting length to 0.

/be
Depends on: 365570

Comment 24

10 years ago
Has this been fixed in the 2.x branch?
Martin: yes it did, see comment 21 and the keywords above.
You need to log in before you can comment on or make changes to this bug.