Bug 460425 (CVE-2009-0356)

[FIX].desktop allows remotely loading "about:config" & "about:plugins" with little user interaction

VERIFIED FIXED in mozilla1.9.1b3

Status

()

Core
Networking: File
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: bz)

Tracking

({verified1.9.0.6, verified1.9.1})

Trunk
mozilla1.9.1b3
x86
Linux
verified1.9.0.6, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.6 +
wanted1.9.0.x +
blocking1.8.1.19 -
wanted1.8.1.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] high/critical with mitigations;)

Attachments

(5 attachments, 3 obsolete attachments)

Created attachment 343527 [details]
perl script - chmod +x ; put on web server ; load ; open with firefox

.desktop files when loaded locally can load "about:config" and "about:plugins" which have chrome privileges.

the remote headers:
Content-type: text/html
Content-Disposition: attachment;filename=v1.desktop

the user interaction:
dialog asking "open with firefox"

somewhat related to Bug 369462
Whiteboard: [sg:investigate]
Uh...  This is exactly what bug 455311 is about.  I suggest testing with a build that has that bug fixed.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 455311
>I suggest testing with a build that has that bug fixed.

you mean the fix isn't checked in?

it works on today's builds.
Hmm.  OK, so links to about:cache are properly blocked, but not about:plugins or about:config, as you note.

The reason for that is that in this case the CheckLoadURI check is happening not on the URI to be loaded but on the channel to be opened, and the channels produced by nsAboutRedirector do not have about: URIs: they have jar:file:// URIs.  So a file:// can link to them.

One obvious way to proceed here is to also check whether we can load the originalURI if it's not the same object as the channel URI.  dveditz, what do you think?
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Except of course the originalURI is the pre-redirect URI.  We could delay setting that until after the security check, I suppose....  Christian?  Would that be ok?
Component: Security → Networking: File
Product: Firefox → Core
QA Contact: firefox → networking.file
note that this works locally:

<object data="a.desktop">

a.desktop: URL=about:plugins

so probably one can get privileged child
> nsAboutRedirector do not have about: URIs: they have jar:file://

i observer that about:plugins redirects to "jar:file://opt/"

when i type in locationbar "javascript:alert(Components.classes)" it passes - probably this means locationbar is lying about the true location.
Locationbar shows the originalURI if the redirect flag is not set.  Permissions are based on the URI (which is not the same thing).
Flags: wanted1.9.0.x+
Flags: wanted1.8.1.x+
Flags: blocking1.9.1?
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.18+
Whiteboard: [sg:investigate] → [sg:moderate] high/critical with mitigations
assigning to bz since he thought his bug 455311 fix would catch this one too.
Assignee: nobody → bzbarsky
It doesn't, in fact.  I need an answer to comment 4 to proceed here.
Note that fixing up http to work per comment 4 is a bit of a royal pain, so I'd really like some feedback on whether it would be acceptable before spending time on it.
Created attachment 344422 [details] [diff] [review]
Unit test (not to be checked in till we open up the bug)
Created attachment 344425 [details] [diff] [review]
Fix as described

This changes the redirect callers and various onChannelRedirect impls.  I consolidated some code while I was here, but I'm still a little taken aback by the fact that we have all three of nsSameOriginChecker::OnChannelRedirect, nsCrossSiteListenerProxy::UpdateChannel, and the CheckMayLoad function in nsXMLHttpRequest.  It really feels like those could be combined somehow.

In any case, once this passes review I'll work on porting to the branches.  Enough of this stuff is different that it'll need to be done from scratch, and I'll have to avoid the more invasive XMLDocument changes I made here and just stick to more copy/paste.
Attachment #344425 - Flags: superreview?(jonas)
Attachment #344425 - Flags: review?(jonas)
Attachment #344425 - Flags: review?(cbiesinger)
another testcase for seamonkey is redirecting to:

addbook://moz-abmdbdirectory/abook.mab?action=print

produces xml parsing error, redirect to addressbook is done.
are chained .desktop redirects broken on purpose, this doesn't work:

a.desktop => a2.desktop => file:///

a2.desktop is displayed as text/plain
hm, bz, will your xml fixes fix the rdf redirect abuse in Bug 414540?
Ugh, I really don't like this. This seems like pushing a lot of complexity onto a lot of people. I suspect people will get this wrong.

