We have a content policy, but this function is bypassing it. As a result I was asked to review a hack to gecko. Essentially this broke the API for nsIContentPolicy
Created attachment 422564 [details] [diff] [review] Proposed fix This makes the image preloading stuff follow the script preloading stuff in terms of content policy calls.
Comment on attachment 422564 [details] [diff] [review] Proposed fix Perfect, thank you. We need to get this on the branches, since this is missing not just the content policy but more importantly CheckLoadURI.
Probably doesn't block 1.9.2, but flagging just to make sure we make that decision somewhere other than my head....
Oh, and 1.9.1 is unaffected, looks like. Good.
Comment on attachment 422564 [details] [diff] [review] Proposed fix I should have patched this in the public version of this bug (bug 539686), if I'd done so, I wouldn't need an sr.
Comment on attachment 422564 [details] [diff] [review] Proposed fix sr=dbaron Does the idea that a document will be passed as the context need to be documented anywhere?
We already pass in the document as the context for various image loads (e.g. CSS generated content and background images), fwiw.
(In reply to comment #3) > Probably doesn't block 1.9.2, but flagging just to make sure we make that > decision somewhere other than my head.... Who can help us make this decision, and what's the effect of taking / not taking this fix on 1.9.2?
The effect of not taking is that we have a security hole. How serious the hole is depends on what extension-implemented protocols the user happens to have installed and what they do. In a default installation you can certainly more or less freeze the browser on Linux/OSX (e.g. by pointing an <img> to /dev/tty) or perhaps crash it due to out of memory (e.g. by pointing an <img> to /dev/random, depending on what imagelib does with that situation). On all OSes you can probe to see which extensions are installed and the like, at least if they include images. On Linux you may be able to trigger the browser to ssh with the user's credentials and execute programs on the ssh target host (possibly including localhost). Yay gnome-vfs. Does the places protocol handler only give read access to data? Or can it do something bad? The effect of taking is that we need to respin, presumably. The patch is very very safe, imo. The only effect it might have is a very slight Tp hit; I would be really surprised if it's noticeable at all.
OK. Talked to Shawn; the places handler doesn't exist on 1.9.2, and the annotation handler (which I'd missed while writing comment 10) is non-destructive. Since the page can't get at the preload data in an way (which means the "can probe" thing above is also not possible), that means the only obvious issues are the /dev/* loads, gnome-vfs, and whatever extensions happen to do.
fwiw, we're a downstream that intends to use 1.9.2, we need this hook, so we'd prefer to have it in 1.9.2 instead of making us make a hash of things, which would involve us publishing the delta and people wondering why we diverged.
Timeless: what's your timeline on needing this fix in your consuming project?
sorry, i don't get bugmail for security bugs. i have no idea, it could be next week, it could be next month. it will not be more than 6 weeks, i hope. i really have absolutely no useful visibility into when management decides to release (my crystal ball never worked, and it's much cloudier here than it was in the new world, too many witches). worse, they randomly add releases to the 'roadmap' (1.0.1 was a surprise before 1.1, and there's a rumor of a 1.1.1 which will include browser fixes, although i'm assuming but not certain that it wouldn't include our new gecko). we also need to pull this patch in for testing. given that it has landed in m-c, it'll probably be imported today.
Note that I had at least one Adblock Plus user already who decided to downgrade to Firefox 3.5 because of this bug. For some people this issue is essential, be it because supposedly blocked tracking images are being loaded or because of wasted bandwidth (yes, there are still people paying for bandwidth). So I would strongly recommend taking this on 1.9.2 branch.
In case it wasn't clear, this is a critical security issue that absolutely needs to land on 1.9.2 ASAP (I'm a little sad it didn't land in time for Fennec to pick it up). The only reason it wasn't in 1.9.2 final was because it was found so late.
Comment on attachment 422564 [details] [diff] [review] Proposed fix Approved for 126.96.36.199, a=dveditz for release-drivers
(In reply to comment #16) > Note that I had at least one Adblock Plus user already who decided to downgrade > to Firefox 3.5 because of this bug. For some people this issue is essential, be > it because supposedly blocked tracking images are being loaded or because of > wasted bandwidth (yes, there are still people paying for bandwidth). So I would > strongly recommend taking this on 1.9.2 branch. Can you give me some clear steps to reproduce the problem for users so I can verify the fix?
Al, that should work: https://adblockplus.org/forum/viewtopic.php?p=30741#p30741 Note that the issue isn't visible every time, you might need to clear the cache and reload the page a few times (which is why I wasn't able to reproduce it at first).
Or you could stick an <img src="file:///dev/tty"> in a web page, stick a slow-loading <script> after it, and see if it hangs your browser on Linux.
(In reply to comment #21) > Al, that should work: > https://adblockplus.org/forum/viewtopic.php?p=30741#p30741 > Note that the issue isn't visible every time, you might need to clear the cache > and reload the page a few times (which is why I wasn't able to reproduce it at > first). Verified for 1.9.2 using the testcase described in the forum with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52pre) Gecko/20100315 Namoroka/3.6.2pre (.NET CLR 3.5.30729). I'm not seeing anything from /smilies with this build but did with 184.108.40.206.
(In reply to comment #17) > In case it wasn't clear, this is a critical security issue What am I missing? sounds like a DoS and potential web bug tracking (sg:low) type issue.
> What am I missing? Any failure to CheckLoadURI was pretty much sg:critical on Linux at some point due to gnome-vfs allowing things like ssh:// URIs and the like. Did we fix that?
Oh, and various extension protocol schemes are really bad news if content script can trigger arbitrary URI loads through them, last I checked.
We limit gnome-vfs to smb: and sftp: (or any a user has added to a hidden pref with no default value). Upping to sg:moderate assuming some add-on somewhere adds a protocol handler you really don't want to be called (but most users won't have it installed).