The default bug view has changed. See this FAQ.

Can still reach remote moz-icon: files through URL encoding

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Core
ImageLib
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: dveditz, Assigned: Biesinger)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha8
fixed1.8.0.13, fixed1.8.0.14, fixed1.8.1.5, fixed1.8.1.8
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.5 +
blocking1.8.1.8 +
wanted1.8.1.x +
blocking1.8.0.13 +
blocking1.8.0.14 +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] vector for a critical (patched) windows exploit, [need testcase])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.
Flags: blocking1.9?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
(Reporter)

Updated

10 years ago
Whiteboard: [sg:low] vector for a critical (patched) windows exploit
Flags: blocking1.9? → blocking1.9+
(Reporter)

Comment 1

10 years ago
Created attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too
Attachment #271985 - Flags: approval1.8.1.5?
Attachment #271985 - Flags: approval1.8.0.13?
(Reporter)

Updated

10 years ago
Attachment #271985 - Flags: review?(timeless)

Comment 2

10 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?

Comment 3

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

this will work and is good enough for branches.
Attachment #271985 - Flags: review?(timeless) → review+
(Reporter)

Comment 4

10 years ago
fix checked in, will seek retroactive branch approval
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.0.13, fixed1.8.1.5
Resolution: --- → FIXED
(Reporter)

Comment 5

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

seeking retroactive review from bz
Attachment #271985 - Flags: review?(bzbarsky)
Comment on attachment 271985 [details] [diff] [review]
check for escaped UNC paths, too

Probably fine given the limited uses of moz-icon.
Attachment #271985 - Flags: review?(bzbarsky) → review+
(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

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

Comment 9

10 years ago
reopening based on last comment.  please correct if this in not the case.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.8.1.6?
(Reporter)

Updated

10 years ago
Flags: blocking1.8.1.6?
Assignee: dveditz → cbiesinger
Status: REOPENED → NEW
(Reporter)

Comment 10

10 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.
Created attachment 274225 [details] [diff] [review]
pach

still have to test this...
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).
Attachment #274225 - Attachment is obsolete: true
Attachment #274246 - Flags: superreview?(bzbarsky)
Attachment #274246 - Flags: review?(dveditz)
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?
Attachment #274246 - Flags: superreview?(bzbarsky) → superreview+
Then again, I guess on trunk -moz-icon: is already restricted to chrome.  So nevermind.  ;)
Target Milestone: --- → mozilla1.9 M8
(Reporter)

Updated

10 years ago
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.7?
Flags: blocking1.8.1.7+
Flags: blocking1.8.1.6?
Flags: blocking1.8.0.14+
(Reporter)

Comment 15

10 years ago
Comment on attachment 274246 [details] [diff] [review]
patch v2

r=dveditz
Attachment #274246 - Flags: review?(dveditz) → review+
(Reporter)

Comment 16

10 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
Attachment #271985 - Attachment is obsolete: true
Attachment #271985 - Flags: approval1.8.1.5?
Attachment #271985 - Flags: approval1.8.0.13?
(Reporter)

Updated

10 years ago
Attachment #274246 - Flags: approval1.8.1.7?
Attachment #274246 - Flags: approval1.8.0.14?
Er, actually on trunk moz-icon is NOT restricted to chrome.
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
Status: NEW → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
(Reporter)

Comment 19

10 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
Attachment #274246 - Flags: approval1.8.1.7?
Attachment #274246 - Flags: approval1.8.1.7+
Attachment #274246 - Flags: approval1.8.0.14?
Attachment #274246 - Flags: approval1.8.0.14+
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
Keywords: fixed1.8.0.14, fixed1.8.1.7
Christian, could you help us verify this on FF 2008rc2? 
Whiteboard: [sg:low] vector for a critical (patched) windows exploit → [sg:low] vector for a critical (patched) windows exploit, [need testcase]
(Reporter)

Updated

10 years ago
Group: security
Is there a test case for this somewhere?
You need to log in before you can comment on or make changes to this bug.