Couldn't we change about: to not redirect the way it does? (sorry, a bit too hung over to fully follow what is going on, and in wrong timezone to catch you on irc)
>Couldn't we change about: to not redirect the way it does?

it is not clear if it is just |about:|. |addbook:| is also affected on seamonkey
> Couldn't we change about: to not redirect the way it does?

Not easily, no.  At least not if you want the URI in the url bar to say about:whatever.

> This seems like pushing a lot of complexity onto a lot of people.

That's mostly because we have stupid code cut-n-paste happening (and already have a lot od complexity pushed onto people).  Ideally, we would just have a simple API on the security manager or whatnot (say taking a loading principal, pre-redirect channel, and post-redirect channel) to do these redirect security checks...  Or an equivalent API on nsContentUtils or whatever.  Rule of thumb is that any security-related code should be implemented no more than once if we want to avoid bugs.  :(

> hm, bz, will your xml fixes fix the rdf redirect abuse in Bug 414540?

No.  This patch aims to not actually change behavior except for the case of this particular security check.

> are chained .desktop redirects broken on purpose, this doesn't work:

Works fine for me.  Please feel free to contact me by mail with further questions about this; no need to clutter up this bug.  I suspect it'll get cluttered enough as it is.
this bug is maybe remote without user interaction if the user has chosen to always open certain file types with external program without asking (checkbox in a dialog).

i chose to always open application/pdf with evince and iframe served "/tmp/pdf.desktop" without interaction (evince whined). for some reason "text/html" always asks on linux.
under the assumption that the user opens .pdf without asking with evince, this is exploitable this way:

pdf.desktop is served to /tmp/pdf.desktop and is valid .desktop
a.html => /tmp/a.html referencing pdf.desktop
/tmp is the default location at least on ubuntu

a real pdf with \TeX hyperlink to file:///tmp/a.html is opened in evince.
if the user clicks on the \TeX hyperlink, a.html can read local files.

can't care less about adobe pdfs.

