The default bug view has changed. See this FAQ.

[FIX]Using image tags with a non image file, and selected view image, file will still load up, allowing access to system resources

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Security
P2
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Eric Foley, Assigned: bz)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
mozilla1.9alpha1
fixed1.8.0.4, fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.9a1 +
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low] SA19698, URL)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.2) Gecko/20060308 Firefox/1.5.0.2

A bug has been discovered by myself(TeamOverload) in Firefox
1.5.0.2(other versions are probably affected too).  Through a
specially crafted webpage you can have any file be disguised as an
image.  If you then right click-view image, the file will attempt to
download or just run if it is on the bypass list.  Some extensions
such as .wma are defaulted like that and a malformed wma can be loaded
just by going to view image.  Other websites can be loaded this way as
well.

Reproducible: Always

Steps to Reproduce:
1.Download attached archive that causes problem
2.Launch web page, and right click and choose show image on both
3.First image should open WindowsMediaPlayer and the second should go to a different web page.

Actual Results:  
Both WMP and the alternate web page opened.

Expected Results:  
The browser should deny access to both of the corrupt images such as done in Internet Explorer.

This allows system access and has been confirmed by Secunia as they are currently creating an advisory.
(Reporter)

Updated

11 years ago
Severity: major → critical
http://secunia.com/advisories/19698/
Where is the attached archive? The URL in the URL field doesn't work outside of your specific gmail session, I'm guessing.

From looking at the secunia description, it seems as though this bug is basically asking for "View Image" to be disabled on broken images pointing to file:// URIs.

I guess this is similar to <a href="file://...">click me!</a>, which is currently blocked.
Severity: critical → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → 1.5.0.x Branch
Actually, "View Image" to a local file (regardless of type) is blocked in 1.5.0.2, as far as I can tell, so now I'm not sure what this bug is about. Testcase needed.
(that was fixed in bug 292774)
(Reporter)

Comment 5

