Closed Bug 1196784 Opened 4 years ago Closed 4 years ago

picture tag and img tag with srcset attribute bypass permissions.default.image=2

Categories

(Core :: DOM: Security, defect)

40 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: msazator, Assigned: edgar81)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150812163655

Steps to reproduce:

1) Go to about:config.
2) Set permissions.default.image=2. After this firefox should not download/show images.
3) Navigate to es.ign.com (make sure you are not redirected to your local ign website)


Actual results:

Most images are gone, but some are still visible.

Closer inspection of a still visible image with "Inspect element" reveals img tag uses srcset attribute:

<img src="http://im.ziffdavisinternational.com/t/ign_es/screenshot/default/he-man-282_tnzb.300.jpg" alt="El guionista de Thor escribirá el reboot de He-Man - He Man" title="El guionista de Thor escribirá el reboot de He-Man - He Man" class="thumb" srcset="http://im.ziffdavisinternational.com/t/ign_es/screenshot/default/he-man-282_tnzb.600.jpg 2x" width="300">


Expected results:

No image should be downloaded/visible.
Component: Untriaged → DOM: Security
Product: Firefox → Core
Flags: needinfo?(john)
I have no idea where or how these permissions work. I would imagine they should be using contentpolicy, and the only spot I can find that seems to do that does honor TYPE_IMAGESET (which srcset images load as):

https://dxr.mozilla.org/mozilla-central/source/embedding/browser/nsWebBrowserContentPolicy.cpp?offset=2000#66
Flags: needinfo?(john)
This appears to function through nsContentBlocker::TestPermission, which is an implementation of nsIContentPolicy. This means that imagesets use a separate preference (permissions.default.imageset) and permission name than normal image loads.

Since we expose this to the user via page info as "Load Images", this pref and the associated site permission needs to be used, not the separate .imageset permission. Do you know why content-policy created a new type for imageset instead of reusing TYPE_IMAGE? Can we get rid of TYPE_IMAGESET and just use TYPE_IMAGE for everything? If not, can we check both TYPE_IMAGE and TYPE_IMAGESET before allowing these images to load?
Flags: needinfo?(john)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> This appears to function through nsContentBlocker::TestPermission, which is
> an implementation of nsIContentPolicy. This means that imagesets use a
> separate preference (permissions.default.imageset) and permission name than
> normal image loads.
> 
> Since we expose this to the user via page info as "Load Images", this pref
> and the associated site permission needs to be used, not the separate
> .imageset permission. Do you know why content-policy created a new type for
> imageset instead of reusing TYPE_IMAGE? Can we get rid of TYPE_IMAGESET and
> just use TYPE_IMAGE for everything? If not, can we check both TYPE_IMAGE and
> TYPE_IMAGESET before allowing these images to load?

IMAGESET is a different request context per spec, and has different behavior in the mixed content blocker among other things. For this particular case would just change the string lookup to use "image" for both: https://dxr.mozilla.org/mozilla-central/source/extensions/permissions/nsContentBlocker.cpp#47
Flags: needinfo?(john)
Duplicate of this bug: 1169196
A lot of sites are using src="imageurl" and using permissions.default.image=2 does nothing.

-You disable images to save bandwidth
-You go to this site for example http://www.computerandyou.net/2010/12/best-practices-to-get-better-battery-life-for-your-laptop/
-Images like this load src="http://www.computerandyou.net/wp-content/uploads/2015/10/desk-laptop-working-technology.jpg"
-Which is a huge 8.5 MB
-You are doomed

