Open Bug 331257 Opened 14 years ago Updated 1 year ago

data: images show up when 'load images' (Tools->Options->content->checkbox) is disabled

Categories

(Core :: Image Blocking, defect, trivial)

defect
Not set
trivial

Tracking

()

People

(Reporter: hobophobe, Unassigned)

References

()

Details

Attachments

(1 file, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1

It is unclear whether this is intended; the option to turn off 'load images' in the preferences does not halt the rendering of data: url images. Arguably the option above is only meant to disable loading of images external to the HTML/XHTML file, but would it then be a future addition to have a separate option to exclude data: images?

Reproducible: Always

Steps to Reproduce:
1.Turn off the option (Tools->Options->'content'->uncheck 'load images' checkbox)
2.Go to the above URL or any with a data: image included
Actual Results:  
Data: Image shows up as normal

Expected Results:  
Depending, not show up, or if the option is not intended to disable data: images, it works fine.

Marked as trivial. Would be good to have a solution in mind, and eventually implemented, but this is not critical by any means given especially that data: images are infrequently used at all.
Depends on: 144766
Version: unspecified → 1.5.0.x Branch
Assignee: nobody → security-bugs
Component: General → Image Blocking
Product: Firefox → Core
QA Contact: general
Version: 1.5.0.x Branch → 1.8 Branch
Yeah.. the problem is this code in nsContentBlocker::ShouldLoad:

166   // we only want to check http, https, ftp
167   // for chrome:// and resources and others, no need to check.
168   nsCAutoString scheme;
169   aContentLocation->GetScheme(scheme);
170   if (!scheme.LowerCaseEqualsLiteral("ftp") &&
171       !scheme.LowerCaseEqualsLiteral("http") &&
172       !scheme.LowerCaseEqualsLiteral("https"))
173     return NS_OK;

Maybe we should use a URI flag here instead?  And set it on more protocols?

Note that this is not restricted to data: -- ftp: , gopher:, etc could have similar problems.
Blocks: 144766
Status: UNCONFIRMED → NEW
No longer depends on: 144766
Ever confirmed: true
Flags: blocking1.9?
Assignee: security-bugs → nobody
Boris, still think this should block?
Well...  it depends on why we have this preference.  If we're OK with it not working in a bunch of cases, then it shouldn't.

I do think this should be wanted, at least.
Version should be changed to Trunk, and if it is an issue on all platforms (I can only test with Windows Vista), Hardware and OS to All too.
-'ing and marking as wanted per boris' comment.
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
OS: Windows XP → All
Hardware: PC → All
Version: 1.8 Branch → Trunk
This is still confirmed in FF 3.1b2 on WinXP.

I believe when images are disabled by the user, data URI images should NOT be loaded because the user intends to disable all images on the pages with this option (no matter what their source is).

Also all other browsers do not load data URI images when images are disabled by the user (only FF loads them).
QA Contact: image-blocking
Duplicate of this bug: 643146
Duplicate of this bug: 644134
Duplicate of this bug: 644134
Duplicate of this bug: 501455
Duplicate of this bug: 658048
Everything we want to consider blocking, except the file protocol, has the flag nsIProtocolHandler::URI_LOADABLE_BY_ANYONE.  file is the only protocol with URI_IS_LOCAL_FILE, so a separate check catches that.

This also adjusts nsContentBlocker::TestPermission to allow data URIs (which would now be blocked) for the BEHAVIOR_NOFOREIGN case (permissions.default.image == 3).
Attachment #615568 - Flags: review?(bzbarsky)
Comment on attachment 615568 [details] [diff] [review]
Rely on URI flags as suggested in comment 1

This is still hardcoding data: in nsContentBlocker, no?  This code will break with various other URI schemes, no?
The ShouldLoad() change works with all schemes.  We only consider blocking images in content (be it from local files, from data:, or from some network protocol).  The TestPermission() change was needed because data: is currently unique in that it ought not be considered a third-party, despite not having the same host as its context.  

data:'s uniqueness could change in the future, so this version replaces the data: hard-coding with a check for a new flag: nsIProtocolHandler::URI_INHERITS_HOST.  This improvement allows new protocols to use that flag and not require modifications to nsContentBlocker.

One possible alternative is using the URI_INHERITS_SECURITY_CONTEXT flag.  It's currently only used by data: and wyciwyg:, but the latter is already excluded in ShouldLoad().  That flag's meaning is different enough than needed here, so the new flag seems safer.  

It should also be noted that the flag doesn't literally cause the URI to inherit the context's host, just to be treated as though it does by nsContentBlocker.  If other code required access to a literal host, that could be changed for data: or other protocols using the flag.
Attachment #615568 - Attachment is obsolete: true
Attachment #615966 - Flags: review?(bzbarsky)
Attachment #615568 - Flags: review?(bzbarsky)
> data:'s uniqueness could change in the future,

It's not unique today.  Extensions can implement new protocol handlers, and quite a number do so.
> One possible alternative is using the URI_INHERITS_SECURITY_CONTEXT flag.

Yes, this is what you should use, imo.  That means it inherits the origin (so not just the host, but also the port and scheme) for purposes of security checks.
Perhaps a better question: if image loading is disabled for a site, should that site be able to load data: images?  That is, what behavior are we actually after?
(In reply to Boris Zbarsky (:bz) from comment #17)
> Perhaps a better question: if image loading is disabled for a site, should
> that site be able to load data: images?  That is, what behavior are we
> actually after?

We treat them as though they are from the context, so we use the context's settings as a proxy, so we can avoid changing TestPermission() altogether.  This patch is updated to reflect that.
Attachment #615966 - Attachment is obsolete: true
Attachment #616283 - Flags: review?(bzbarsky)
Attachment #615966 - Flags: review?(bzbarsky)
Comment on attachment 616283 [details] [diff] [review]
Use the URI_INHERITS_SECURITY_CONTEXT flag, and use the context's permission as a proxy for those protocols

r=me.  Thanks!
Attachment #616283 - Flags: review?(bzbarsky) → review+
Attachment #616283 - Flags: approval-mozilla-central?
I fail to see this bug's point. As I understand it, the point of that pref is to save bandwidth. Presumably that's why it says "load" rather than "display".
Keywords: checkin-needed
(In reply to Dão Gottwald [:dao] from comment #20)
> I fail to see this bug's point. As I understand it, the point of that pref
> is to save bandwidth. Presumably that's why it says "load" rather than
> "display".

Or block offensive images, or simplify page perusal, etc. IMHO this is the correct thing to do, especially since most users won't understand the difference between data: images and loaded images and will just think "Firefox is broken". So that would be another reason.
> Or block offensive images, or simplify page perusal, etc.

Wouldn't be the first time that people wanted to use features for something they weren't intended for. I still believe the reason we have that option is the one I mentioned. It's particularly useful for mobile browsing, where slow connections or bandwidth limits are still common.
Attachment #616283 - Flags: approval-mozilla-central? → approval-mozilla-central+
Comment on attachment 616283 [details] [diff] [review]
Use the URI_INHERITS_SECURITY_CONTEXT flag, and use the context's permission as a proxy for those protocols

I think we should close this as WONTFIX. The pref is working as intended.
Attachment #616283 - Flags: feedback-
I would prefer parity with other browsers.

Chrome: "Do not show any images" - no data: images
Safari: "Display images when the page opens" - no data: images
IE: "Show pictures" - no data: images

Opera is the exception here, with two settings: 
"No images" - no data: images 
"Cached images" - DOES display data: images
(In reply to lowbatteries from comment #24)
> I would prefer parity with other browsers.
> 
> Chrome: "Do not show any images" - no data: images
> Safari: "Display images when the page opens" - no data: images
> IE: "Show pictures" - no data: images

As phrased, this makes sense for them. Our option is labeled differently and I argue it's in line with why most users would disable images.
I think the feature, as it's currently presented to users, is about blocking images in content.  User expectation is that no images will display, which is what this bug is about.

Most users of it want to save bandwidth, but when they do run across a data: image, it is a distraction.  They may pause their activity to evaluate whether their browser is acting correctly.
Have the potential security ramifications of this bug been properly considered?

Apart from making it impossible for users to prevent themselves from being presented with potentially objectionable images, as already discussed, the embedded images must be decoded and rendered. When vulnerabilities are discovered in a browser's image code, isn't the usual first response to warn users to mitigate the bug by disabling images? While that advice would appear sound for every other browser, doesn't this bug present an obvious vector against Mozilla users under such circumstances?

Furthermore, some users who seek to make their browser as secure as possible routinely (attempt to) block images as a precaution against undisclosed "zero day" exploits. The current behaviour/bug would seem to negate their efforts and can come as quite a shock - as illustrated here: https://tails.boum.org/forum/Base64_Encoded_Images:_How_to_block_them__63__/

I can't find any discussion of potential security aspects to this bug yet, but I notice it was reassigned from a security-bugs@... address to nobody@mozilla.org (before comment 2). So as at least someone else must have been worried by this I've requested a review by the security team. I hope that's not some awful faux-pas :-B
Flags: sec-review?
Re comment #27:  

Images with malware are indeed known.  The US-CERT (an agency of the U.S. Department of Homeland Security) lists many.  Try the following query at the US-CERT Web site:
<http://search.us-cert.gov/search?utf8=%E2%9C%93&input-form=advanced&affiliate=us-cert&query-or=JPEG+GIF+PNG+BMP&per-page=10&filter=off&x=31&y=9>.
The security team doesn't think we need to review this decision from a security POV. Our understanding was that the original intent was to save bandwidth; not wanting to see objectionable content is a different valid reason, but it's a product reason and needs to be made at the product level.

Yes, image formats sometimes have vulnerabilities, but we have to fix those because most people will not discover this feature nor protect themselves using it. If we did decide this pref was actually a defensive measure then we would also need to disable <canvas> should it be flipped, because you can programmatically create images (of some formats, at least) using canvas.

Marking this "sec-review+" to indicate we've looked at the issue, not that we're endorsing this specific patch.
Flags: sec-review? → sec-review+
It's not a security issue, it's an accessibility issue. A user may want to disable images for a variety of reasons, which are none of our business really. A user with cognitive disabilities may simply find images in general to distractive and prefer a text only version of a web page. A user with low vision may want to save layout by stripping out images in an enlarged text view. A user may simply prefer to see alt text rather than the actual image.

Whatever the reasons are, the average user wouldn't have a clue what the difference is between an external image source and data URI image source, it's only us developers that would know this. Since this is a feature intended to improve user experience, it should function in a way that makes sense to that user. Unchecking a feature called "Load images automatically" should mean "disable all images", not "disable most but not all images".

In addition, handling data URI images consistently when images are disabled would make it a lot easier for web developers to detect whether those images are currently disabled or not. Because of this bug, we're forced to load an external sample image in order to be able to detect this in Firefox. I'n all other major browser we can use a data URI img src, which is much easier for this purpose.

The comments for this bug go back to 2006, but I haven't heard a single good reason why disabling images in the browser's user settings SHOULDN'T apply for data URI images as well. I would really appreciate it if it got fixed, if only for the sake of consistency.
I agree with what Hans Hillen said ^^above^^.

It feels like data URI images are a way for them to force images on me that I dont want, and ABP cant block them and neither can NoScript. And I was angry when found out from this thread that other browsers can block data URI images but firefox cant. I have stuck with firefox cuz I cant do without the addons such as ABP and NoScript, but I see that Opera now has addons like firefox so I am thinking about switching to Opera.

I am running across data URI images with increasing frequency.

A web page can have many large data URI images embedded in it making the user download several megabytes of content he does not want.

A bad example of data:image is here http://newworldordernews.com/page4.php I have a slow connection it takes me a coons age to download that page, I want to block all data:image on that page, and that page is not the worst example.

What I would really like is and addon that specifically blocks just the data URI images.
Duplicate of this bug: 1141161
(In reply to magnus from comment #31)
> What I would really like is and addon that specifically blocks just the data
> URI images.

Image Block addon might be what you're looking for.

https://addons.mozilla.org/en-US/firefox/addon/image-block/
You need to log in before you can comment on or make changes to this bug.