540642 - (CVE-2010-0168) nsDocument::MaybePreLoadImage doesn't play nicely with nsIContentPolicy

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[timeless]

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) (inactive)]

Attachment: Proposed fix

This makes the image preloading stuff follow the script preloading stuff in terms of content policy calls.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Oh, and 1.9.1 is unaffected, looks like.  Good.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

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) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/02135cbe7432

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

(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 [:bzbarsky]]

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 [:bzbarsky]]

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]

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]]

Timeless: what's your timeline on needing this fix in your consuming project?

Comment 14

[timeless]

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 16

[Wladimir Palant]

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 [:bzbarsky]]

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]]

Comment on attachment 422564 [details] [diff] [review]
Proposed fix

Approved for 1.9.2.2, a=dveditz for release-drivers

Comment 19

[timeless]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9acd0c5d78b4

Comment 20

[Al Billings [:abillings - ex-MoCo]]

(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]

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 [:bzbarsky]]

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 - ex-MoCo]]

(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]]

(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 [:bzbarsky]]

> 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 [:bzbarsky]]

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]]

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).