Hope this gets fixed.
(In reply to Alhaitham Ibrahim from comment #5)
> Hope this gets fixed.

Did you vote for this bug to be fixed?
(In reply to Eduard R. from comment #6)
> (In reply to Alhaitham Ibrahim from comment #5)
> > Hope this gets fixed.
> 
> Did you vote for this bug to be fixed?

Like a hundred years ago :)
(In reply to John Schoenick [:johns] from comment #3)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> > This appears to function through nsContentBlocker::TestPermission, which is
> > an implementation of nsIContentPolicy. This means that imagesets use a
> > separate preference (permissions.default.imageset) and permission name than
> > normal image loads.
> > 
> > Since we expose this to the user via page info as "Load Images", this pref
> > and the associated site permission needs to be used, not the separate
> > .imageset permission. Do you know why content-policy created a new type for
> > imageset instead of reusing TYPE_IMAGE? Can we get rid of TYPE_IMAGESET and
> > just use TYPE_IMAGE for everything? If not, can we check both TYPE_IMAGE and
> > TYPE_IMAGESET before allowing these images to load?
> 
> IMAGESET is a different request context per spec, and has different behavior
> in the mixed content blocker among other things. For this particular case
> would just change the string lookup to use "image" for both:
> https://dxr.mozilla.org/mozilla-central/source/extensions/permissions/
> nsContentBlocker.cpp#47

I have fixed it, using John's suggestion (i.e. changing the string lookup to use "image" for both).
I have the patch ready. If someone could assign me to the bug and walk me through the required steps, that would be great.

Thanks in advance!
Flags: needinfo?(john)
The best thing to do here is attach the patch and figure out who will review it. Do your changes include an automated test? You can see an example of one that changes preferences in dom/tests/mochitest/general/test_srcset_pref.html. Please create a new one in the same directory (listing it in the mochitest.ini file that exists in that directory) - you can run it using `./mach mochitest path/to/test.html`.
Assignee: nobody → edgar81
Flags: needinfo?(john)
Actually, we may want a reftest for this instead, to verify that no images appear. mxr.mozilla.org/mozilla-central/source/dom/html/reftests/reftest.list#45 is an example of a reftest that relies on srcset and changes preferences. These can be run with `./mach reftest path/to/test.html` instead.
I've created the test. However, the images always display. Give it a try yourself. Create a html file with an img element having its source image on your disk. Even if you change permissions.default.image to 2, the image will still be displayed.

When opening a page on the internet, it works as expected.

How should I define the test so it works locally?

pref(permissions.default.image,1) == bug1196784-no-srcset.html bug1196784-with-srcset.html
pref(permissions.default.image,2) == bug1196784-no-srcset.html bug1196784-with-srcset.html

bug1196784-no-srcset.html

<html>
   <head>
      <title>test image</title>
   </head>
   <body>
      <img src="bug1196784.png" alt="some image" width="100" height="100"></img>
   </body>
</html>

bug1196784-with-srcset.html

<html>
   <head>
      <title>test image</title>
   </head>
   <body>
      <img src="bug1196784.png" srcset="bug1196784.png" alt="some image" width="100" height="100"></img>
   </body>
</html>
Status: NEW → ASSIGNED
I can't really try it myself without the patch mentioned in comment 10 :)
Oh, and I'm not sure if the preference is applied to both files or only the first file after the ==. That may be causing the test problems.
Sorry, I haven't been clear enough.

In a normal unpatched Firefox, create a html with an image in it.

Modifying permissions.default.image (1/2) will have no effect.

IE and Google Chrome won't load the image if I specifically set this behavior in the browser settings.
It works if I run a local HTTP server and view the page using http://. It doesn't work if I load the page via file://.
Then how should I define the test - because the test engine won't allow me to use http:// ?
The test should already be using HTTP automatically. My suspicion is the test is failing because of comment 14. The file that gets compared against should probably contain a broken image, or something that ends up appearing the same as an image with image loading disabled.
Attached patch bug1196784.patch (obsolete) — Splinter Review
The patch is missing the new test HTML files used in the test. Please add them and re-attach :)
Attached patch bug1196784.patchSplinter Review
Added missing html files.
Attachment #8725126 - Attachment is obsolete: true
Attachment #8725570 - Flags: review?(josh)
Comment on attachment 8725570 [details] [diff] [review]
bug1196784.patch

Review of attachment 8725570 [details] [diff] [review]:
-----------------------------------------------------------------

These changes look great; thank you for the patch! Do you have access to the tryserver yet? If not, you should apply for it: https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server
Attachment #8725570 - Flags: review?(josh) → review+
Thanks Josh! Just wondering if it's really needed... It's a trivial change and I guess someone else could commit the patch for me.

However, I will apply for level 1 access. Since you approved the patch, I will add you as voucher.

Thank you!
Was surprised to discover this bug doesn't occur with the new image blocking on Android (Bug 1170725), though it still shows data images (Bug 331257). Nice to see this bug being worked on.
Duplicate of this bug: 1248166
That try push only included a Windows 8 build and no tests. Could you do another one with this command, which will ensure the new tests are executed on multiple platforms and build configurations?

./mach try -b do -p linux,linux64,macosx64,win32,win64 -u reftest,reftest-1,reftest-2,reftest-3,reftest-4,reftest-e10s -t none
Looks good! Thank you for doing this work :)
Keywords: checkin-needed
And thank you for your patience :)
https://hg.mozilla.org/mozilla-central/rev/b94443c36774
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Summary: img tag with srcset attribute bypasses permissions.default.image=2 → picture tag and img tag with srcset attribute bypass permissions.default.image=2
Duplicate of this bug: 1334046
You need to log in before you can comment on or make changes to this bug.