Last Comment Bug 540642 - (CVE-2010-0168) nsDocument::MaybePreLoadImage doesn't play nicely with nsIContentPolicy
(CVE-2010-0168)
: nsDocument::MaybePreLoadImage doesn't play nicely with nsIContentPolicy
Status: RESOLVED FIXED
[sg:moderate]
: regression, verified1.9.2
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
: 533247 (view as bug list)
Depends on:
Blocks: 457809
  Show dependency treegraph
 
Reported: 2010-01-19 10:01 PST by timeless
Modified: 2010-03-24 00:28 PDT (History)
14 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.2+
.2-fixed
unaffected


Attachments
Proposed fix (1.48 KB, patch)
2010-01-20 10:31 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
bzbarsky: review+
dbaron: superreview+
dveditz: approval1.9.2.2+
Details | Diff | Review

Description timeless 2010-01-19 10:01:53 PST
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
Comment 1 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-01-20 10:31:37 PST
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 2 Boris Zbarsky [:bz] 2010-01-20 11:38:18 PST
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.
Comment 3 Boris Zbarsky [:bz] 2010-01-20 11:40:09 PST
Probably doesn't block 1.9.2, but flagging just to make sure we make that decision somewhere other than my head....
Comment 4 Boris Zbarsky [:bz] 2010-01-20 11:41:58 PST
Oh, and 1.9.1 is unaffected, looks like.  Good.
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-01-20 11:47:37 PST
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 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2010-01-20 11:54:52 PST
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?
Comment 7 Boris Zbarsky [:bz] 2010-01-20 11:59:16 PST
We already pass in the document as the context for various image loads (e.g. CSS generated content and background images), fwiw.
Comment 8 Blake Kaplan (:mrbkap) (please use needinfo!) 2010-01-20 12:09:28 PST
http://hg.mozilla.org/mozilla-central/rev/02135cbe7432
Comment 9 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-20 13:20:08 PST
(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?
Comment 10 Boris Zbarsky [:bz] 2010-01-20 13:52:41 PST
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.
Comment 11 Boris Zbarsky [:bz] 2010-01-20 13:59:24 PST
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.
Comment 12 timeless 2010-01-20 18:46:51 PST
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.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-20 23:39:06 PST
Timeless: what's your timeline on needing this fix in your consuming project?
Comment 14 timeless 2010-01-25 03:11:21 PST
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.
Comment 15 Jonas Sicking (:sicking) 2010-01-27 02:10:24 PST
*** Bug 533247 has been marked as a duplicate of this bug. ***
Comment 16 Wladimir Palant 2010-01-27 02:42:22 PST
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.
Comment 17 Boris Zbarsky [:bz] 2010-01-27 05:35:50 PST
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 18 Daniel Veditz [:dveditz] 2010-02-05 13:30:18 PST
Comment on attachment 422564 [details] [diff] [review]
Proposed fix

Approved for 1.9.2.2, a=dveditz for release-drivers
Comment 20 Al Billings [:abillings] 2010-02-25 16:22:55 PST
(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?
Comment 21 Wladimir Palant 2010-02-26 04:39:56 PST
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).
Comment 22 Boris Zbarsky [:bz] 2010-02-26 09:28:01 PST
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.
Comment 23 Al Billings [:abillings] 2010-03-15 16:19:50 PDT
(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:1.9.2.2pre) Gecko/20100315 Namoroka/3.6.2pre (.NET CLR 3.5.30729). I'm not seeing anything from /smilies with this build but did with 1.9.2.0.
Comment 24 Daniel Veditz [:dveditz] 2010-03-22 16:39:55 PDT
(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.
Comment 25 Boris Zbarsky [:bz] 2010-03-22 16:45:57 PDT
> 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?
Comment 26 Boris Zbarsky [:bz] 2010-03-22 16:46:50 PDT
Oh, and various extension protocol schemes are really bad news if content script can trigger arbitrary URI loads through them, last I checked.
Comment 27 Daniel Veditz [:dveditz] 2010-03-22 17:30:18 PDT
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).

Note You need to log in before you can comment on or make changes to this bug.