Closed
Bug 1196784
Opened 10 years ago
Closed 9 years ago
picture tag and img tag with srcset attribute bypass permissions.default.image=2
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: msazator, Assigned: edgar81)
References
Details
Attachments
(1 file, 1 obsolete file)
3.32 KB,
patch
|
jdm
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
||
Updated•10 years ago
|
Component: Untriaged → DOM: Security
Product: Firefox → Core
Updated•10 years ago
|
Flags: needinfo?(john)
Comment 1•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
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)
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 3•10 years ago
|
||
(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)
Comment 5•9 years ago
|
||
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?
Comment 7•9 years ago
|
||
(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)
Comment 10•9 years ago
|
||
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)
Comment 11•9 years ago
|
||
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.
Assignee | ||
Comment 12•9 years ago
|
||
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>
Comment 13•9 years ago
|
||
I can't really try it myself without the patch mentioned in comment 10 :)
Comment 14•9 years ago
|
||
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.
Assignee | ||
Comment 15•9 years ago
|
||
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.
Comment 16•9 years ago
|
||
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://.
Assignee | ||
Comment 17•9 years ago
|
||
Then how should I define the test - because the test engine won't allow me to use http:// ?
Comment 18•9 years ago
|
||
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.
Assignee | ||
Comment 19•9 years ago
|
||
Comment 20•9 years ago
|
||
The patch is missing the new test HTML files used in the test. Please add them and re-attach :)
Assignee | ||
Comment 21•9 years ago
|
||
Added missing html files.
Attachment #8725126 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8725570 -
Flags: review?(josh)
Comment 22•9 years ago
|
||
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+
Assignee | ||
Comment 23•9 years ago
|
||
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!
Comment 24•9 years ago
|
||
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.
Assignee | ||
Comment 26•9 years ago
|
||
Comment 27•9 years ago
|
||
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
Assignee | ||
Comment 28•9 years ago
|
||
Assignee | ||
Comment 30•9 years ago
|
||
And thank you for your patience :)
Comment 31•9 years ago
|
||
Keywords: checkin-needed
Comment 32•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
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
Comment 34•6 years ago
|
||
Comment 35•3 years ago
|
||
فروش اقساطی خودرو
https://sepandkhodro.com/
You need to log in
before you can comment on or make changes to this bug.
Description
•