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

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[Eric Foley]

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

http://secunia.com/advisories/19698/

Comment 2

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

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: [e-mail]]]

(that was fixed in bug 292774)

Comment 5

[Eric Foley]

Attachment: Test case added

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

Attachment: testcase (for Windows)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

oops, didn't mean to take this.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So who's producing a file:// URI here?  And WHY?

Comment 10

[:Gavin Sharp [email: [e-mail]]]

(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: [e-mail]]]

Ah, I guess we could just use the no-fixup flag roc added to fix this?

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(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 [:bzbarsky]]

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)]

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 [:bzbarsky]]

> 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)]

*sigh*

Comment 17

[Boris Zbarsky [:bzbarsky]]

Where does the security check in question live?  Gavin, do you know?

Comment 18

[Darin Fisher]

bz, biesi, gavin: can one of you take this bug?  thanks!

Comment 19

[:Gavin Sharp [email: [e-mail]]]

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 [:bzbarsky]]

Attachment: Possible fix

Could someone give that a spin and let me know whether it fixes the bug?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Yes, that does fix the testcase in this bug. Feel free to take this :)

Comment 22

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Neil: the Seamonkey UI may need similar changes to use checkLoadURIStr.

Comment 24

[[e-mail]]

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

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

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)]

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 [:bzbarsky]]

> 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)]

checkLoadURI, since checkLoadURIStr callers are all safe with this patch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Oh, also nsLivemarkLoadListener::IsLinkValid is vulnerable.

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 219194 [details] [diff] [review]
Possible fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 33

[Eric Foley]

This seems like a messy situation, glad to see it is being worked on though

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 35

[Boris Zbarsky [:bzbarsky]]

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]

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 [:bzbarsky]]

No.  This bug is a lot less severe.

Comment 38

[nathaniel]

Secunia rates them as the same, but I guess their rating is kinda skewed.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Secunia's rating of the other bug is totally off, yeah.

Comment 40

[Juha-Matti Laurio]

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

Attachment: 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]

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

new is coming

Comment 43

[Alexander Sack]

Attachment: aviary 1.0.9 2nd

updated version