the relevant \TeX is:
\usepackage{hyperref} 
\href{file:///tmp/a.html}{Click me baby}
Because of windows we consider ".url" a dangerous extension, which means we change it to something else if we get a temp file like that. Should we do the same with .desktop as a stopgap if we can't fix this bug in 1.9.0.4?
Probably not a bad idea...
>we consider ".url" a dangerous extension, which means we
>change it to something else if we get a temp file like that.

what about saltifying all temp files passed to external apps?
i.e. replacing file.ext to file.ext.SALT
So for what it's worth i'm fine with landing something like this on the branch. But I'd prefer that we tried to do a cleaner fix for trunk where we'll have to maintain the code longer.
So... do we care if about:plugins leads to an ugly URL?
i don't care about the beauty of URIs. as i wrote it is unclear are about:plugins and about:config the only instances that must be made ugly.
> what about saltifying all temp files passed to external apps?

Please go read some of the existing bugs about helper app temp files.  In particular, this would make a lot of helper apps not work.

> But I'd prefer that we tried to do a cleaner fix for trunk

I'm fine with discussing necko architecture and API changes that might make this cleaner on trunk, but I'd prefer we do that after we get a fix landed on trunk, tested, and then ported to branches.  I'm happy to file a followup bug to discuss trunk improvements...

I should note that I did think about this a good bit and this is the cleanest fix I can come up with so far that doesn't involve significant rearchitecture of parts of necko and widespread changes to necko consumers.
Boris, how about that stopgap? If the current solution here isn't going to be ready asap, we need to take the stopgap now and do the Correct Branch Fix(tm) in the next release.
The stopgap will help with the "automatic" aspect of loading about:config or about:plugins, but it doesn't help if the user opens a file he saves...

I'm happy to do a separate bug for the stopgap (which is a good idea) tomorrow; I need to catch a plane in 2 hours here.
Boris, please file the stopgap bug and let's mark it blocking 1.9.0.4 (meaning land a fix asap). We'll push this bug to 1.9.0.5.
Flags: blocking1.9.0.4+ → blocking1.9.0.5+
OK, sicking and I talked about this at length today and we'll go with the plane from comment 27, with a followup bug to centralize changes on trunk.

I filed bug 462034 on treating .desktop files as executable.
Flags: blocking1.8.1.18+ → blocking1.8.1.19+
Flags: blocking1.9.1? → blocking1.9.1+
Summary: .desktop allows remotely loading "about:config" & "about:plugins" with little user interaction → [FIX].desktop allows remotely loading "about:config" & "about:plugins" with little user interaction

Comment 32

9 years ago
ReadURLFile seems to be not implemented for XP_UNIX on 1.8 branches. is there anything else here that makes this applicable there?
according to my tests 1.8 branch ignores .desktop files...
We need this fix on 1.8 branch because .url files _are_ supported there on Windows.
is windows affected by remote extension forcing ? (someone wrote it renames dangerous files)
I'm not worried about the remote thing, but there is still a local issue on Windows.
Whiteboard: [sg:moderate] high/critical with mitigations → [sg:moderate] high/critical with mitigations [needs r/sr biesi/sicking]
in theory this may exploitable.

according to grep some parts of chrome do stuff with top.STUFF. if the chrome is loaded in object/iframe |top| is untrusted.
Boris, any update on the "real" version of this?
Or rather, are we still just waiting for reviews here? Jonas?
Yep.  Still waiting on reviews.  I sent mail to Jonas and Christian about it 2008-11-10; no responses so far.
Comment on attachment 344425 [details] [diff] [review]
Fix as described

r=biesi (I looked at the necko changes & the security manager ones)
Attachment #344425 - Flags: review?(cbiesinger) → review+
Whiteboard: [sg:moderate] high/critical with mitigations [needs r/sr biesi/sicking] → [sg:moderate] high/critical with mitigations [needs r/sr sicking]
Attachment #344425 - Flags: superreview?(jonas)
Attachment #344425 - Flags: superreview+
Attachment #344425 - Flags: review?(jonas)
Attachment #344425 - Flags: review+
Whiteboard: [sg:moderate] high/critical with mitigations [needs r/sr sicking] → [sg:moderate] high/critical with mitigations
We're going to push this to the next release (1.9.0.6). But it really needs to make it then, so this needs to hard block 1.9.1.
Flags: blocking1.9.0.6+
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19-
Flags: blocking1.8.1.19+
Target Milestone: --- → mozilla1.9.1
Created attachment 350052 [details] [diff] [review]
mq diff
Attachment #344425 - Attachment is obsolete: true
Pushed http://hg.mozlla.org/mozilla-central/rev/5d1fbada8589

I'll work on the branch merge (at least for 1.9) and filing a trunk followup to clean this up.  Need to land the test once we open up the bug.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Make that http://hg.mozilla.org/mozilla-central/rev/5d1fbada8589
I had to also push http://hg.mozilla.org/mozilla-central/rev/006236d9a610 to fix a test failure (didn't catch it locall, because the test is a no-op on mac  :().
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
Filed bug 468572 as a followup.
Created attachment 352061 [details] [diff] [review]
1.9 branch backport

It's almost tempting to backport the same origin checker, but let's start with this.

Jonas, let me know if you want to review this, ok?  I think it was straightforward enough that we can just take it, but I did end up having to touch more callsites.
Attachment #352061 - Flags: approval1.9.0.6?
Keywords: fixed1.9.1
Attachment #352061 - Flags: review?(jonas)
Whiteboard: [sg:moderate] high/critical with mitigations → [sg:moderate] high/critical with mitigations; needs r=sicking
Whiteboard: [sg:moderate] high/critical with mitigations; needs r=sicking → [sg:moderate][needs branch r=sicking] high/critical with mitigations;
Jonas, can you please review this so we can take it on 1.9.0?
Comment on attachment 352061 [details] [diff] [review]
1.9 branch backport

rs=me, didn't look, but the original patch was simple and sounded like the porting was too
Attachment #352061 - Flags: review?(jonas)
Whiteboard: [sg:moderate][needs branch r=sicking] high/critical with mitigations; → [sg:moderate] high/critical with mitigations;
Comment on attachment 352061 [details] [diff] [review]
1.9 branch backport

Approved for 1.9.0.6, a=dveditz for release-drivers.
Attachment #352061 - Flags: approval1.9.0.6? → approval1.9.0.6+
Fixed on 1.9.0.6.
Keywords: fixed1.9.0.6
Verified for 1.9.0.6 with  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.6pre) Gecko/2009011404 GranParadiso/3.0.6pre. 

