460425 - (CVE-2009-0356) [FIX].desktop allows remotely loading "about:config" & "about:plugins" with little user interaction

Status: VERIFIED FIXED

(Core :: Networking: File, defect)

Description

[georgi - hopefully not receiving bugspam]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Uh...  This is exactly what bug 455311 is about.  I suggest testing with a build that has that bug fixed.

Comment 2

[georgi - hopefully not receiving bugspam]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[georgi - hopefully not receiving bugspam]

note that this works locally:

<object data="a.desktop">

a.desktop: URL=about:plugins

so probably one can get privileged child

Comment 6

[georgi - hopefully not receiving bugspam]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Locationbar shows the originalURI if the redirect flag is not set.  Permissions are based on the URI (which is not the same thing).

Comment 8

[Daniel Veditz [:dveditz]]

assigning to bz since he thought his bug 455311 fix would catch this one too.

Comment 9

[Boris Zbarsky [:bzbarsky]]

It doesn't, in fact.  I need an answer to comment 4 to proceed here.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Unit test (not to be checked in till we open up the bug)

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[georgi - hopefully not receiving bugspam]

another testcase for seamonkey is redirecting to:

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

produces xml parsing error, redirect to addressbook is done.

Comment 14

[georgi - hopefully not receiving bugspam]

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

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

a2.desktop is displayed as text/plain

Comment 15

[georgi - hopefully not receiving bugspam]

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

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)

Comment 17

[georgi - hopefully not receiving bugspam]

>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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

Comment 19

[georgi - hopefully not receiving bugspam]

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.

Comment 20

[georgi - hopefully not receiving bugspam]

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}

Comment 21

[Daniel Veditz [:dveditz]]

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?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Probably not a bad idea...

Comment 23

[georgi - hopefully not receiving bugspam]

>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

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 25

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

So... do we care if about:plugins leads to an ugly URL?

Comment 26

[georgi - hopefully not receiving bugspam]

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.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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

Comment 28

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

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

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

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.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Alexander Sack]

ReadURLFile seems to be not implemented for XP_UNIX on 1.8 branches. is there anything else here that makes this applicable there?

Comment 33

[georgi - hopefully not receiving bugspam]

according to my tests 1.8 branch ignores .desktop files...

Comment 34

[Boris Zbarsky [:bzbarsky]]

We need this fix on 1.8 branch because .url files _are_ supported there on Windows.

Comment 35

[georgi - hopefully not receiving bugspam]

is windows affected by remote extension forcing ? (someone wrote it renames dangerous files)

Comment 36

[Boris Zbarsky [:bzbarsky]]

I'm not worried about the remote thing, but there is still a local issue on Windows.

Comment 37

[georgi - hopefully not receiving bugspam]

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.

Comment 38

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

Boris, any update on the "real" version of this?

Comment 39

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

Or rather, are we still just waiting for reviews here? Jonas?

Comment 40

[Boris Zbarsky [:bzbarsky]]

Yep.  Still waiting on reviews.  I sent mail to Jonas and Christian about it 2008-11-10; no responses so far.

Comment 41

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

Comment on attachment 344425 [details] [diff] [review]
Fix as described

r=biesi (I looked at the necko changes & the security manager ones)

Comment 42

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

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.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Attachment: mq diff

Comment 44

[Boris Zbarsky [:bzbarsky]]

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.

Comment 45

[Boris Zbarsky [:bzbarsky]]

Make that http://hg.mozilla.org/mozilla-central/rev/5d1fbada8589

Comment 46

[Boris Zbarsky [:bzbarsky]]

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  :().

Comment 47

[Boris Zbarsky [:bzbarsky]]

Filed bug 468572 as a followup.

Comment 48

[Boris Zbarsky [:bzbarsky]]

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

Comment 49

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

Jonas, can you please review this so we can take it on 1.9.0?

Comment 50

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 51

[Daniel Veditz [:dveditz]]

Comment on attachment 352061 [details] [diff] [review]
1.9 branch backport

Approved for 1.9.0.6, a=dveditz for release-drivers.

Comment 52

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.9.0.6.

Comment 53

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

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.

Comment 54

[Tony Chung [:tchung]]

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.

Comment 55

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

Verified for trunk with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090115 Minefield/3.2a1pre.

Comment 56

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

Attachment: 1.8 backport

Comment 57

[Alexander Sack]

(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

[Alexander Sack]

(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

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

Attachment: 1.8 with nsFileChannel::HandleRedirect

This should address it

Comment 60

[Boris Zbarsky [:bzbarsky]]

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

Comment 61

[Boris Zbarsky [:bzbarsky]]

If we land this on 1.8 branch, we should back out the bug 462034 hack we did there.

Comment 62

[Daniel Veditz [:dveditz]]

asac: do you want this to block 1.8.1.next?

Comment 63

[Hubert Figuiere [:hub]]

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

Comment 64

[Daniel Veditz [:dveditz]]

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.

Comment 65

[Daniel Veditz [:dveditz]]

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?

Comment 66

[Hubert Figuiere [:hub]]

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.

Comment 67

[Hubert Figuiere [:hub]]

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());

Comment 68

[Alexander Sack]

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

Comment 69

[Boris Zbarsky [: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?

Comment 70

[Alexander Sack]

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?

Comment 71

[Boris Zbarsky [:bzbarsky]]

I thought someone was working on backporting the .desktop support.

If not, you could test on Windows by disabling the ".url is executable" bit.

Comment 72

[Daniel Veditz [:dveditz]]

Not blocking 1.8 branch until .desktop support lands on that branch, taking off the list for now.

Comment 73

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

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

Comment 74

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

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

Comment 75

[Boris Zbarsky [:bzbarsky]]

Bug 455311 might be enough if the redirect check is done on URIs, not channels, and if the right URI is used.