Last Comment Bug 334341 - [FIX]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 wi...
Status: RESOLVED FIXED
[sg:low] SA19698
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
http://www.gavinsharp.com/tmp/ImageVu...
Depends on: 321589
Blocks: 335334 335335
  Show dependency treegraph
 
Reported: 2006-04-17 06:57 PDT by Eric Foley
Modified: 2006-06-13 15:04 PDT (History)
21 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.9a1+
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case added (914 bytes, application/x-zip)
2006-04-18 08:37 PDT, Eric Foley
no flags Details
testcase (for Windows) (148 bytes, text/html)
2006-04-18 09:13 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
Possible fix (4.38 KB, patch)
2006-04-20 14:28 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
dveditz: superreview+
dveditz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review
Updated to comments (4.41 KB, patch)
2006-04-24 20:24 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
for aviary1.0.9 (2.73 KB, patch)
2006-05-09 02:16 PDT, Alexander Sack
no flags Details | Diff | Review
aviary 1.0.9 2nd (3.20 KB, patch)
2006-06-13 15:02 PDT, Alexander Sack
asac: review? (caillon)
Details | Diff | Review

Description Eric Foley 2006-04-17 06:57:25 PDT
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.
Comment 1 Steve England [:stevee] 2006-04-18 05:28:06 PDT
http://secunia.com/advisories/19698/
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 05:45:50 PDT
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.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 05:52:41 PDT
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.
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 05:53:56 PDT
(that was fixed in bug 292774)
Comment 5 Eric Foley 2006-04-18 08:37:38 PDT
Created attachment 218829 [details]
Test case added
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 09:07:52 PDT
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 .
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 09:13:37 PDT
Created attachment 218832 [details]
testcase (for Windows)
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 09:30:25 PDT
oops, didn't mean to take this.
Comment 9 Boris Zbarsky [:bz] 2006-04-18 09:56:57 PDT
So who's producing a file:// URI here?  And WHY?
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 10:21:12 PDT
(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
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 10:22:52 PDT
Ah, I guess we could just use the no-fixup flag roc added to fix this?
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-18 10:25:30 PDT
(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).
Comment 13 Boris Zbarsky [:bz] 2006-04-18 10:31:16 PDT
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?
Comment 14 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-20 05:24:32 PDT
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...
Comment 15 Boris Zbarsky [:bz] 2006-04-20 07:38:03 PDT
> 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).
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-20 09:24:08 PDT
*sigh*
Comment 17 Boris Zbarsky [:bz] 2006-04-20 10:29:37 PDT
Where does the security check in question live?  Gavin, do you know?
Comment 18 Darin Fisher 2006-04-20 11:54:09 PDT
bz, biesi, gavin: can one of you take this bug?  thanks!
Comment 19 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-20 13:57:06 PDT
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?
Comment 20 Boris Zbarsky [:bz] 2006-04-20 14:28:26 PDT
Created attachment 219194 [details] [diff] [review]
Possible fix

Could someone give that a spin and let me know whether it fixes the bug?
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-04-20 14:51:45 PDT
Yes, that does fix the testcase in this bug. Feel free to take this :)
Comment 22 Boris Zbarsky [:bz] 2006-04-20 15:03:10 PDT
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.
Comment 23 Boris Zbarsky [:bz] 2006-04-20 15:04:56 PDT
Neil: the Seamonkey UI may need similar changes to use checkLoadURIStr.
Comment 24 neil@parkwaycc.co.uk 2006-04-20 15:57:50 PDT
(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]
Comment 25 Daniel Veditz [:dveditz] 2006-04-20 16:00:05 PDT
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 26 Daniel Veditz [:dveditz] 2006-04-20 16:27:08 PDT
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=)
Comment 27 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-21 13:10:40 PDT
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?
Comment 28 Boris Zbarsky [:bz] 2006-04-21 13:56:34 PDT
> did you verify that all other checkLoadURI callers are OK?

checkLoadURI, or checkLoadURIStr?
Comment 29 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-21 14:03:10 PDT
checkLoadURI, since checkLoadURIStr callers are all safe with this patch.
Comment 30 Boris Zbarsky [:bz] 2006-04-21 14:21:06 PDT
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.

Comment 31 Boris Zbarsky [:bz] 2006-04-21 14:21:40 PDT
Oh, also nsLivemarkLoadListener::IsLinkValid is vulnerable.
Comment 32 Daniel Veditz [:dveditz] 2006-04-24 17:08:50 PDT
Comment on attachment 219194 [details] [diff] [review]
Possible fix

approved for 1.8.0 branch, a=dveditz for drivers
Comment 33 Eric Foley 2006-04-24 18:47:40 PDT
This seems like a messy situation, glad to see it is being worked on though
Comment 34 Boris Zbarsky [:bz] 2006-04-24 20:24:32 PDT
Created attachment 219715 [details] [diff] [review]
Updated to comments
Comment 35 Boris Zbarsky [:bz] 2006-04-24 20:34:56 PDT
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.
Comment 36 nathaniel 2006-04-27 18:20:22 PDT
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?
Comment 37 Boris Zbarsky [:bz] 2006-04-27 18:26:49 PDT
No.  This bug is a lot less severe.
Comment 38 nathaniel 2006-04-27 22:59:32 PDT
Secunia rates them as the same, but I guess their rating is kinda skewed.
Comment 39 Boris Zbarsky [:bz] 2006-04-28 08:04:04 PDT
Secunia's rating of the other bug is totally off, yeah.
Comment 40 Juha-Matti Laurio 2006-05-07 23:47:11 PDT
(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 Alexander Sack 2006-05-09 02:16:45 PDT
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?
Comment 42 Alexander Sack 2006-06-13 15:01:57 PDT
Comment on attachment 221432 [details] [diff] [review]
for aviary1.0.9

new is coming
Comment 43 Alexander Sack 2006-06-13 15:02:57 PDT
Created attachment 225483 [details] [diff] [review]
aviary 1.0.9 2nd

updated version

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