366568 - potential problem with favicons and useDefaultIcon

Status: RESOLVED INVALID

(Firefox :: Security, defect)

Description

[georgi - hopefully not receiving bugspam]

no exploit for this yet, filing seperate bug per bz's suggestion.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/tabbrowser.xml&rev=1.216&mark=496-517#496


in tabbrowser.xml |shouldLoadFavIcon| tries to prevent active content in
favicons. this does not work for |data:image/png;,VALIDPNG| (in this case the
uri is loaded as favicon because of the special ImageDocument check) - so at
least
data: uris are loaded as favicons. in case |javascript:| uris may return 
|instanceof ImageDocument| this may lead to problems with favicons (according to bz this is possible).

probably other abuse may be possible in this case.

having in mind the problems with active favicons in the past, regardless
whether comment #86 is exploitable or not, the check for sane favicon better be
applied for the |image| case.

Comment 1

[georgi - hopefully not receiving bugspam]

example javascript uri will be appreciated - manage to get just 'text/html' or 'text/plain' from javascript.

Comment 2

[georgi - hopefully not receiving bugspam]

dveditz, believe that can fix this tomorrow my time by moving |shouldload...| in the innermost place that doesn't break world no matter if the bug is real or imaginary. feel free to assign to me if you feel like it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, hmm...  So actually, javascript: seems to always return text/html for now:

494     // If the resultant script evaluation actually does return a value, we
495     // treat it as html.
496     rv = NS_NewInputStreamChannel(getter_AddRefs(channel), aURI, mIOThunk,
497                                   NS_LITERAL_CSTRING("text/html"));
498     if (NS_FAILED(rv)) return rv;

While this works for loading images with <img> because that ignores the type, that won't work for image documents... unless we start to content-sniff more or something.

I still think we should check here.  Just a checkLoadURI for DISALLOW_SCRIPT should do the trick.  No need to filter out data:.

Comment 4

[Daniel Veditz [:dveditz]]

(In reply to comment #0)
> having in mind the problems with active favicons in the past, regardless
> whether comment #86 is exploitable or not,

comment 86 in what other bug?

Comment 5

[georgi - hopefully not receiving bugspam]

(In reply to comment #4)
> 
> comment 86 in what other bug?
> 

in the meta. this was just mistyping, the full description is in the report.

Comment 6

[Jay Patel [:jay]]

We need an owner and patch quickly if this is going to make the next release... otherwise this is at-risk for being pushed out.

Comment 7

[Mike Connor [:mconnor]]

Gavin, Boris's comment 3 should get you started

Comment 8

[Mike Connor [:mconnor]]

mustfix for b1

Comment 9

[georgi - hopefully not receiving bugspam]

i doubt this is exploitable => [sg:nse]

Comment 10

[Mike Connor [:mconnor]]

Not blocking on this bug for final ship. Would take a safe enough patch if one comes through.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

I can't really make any sense out of why we have both browser.chrome.site_icons and browser.chrome.favicons. Rather than explicitly checking for javascript:, I decided to refactor the code and just re-use shouldLoadFavicon (which restricts to http[s]).

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 603960 [details] [diff] [review]
patch

> re-use shouldLoadFavicon (which restricts to http[s]).

This restriction makes sense for appending /favicon.ico, but I don't think it makes sense for image documents. E.g. we still want favicons for file:// URI images.

Comment 13

[Dão Gottwald [:dao]]

I think we can resolve this as invalid as per comment 3...

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Still seems prudent to check for javascript:, I think.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

I changed my mind!