Last Comment Bug 460425 - (CVE-2009-0356) [FIX].desktop allows remotely loading "about:config" & "about:plugins" with little user interaction
(CVE-2009-0356)
: [FIX].desktop allows remotely loading "about:config" & "about:plugins" with l...
Status: VERIFIED FIXED
[sg:moderate] high/critical with miti...
: verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: Networking: File (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla1.9.1b3
Assigned To: Boris Zbarsky [:bz] (TPAC)
:
Mentors:
Depends on:
Blocks: 480907
  Show dependency treegraph
 
Reported: 2008-10-17 02:59 PDT by georgi - hopefully not receiving bugspam
Modified: 2009-03-04 18:49 PST (History)
14 users (show)
benjamin: blocking1.9.1+
samuel.sidler+old: blocking1.9.0.6+
dveditz: wanted1.9.0.x+
samuel.sidler+old: blocking1.8.1.19-
dveditz: wanted1.8.1.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
perl script - chmod +x ; put on web server ; load ; open with firefox (199 bytes, text/plain)
2008-10-17 02:59 PDT, georgi - hopefully not receiving bugspam
no flags Details
Unit test (not to be checked in till we open up the bug) (5.35 KB, patch)
2008-10-22 19:55 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
Fix as described (22.13 KB, patch)
2008-10-22 19:58 PDT, Boris Zbarsky [:bz] (TPAC)
cbiesinger: review+
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
mq diff (22.10 KB, patch)
2008-11-25 13:44 PST, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
1.9 branch backport (19.07 KB, patch)
2008-12-08 22:39 PST, Boris Zbarsky [:bz] (TPAC)
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review
1.8 backport (9.08 KB, patch)
2009-01-26 13:18 PST, Martin Stránský
no flags Details | Diff | Splinter Review
1.8 with nsFileChannel::HandleRedirect (16.55 KB, patch)
2009-01-27 05:15 PST, Martin Stránský
no flags Details | Diff | Splinter Review
1.8 with build fixes and 462034 backout (16.00 KB, patch)
2009-01-30 04:02 PST, Alexander Sack
bzbarsky: review-
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2008-10-17 02:59:21 PDT
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
Comment 1 Boris Zbarsky [:bz] (TPAC) 2008-10-17 05:37:28 PDT
Uh...  This is exactly what bug 455311 is about.  I suggest testing with a build that has that bug fixed.

*** This bug has been marked as a duplicate of bug 455311 ***
Comment 2 georgi - hopefully not receiving bugspam 2008-10-17 05:57:25 PDT
>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 [:bz] (TPAC) 2008-10-17 06:37:19 PDT
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 [:bz] (TPAC) 2008-10-17 06:40:20 PDT
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 2008-10-17 07:49:34 PDT
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 2008-10-17 08:16:11 PDT
> 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 [:bz] (TPAC) 2008-10-17 09:03:31 PDT
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] 2008-10-20 11:35:58 PDT
assigning to bz since he thought his bug 455311 fix would catch this one too.
Comment 9 Boris Zbarsky [:bz] (TPAC) 2008-10-20 12:39:02 PDT
It doesn't, in fact.  I need an answer to comment 4 to proceed here.
Comment 10 Boris Zbarsky [:bz] (TPAC) 2008-10-20 12:44:06 PDT
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 [:bz] (TPAC) 2008-10-22 19:55:00 PDT
Created attachment 344422 [details] [diff] [review]
Unit test (not to be checked in till we open up the bug)
Comment 12 Boris Zbarsky [:bz] (TPAC) 2008-10-22 19:58:36 PDT
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.
Comment 13 georgi - hopefully not receiving bugspam 2008-10-23 01:11:46 PDT
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 2008-10-23 02:24:20 PDT
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 2008-10-23 04:05:35 PDT
hm, bz, will your xml fixes fix the rdf redirect abuse in Bug 414540?
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2008-10-23 06:29:12 PDT
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 2008-10-23 06:57:36 PDT
>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 [:bz] (TPAC) 2008-10-23 07:41:53 PDT
> 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 2008-10-24 05:54:49 PDT
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 2008-10-24 06:25:29 PDT
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] 2008-10-24 11:08:20 PDT
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 [:bz] (TPAC) 2008-10-24 11:30:21 PDT
Probably not a bad idea...
Comment 23 georgi - hopefully not receiving bugspam 2008-10-24 23:57:13 PDT
>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 2008-10-25 00:02:51 PDT
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) 2008-10-25 01:43:49 PDT
So... do we care if about:plugins leads to an ugly URL?
Comment 26 georgi - hopefully not receiving bugspam 2008-10-25 02:14:46 PDT
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 [:bz] (TPAC) 2008-10-25 06:15:23 PDT
> 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) 2008-10-27 09:17:51 PDT
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 [:bz] (TPAC) 2008-10-27 09:31:10 PDT
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) 2008-10-27 11:19:21 PDT
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 [:bz] (TPAC) 2008-10-28 13:48:18 PDT
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 2008-11-06 05:10:38 PST
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 2008-11-06 06:23:23 PST
according to my tests 1.8 branch ignores .desktop files...
Comment 34 Boris Zbarsky [:bz] (TPAC) 2008-11-06 07:31:41 PST
We need this fix on 1.8 branch because .url files _are_ supported there on Windows.
Comment 35 georgi - hopefully not receiving bugspam 2008-11-06 09:01:01 PST
is windows affected by remote extension forcing ? (someone wrote it renames dangerous files)
Comment 36 Boris Zbarsky [:bz] (TPAC) 2008-11-06 09:12:26 PST
I'm not worried about the remote thing, but there is still a local issue on Windows.
Comment 37 georgi - hopefully not receiving bugspam 2008-11-12 04:32:28 PST
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) 2008-11-14 10:54:58 PST
Boris, any update on the "real" version of this?
Comment 39 Samuel Sidler (old account; do not CC) 2008-11-14 10:55:50 PST
Or rather, are we still just waiting for reviews here? Jonas?
Comment 40 Boris Zbarsky [:bz] (TPAC) 2008-11-14 11:40:15 PST
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) 2008-11-18 12:16:25 PST
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) 2008-11-19 15:29:45 PST
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 [:bz] (TPAC) 2008-11-25 13:44:37 PST
Created attachment 350052 [details] [diff] [review]
mq diff
Comment 44 Boris Zbarsky [:bz] (TPAC) 2008-11-25 17:57:58 PST
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 [:bz] (TPAC) 2008-11-25 18:00:53 PST
Make that http://hg.mozilla.org/mozilla-central/rev/5d1fbada8589
Comment 46 Boris Zbarsky [:bz] (TPAC) 2008-11-25 19:33:58 PST
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 [:bz] (TPAC) 2008-12-08 22:12:15 PST
Filed bug 468572 as a followup.
Comment 48 Boris Zbarsky [:bz] (TPAC) 2008-12-08 22:39:01 PST
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.
Comment 49 Samuel Sidler (old account; do not CC) 2008-12-22 11:51:36 PST
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 2009-01-05 14:09:11 PST
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] 2009-01-07 15:04:50 PST
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 [:bz] (TPAC) 2009-01-08 09:28:57 PST
Fixed on 1.9.0.6.
Comment 53 Al Billings [:abillings] 2009-01-14 13:57:49 PST
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] 2009-01-15 15:26:55 PST
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] 2009-01-15 17:05:58 PST
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ý 2009-01-26 13:18:25 PST
Created attachment 358897 [details] [diff] [review]
1.8 backport
Comment 57 Alexander Sack 2009-01-27 04:43:32 PST
(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 2009-01-27 04:51:13 PST
(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ý 2009-01-27 05:15:19 PST
Created attachment 359033 [details] [diff] [review]
1.8 with nsFileChannel::HandleRedirect

This should address it
Comment 60 Boris Zbarsky [:bz] (TPAC) 2009-01-27 06:35:37 PST
> 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 [:bz] (TPAC) 2009-01-27 08:00:25 PST
If we land this on 1.8 branch, we should back out the bug 462034 hack we did there.
Comment 62 Daniel Veditz [:dveditz] 2009-01-27 08:27:47 PST
asac: do you want this to block 1.8.1.next?
Comment 63 Hubert Figuiere [:hub] 2009-01-29 14:20:46 PST
(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] 2009-01-29 14:37:08 PST
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] 2009-01-29 14:39:33 PST
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] 2009-01-29 14:52:54 PST
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] 2009-01-29 16:51:22 PST
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 2009-01-30 04:02:01 PST
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!
Comment 69 Boris Zbarsky [:bz] (TPAC) 2009-01-30 13:21:09 PST
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 2009-02-04 06:07:01 PST
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 [:bz] (TPAC) 2009-02-04 07:07:26 PST
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] 2009-02-27 13:55:07 PST
Not blocking 1.8 branch until .desktop support lands on that branch, taking off the list for now.
Comment 73 Martin Stránský 2009-03-01 23:45:09 PST
A patch for .desktop file support for Linux is in Bug 480907.
Comment 74 Martin Stránský 2009-03-02 01:23:29 PST
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 [:bz] (TPAC) 2009-03-02 07:09:08 PST
Bug 455311 might be enough if the redirect check is done on URIs, not channels, and if the right URI is used.

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