Verified for 1.9.1 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20090114 Shiretoko/3.1b3pre.
Keywords: fixed1.9.0.6, fixed1.9.1 → verified1.9.0.6, verified1.9.1

Updated

9 years ago
Keywords: verified1.9.1
sorry for the inconvenience.  i meant to detect any bugs before the branch merge that were still RESO fixed, but had a verified1.9.1 keyword next to them.
Keywords: verified1.9.1
Verified for trunk with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre.
Status: RESOLVED → VERIFIED

Comment 56

8 years ago
Created attachment 358897 [details] [diff] [review]
1.8 backport

Comment 57

8 years ago
(In reply to comment #56)
> Created an attachment (id=358897) [details]
> 1.8 backport

-    do_check_eq(newChan.originalURI.spec, this._origURI.spec);
-    do_check_eq(newChan.originalURI, this._origURI);
+    do_check_eq(newChan.originalURI.spec, this._newURI.spec);
+    do_check_eq(newChan.originalURI, newChan.URI);

Isn't this inconsistent (newChan vs. this._newURI)?

Also, the channels derived from nsBaseChannel on 1.9 probably need manual attention for 1.8.

Comment 58

8 years ago
(In reply to comment #57)
> (In reply to comment #56)
> > Created an attachment (id=358897) [details] [details]
> > 1.8 backport
> 
> -    do_check_eq(newChan.originalURI.spec, this._origURI.spec);
> -    do_check_eq(newChan.originalURI, this._origURI);
> +    do_check_eq(newChan.originalURI.spec, this._newURI.spec);
> +    do_check_eq(newChan.originalURI, newChan.URI);
> 
> Isn't this inconsistent (newChan vs. this._newURI)?

ignore this. looked at the wrong attachment. this comes from 1.9 branch

Comment 59

8 years ago
Created attachment 359033 [details] [diff] [review]
1.8 with nsFileChannel::HandleRedirect

This should address it
Attachment #358897 - Attachment is obsolete: true
> Also, the channels derived from nsBaseChannel on 1.9 probably need manual
> attention for 1.8.

While true, of those only nsFileChannel does redirects as I recall.
If we land this on 1.8 branch, we should back out the bug 462034 hack we did there.
asac: do you want this to block 1.8.1.next?
Flags: blocking1.8.1.next?
(I'm the new Firefox maintainer for SLED at Novell)

This bug is marked as fixed but still have a pending unreviewed patch for 1.8.1.

My biggest concern is that the workaround for bug 462034 caused a regression for our SLED 10 customers since 2.0.0.18, and that in order to solve it, I had to revert the patch (at least with a test release).

I would love to just add a patch to fix this bug and revert the other one.

Thanks.
Hubert: the fixed status refers to the development trunk. Other releases are tracked through blocking flags and fixed/verified keywords (as you see for the 1.9.1 and 1.9.0 branches). It's a horrid way to do it, yes. Maybe one day we'll have bug 55970 fixed.
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Comment on attachment 359033 [details] [diff] [review]
1.8 with nsFileChannel::HandleRedirect

Boris, can you give this a quick glance and check that the backport hasn't tripped some hidden 1.8-branch landmines?

Hubert: have you tried this patch to see if it solves the earlier breakage?
Attachment #359033 - Flags: review?(bzbarsky)
The breakage is caused by the patch for bug 462034. But since this bug should fix the security issues that bug 462034 works around I wanted to make sure I could revert the other one and add this one without security concerns. For now I'm shy to revert bug 462034.

I'll give a shot at the patch.
I applied the patch from attachment 359033 [details] [diff] [review]. I get the following error when building.

ccache g++ -o nsFileChannel.o -c  -DMOZILLA_INTERNAL_API -DOSTYPE=\"Linux2.6.27\" -DOSARCH=\"Linux\" -DBUILD_ID=0000000000 -DIMPL_NS_NET -I./../../../base/src -I../../../../xpcom/ds  -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/mimetype -I../../../../dist/include/pref -I../../../../dist/include/uconv -I../../../../dist/include/caps -I../../../../dist/include/xpconnect -I../../../../dist/include/js -I../../../../dist/include/necko -I../../../../dist/include -I/usr/include/nspr4    -I../../../../dist/sdk/include       -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -g -O0   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/nsFileChannel.pp nsFileChannel.cpp
In file included from ../../../../dist/include/string/nsStringIterator.h:44,
                 from ../../../../dist/include/string/nsAString.h:48,
                 from ../../../../dist/include/string/nsSubstring.h:44,
                 from ../../../../dist/include/string/nsString.h:45,
                 from nsFileChannel.h:53,
                 from nsFileChannel.cpp:40:
../../../../dist/include/string/nsCharTraits.h: In static member function ‘static PRUnichar nsCharTraits<short unsigned int>::ASCIIToLower(PRUnichar)’:
../../../../dist/include/string/nsCharTraits.h:280: warning: conversion to ‘PRUnichar’ from ‘int’ may alter its value
../../../../dist/include/string/nsCharTraits.h: In static member function ‘static char nsCharTraits<char>::ASCIIToLower(char)’:
../../../../dist/include/string/nsCharTraits.h:535: warning: conversion to ‘char’ from ‘int’ may alter its value
In file included from ../../../../dist/include/string/nsDependentSubstring.h:49,
                 from ../../../../dist/include/string/nsString.h:49,
                 from nsFileChannel.h:53,
                 from nsFileChannel.cpp:40:
../../../../dist/include/string/nsTDependentSubstring.h: In constructor ‘nsDependentSubstring::nsDependentSubstring(const PRUnichar*, const PRUnichar*)’:
../../../../dist/include/string/nsTDependentSubstring.h:74: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
../../../../dist/include/string/nsTDependentSubstring.h: In constructor ‘nsDependentSubstring::nsDependentSubstring(const nsReadingIterator<short unsigned int>&, const nsReadingIterator<short unsigned int>&)’:
../../../../dist/include/string/nsTDependentSubstring.h:77: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
In file included from ../../../../dist/include/string/nsDependentSubstring.h:54,
                 from ../../../../dist/include/string/nsString.h:49,
                 from nsFileChannel.h:53,
                 from nsFileChannel.cpp:40:
../../../../dist/include/string/nsTDependentSubstring.h: In constructor ‘nsDependentCSubstring::nsDependentCSubstring(const char*, const char*)’:
../../../../dist/include/string/nsTDependentSubstring.h:74: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
../../../../dist/include/string/nsTDependentSubstring.h: In constructor ‘nsDependentCSubstring::nsDependentCSubstring(const nsReadingIterator<char>&, const nsReadingIterator<char>&)’:
../../../../dist/include/string/nsTDependentSubstring.h:77: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
In file included from ../../../../dist/include/string/nsString.h:75,
                 from nsFileChannel.h:53,
                 from nsFileChannel.cpp:40:
../../../../dist/include/string/nsTString.h: In constructor ‘nsFixedString::nsFixedString(PRUnichar*, PRUint32)’:
../../../../dist/include/string/nsTString.h:464: warning: conversion to ‘PRUint32’ from ‘size_t’ may alter its value
In file included from ../../../../dist/include/string/nsString.h:80,
                 from nsFileChannel.h:53,
                 from nsFileChannel.cpp:40:
../../../../dist/include/string/nsTString.h: In constructor ‘nsFixedCString::nsFixedCString(char*, PRUint32)’:
../../../../dist/include/string/nsTString.h:464: warning: conversion to ‘PRUint32’ from ‘size_t’ may alter its value
In file included from ../../../../dist/include/string/nsDependentString.h:53,
                 from ../../../../dist/include/string/nsString.h:203,
                 from nsFileChannel.h:53,
                 from nsFileChannel.cpp:40:
../../../../dist/include/string/nsTDependentString.h: In constructor ‘nsDependentString::nsDependentString(const PRUnichar*, const PRUnichar*)’:
../../../../dist/include/string/nsTDependentString.h:76: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
../../../../dist/include/string/nsTDependentString.h: In constructor ‘nsDependentString::nsDependentString(const PRUnichar*)’:
../../../../dist/include/string/nsTDependentString.h:89: warning: conversion to ‘PRUint32’ from ‘size_t’ may alter its value
../../../../dist/include/string/nsTDependentString.h: In member function ‘void nsDependentString::Rebind(const PRUnichar*)’:
../../../../dist/include/string/nsTDependentString.h:117: warning: conversion to ‘PRUint32’ from ‘size_t’ may alter its value
../../../../dist/include/string/nsTDependentString.h: In member function ‘void nsDependentString::Rebind(const PRUnichar*, const PRUnichar*)’:
../../../../dist/include/string/nsTDependentString.h:124: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
In file included from ../../../../dist/include/string/nsDependentString.h:58,
                 from ../../../../dist/include/string/nsString.h:203,
                 from nsFileChannel.h:53,
                 from nsFileChannel.cpp:40:
../../../../dist/include/string/nsTDependentString.h: In constructor ‘nsDependentCString::nsDependentCString(const char*, const char*)’:
../../../../dist/include/string/nsTDependentString.h:76: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
../../../../dist/include/string/nsTDependentString.h: In constructor ‘nsDependentCString::nsDependentCString(const char*)’:
../../../../dist/include/string/nsTDependentString.h:89: warning: conversion to ‘PRUint32’ from ‘size_t’ may alter its value
../../../../dist/include/string/nsTDependentString.h: In member function ‘void nsDependentCString::Rebind(const char*)’:
../../../../dist/include/string/nsTDependentString.h:117: warning: conversion to ‘PRUint32’ from ‘size_t’ may alter its value
../../../../dist/include/string/nsTDependentString.h: In member function ‘void nsDependentCString::Rebind(const char*, const char*)’:
../../../../dist/include/string/nsTDependentString.h:124: warning: conversion to ‘PRUint32’ from ‘long int’ may alter its value
nsFileChannel.cpp: In member function ‘void nsFileChannel::HandleRedirect(nsIChannel*)’:
nsFileChannel.cpp:127: error: ‘OriginalURI’ was not declared in this scope
nsFileChannel.cpp: In member function ‘nsresult nsFileChannel::EnsureStream()’:
nsFileChannel.cpp:192: warning: conversion to ‘PRPackedBool’ from ‘PRBool’ may alter its value
gmake[5]: *** [nsFileChannel.o] Error 1


Looks like a typo 
+                   newChannel->SetOriginalURI(OriginalURI());
Flags: blocking1.8.1.next+ → blocking1.8.1.next?

Comment 68

8 years ago
Created attachment 359735 [details] [diff] [review]
1.8 with build fixes and 462034 backout

fixed build failure; also include backout for 462034 as requested by boris. I would like to get this landed for 1.8.1.21 still, so a quick look would be appreciated. Thanks!
Attachment #359033 - Attachment is obsolete: true
Attachment #359735 - Flags: review?(bzbarsky)
Attachment #359033 - Flags: review?(bzbarsky)
Comment on attachment 359735 [details] [diff] [review]
1.8 with build fixes and 462034 backout

On 1.8 branch, security manager doesn't do its CheckLoadURI checks on redirects via the global redirect observer.  See the explicit CheckLoadURI call in nsHttpChannel.  You need a similar one in nsFileChannel.

As in, this patch doesn't actually fix this bug, as far as I can tell.  Was it tested?
Attachment #359735 - Flags: review?(bzbarsky) → review-
Flags: blocking1.8.1.next? → blocking1.8.1.next+
Group: core-security
Alias: CVE-2009-0356

Comment 70

8 years ago
AFAIK, the .desktop file support is not available on 1.8 branches ... so I assumed the test case wont work at all ... hence the no testing, sorry. Are there other ways to trigger this issue even without .desktop files on 1.8 branch?
I thought someone was working on backporting the .desktop support.

If not, you could test on Windows by disabling the ".url is executable" bit.
Not blocking 1.8 branch until .desktop support lands on that branch, taking off the list for now.
Flags: blocking1.8.1.next+

Comment 73

8 years ago
A patch for .desktop file support for Linux is in Bug 480907.

Comment 74

8 years ago
Is the fix really needed for 1.8?

Patch for Bug 455311 seems to be enough, nsScriptSecurityManager::CheckLoadURIWithPrincipal() refuses to load about:* scheme from .desktop file (1.8 code uses the scheme lookup table).
Blocks: 480907
Bug 455311 might be enough if the redirect check is done on URIs, not channels, and if the right URI is used.
You need to log in before you can comment on or make changes to this bug.