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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

defect
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: dveditz, Assigned: Biesinger)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha8
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

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

Updated

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

Comment 1

12 years ago
Attachment #271985 - Flags: approval1.8.1.5?
Attachment #271985 - Flags: approval1.8.0.13?
(Reporter)

Updated

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

Comment 2

12 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

12 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

12 years ago
fix checked in, will seek retroactive branch approval
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 5

12 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

12 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

12 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

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

Comment 10

12 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.
Posted patch pach (obsolete) — Splinter Review
still have to test this...
Posted patch patch v2Splinter Review
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

12 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

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

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

Comment 16

12 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

12 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: 12 years ago12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
(Reporter)

Comment 19

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

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