Daniel Veditz [:dveditz] Reporter
	Description • 17 years ago

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.

	timeless
	Comment 2 • 17 years ago

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?

	timeless
	Comment 3 • 17 years ago

Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

this will work and is good enough for branches.

	Daniel Veditz [:dveditz] Reporter
	Comment 4 • 17 years ago

fix checked in, will seek retroactive branch approval

	Daniel Veditz [:dveditz] Reporter
	Comment 5 • 17 years ago

Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

seeking retroactive review from bz

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 17 years ago

Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

Probably fine given the limited uses of moz-icon.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 7 • 17 years ago

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

	Alexander Sotirov
	Comment 8 • 17 years ago

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.

	chris hofmann
	Comment 9 • 17 years ago

reopening based on last comment.  please correct if this in not the case.

	Daniel Veditz [:dveditz] Reporter
	Comment 10 • 17 years ago

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.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 11 • 17 years ago

still have to test this...

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 12 • 17 years ago

This version actually compiles & works

It now treats UNC paths as malformed URIs (instead of ignoring the UNC part).

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 17 years ago

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?

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 17 years ago

Then again, I guess on trunk -moz-icon: is already restricted to chrome.  So nevermind.  ;)

	Daniel Veditz [:dveditz] Reporter
	Comment 15 • 17 years ago

Comment on attachment 274246 [details] [diff] [review]
patch v2

r=dveditz

	Daniel Veditz [:dveditz] Reporter
	Comment 16 • 17 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 17 years ago

Er, actually on trunk moz-icon is NOT restricted to chrome.

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 18 • 17 years ago

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

	Daniel Veditz [:dveditz] Reporter
	Comment 19 • 17 years ago

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

	Christian :Biesinger (don't email me, ping me on IRC) Assignee
	Comment 20 • 17 years ago

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

	juan becerra [:juanb]
	Comment 21 • 17 years ago

Christian, could you help us verify this on FF 2008rc2? 

	Al Billings [:abillings - ex-MoCo]
	Comment 22 • 17 years ago

Is there a test case for this somewhere?