11 years ago
Created attachment 218829 [details]
Test case added
Attachment #218829 - Attachment mime type: text/html → application/x-zip
Ah, I was using file:// URIs to test. This is a similar issue to bug 321589. The uri created from newURI("C:\...") makes a nsIURI with scheme == "c", which hits http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/caps/src/nsScriptSecurityManager.cpp&rev=1.266.2.7.2.2#1379 .
Created attachment 218832 [details]
testcase (for Windows)
Attachment #218829 - Attachment is obsolete: true
Attachment #218832 - Attachment description: testcase → testcase (for Windows)
oops, didn't mean to take this.
Assignee: gavin.sharp → nobody
Depends on: 321589
So who's producing a file:// URI here?  And WHY?
(In reply to comment #9)
> So who's producing a file:// URI here?  And WHY?

nsDocShell::LoadURI calls CreateFixupURI which does the munging to make it a file:// URI (this is the same codepath as entering a URL in the location bar).

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/docshell/base/nsDefaultURIFixup.cpp&rev=1.46#190
Ah, I guess we could just use the no-fixup flag roc added to fix this?
(In reply to comment #11)
> Ah, I guess we could just use the no-fixup flag roc added to fix this?

Nevermind, that's a differet "fixup" (keyword).
Er... Yeah.  That's bad.  We really don't want to be doing fixup in this case.  Is there a flag to disable it altogether?  Or an nsIURI version of this method chrome could call?  Or something?
Flags: blocking1.9a1?
Flags: blocking1.8.0.3?
Flags: blocking-firefox2?
Whiteboard: [sg:low] SA19698
I do believe that an nsIURI version already exists: just do what this code does: http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#2828

But this could also be fixed by never calling urifixup if NS_NewURI succeeded...
> I do believe that an nsIURI version already exists: just do what this code

That's on nsIDocShell.  I really don't want us adding more nsIDocShell consumers...

> But this could also be fixed by never calling urifixup if NS_NewURI
> succeeded...

Then typing a file path in the URL bar wouldn't work on Windows (which is presumably the reason we have this fixup in the first place).
*sigh*
Where does the security check in question live?  Gavin, do you know?
Flags: blocking1.9a1?
Flags: blocking1.9a1+
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3+
Flags: blocking-firefox2?
Flags: blocking-firefox2+

Comment 18

11 years ago
bz, biesi, gavin: can one of you take this bug?  thanks!
The security check is here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.js&rev=1.611#4658
which is a wrapper around checkLoadURI:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/contentAreaUtils.js&rev=1.83#118

I guess I can just change these two callers (View Image and View Background Image) to do the fixup themselves before the security check. That fixes this bug, but it would probably be a good idea to check that other callers of loadURI after checkLoadURI don't have similar problems.

As a more general solution, maybe we could add something like a LOAD_FLAGS_SKIP_URI_FIXUP flag on nsIWebNavigation, for loadURI callers that don't want fixup?
Assignee: nobody → gavin.sharp
Created attachment 219194 [details] [diff] [review]
Possible fix

Could someone give that a spin and let me know whether it fixes the bug?
Yes, that does fix the testcase in this bug. Feel free to take this :)
Comment on attachment 219194 [details] [diff] [review]
Possible fix

I'm not so happy with this in general, but until we start using URIs end to end, this is the best we can do.  :(  Luckily, nothing perf-sensitive calls this code.
Attachment #219194 - Flags: superreview?(dveditz)
Attachment #219194 - Flags: review?(cbiesinger)
Neil: the Seamonkey UI may need similar changes to use checkLoadURIStr.
Assignee: gavin.sharp → bzbarsky
Component: Security → Security
Flags: review?(cbiesinger)
Flags: blocking-firefox2+
Priority: -- → P2
Product: Firefox → Core
Version: 1.5.0.x Branch → Trunk
Attachment #219194 - Flags: approval1.8.0.3?
Attachment #219194 - Flags: approval-branch-1.8.1?(dveditz)
Attachment #219194 - Flags: review?(cbiesinger)
Summary: Using image tags with a non image file, and selected view image, file will still load up, allowing access to system resources → [FIX]Using image tags with a non image file, and selected view image, file will still load up, allowing access to system resources
Target Milestone: --- → mozilla1.9alpha

Comment 24

11 years ago
(In reply to comment #23)
>Neil: the Seamonkey UI may need similar changes to use checkLoadURIStr.
As it happens it already does ;-)
[although I still need to land the javascript blocker]
checkLoadURIStr is used by the plugin finder. Trying to abuse it in a similar way results in the message "protocol (C) is not associated with any program"

data:text/html,<embed src="http://www.mozilla.org/" type="application/x-xoxo" height=100 width=100 pluginspage="c:\windows\media\wingin.wav"></embed>

Click on the puzzle piece, then on the "Manual" button.

If a "file" url is used then the manual button does nothing (exception appears on JS Console). This fix ought to make the C: case work there as well.
Comment on attachment 219194 [details] [diff] [review]
Possible fix

This is pretty ugly having to test all the different fixups.

sr=dveditz, OK for the 1.8 branch (pending r=)
Attachment #219194 - Flags: superreview?(dveditz)
Attachment #219194 - Flags: superreview+
Attachment #219194 - Flags: approval-branch-1.8.1?(dveditz)
Attachment #219194 - Flags: approval-branch-1.8.1+
Comment on attachment 219194 [details] [diff] [review]
Possible fix

+    nsCOMPtr<nsIURIFixup> fixup = do_GetService(NS_URIFIXUP_CONTRACTID);
+    if (!fixup) {
+        return rv;

it would probably be clearer to make this "return NS_OK"

+    PRUint32 flags[] = {

make this const please

+        0,

should this be FIXUP_FLAGS_NONE?

+    for (PRInt32 i = 0; i < NS_ARRAY_LENGTH(flags); ++i) {

why signed?


did you verify that all other checkLoadURI callers are OK?
Attachment #219194 - Flags: review?(cbiesinger) → review+
> did you verify that all other checkLoadURI callers are OK?

checkLoadURI, or checkLoadURIStr?
checkLoadURI, since checkLoadURIStr callers are all safe with this patch.
Ah.  Well, the following look vulnerable:

nsFeedLoadListener::IsLinkValid
<method name="onLinkAdded"> in tabbrowser.xml (both versions; this also calls
   content policy with the URI...)
<method name="open"> in text.xml (toolkit only)
onDrop function in browser.js (two different onDrop functions, actually)

I _think_ the remaining callers are ok.  But maybe someone should audit.  And also CheckLoadURIWithPrincipal, to be safe.

Oh, also nsLivemarkLoadListener::IsLinkValid is vulnerable.
Comment on attachment 219194 [details] [diff] [review]
Possible fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #219194 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Reporter)

Comment 33

11 years ago
This seems like a messy situation, glad to see it is being worked on though
Status: NEW → ASSIGNED
Created attachment 219715 [details] [diff] [review]
Updated to comments
Attachment #219194 - Attachment is obsolete: true
Blocks: 335334
Blocks: 335335
Checked in, trunk and branches.  I filed followup bugs for the remaining issues; I'm not likely to be able to fix them in the next few days (not with proper testing and all), and we want them for 1.8.0.3, imo.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.3, fixed1.8.1
Resolution: --- → FIXED

Comment 36

11 years ago
Should this be included in the 1.8.0.3 patch in addition to the DoS exploit report because this bug was publically reported also?
No.  This bug is a lot less severe.

Comment 38

11 years ago
Secunia rates them as the same, but I guess their rating is kinda skewed.
Secunia's rating of the other bug is totally off, yeah.

Comment 40

11 years ago
(In reply to comment #7)
> Created an attachment (id=218832) [edit]
> testcase
> 

Testcase page http://www.gavinsharp.com/tmp/ImageVuln.html 
updated to share background information after the following BugTraq posting:
http://www.securityfocus.com/archive/1/433138/30/0/threaded

For some reason title 'code execution' used in that mailing list posting.
Non-Bugzilla version of the testcase URL seems to be 
http://www.gavinsharp.com/tmp/ImageVuln2.html 
now.

Comment 41

11 years ago
Created attachment 221432 [details] [diff] [review]
for aviary1.0.9

made attached patch apply for aviary1.0.9. Any comments before I request review? Maybe there is more for this issue in #335334, #335335 or #321589 - which I can't see?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?

Updated

11 years ago
Attachment #221432 - Flags: review?(caillon)

Comment 42

11 years ago
Comment on attachment 221432 [details] [diff] [review]
for aviary1.0.9

new is coming
Attachment #221432 - Attachment is obsolete: true
Attachment #221432 - Flags: review?(caillon)

Comment 43

11 years ago
Created attachment 225483 [details] [diff] [review]
aviary 1.0.9 2nd

updated version

Updated

11 years ago
Attachment #225483 - Flags: review?(caillon)
You need to log in before you can comment on or make changes to this bug.