221490 - files with xhtml extension are not threated like .html in chrome - file nsChromeProtocolHandler.cpp

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Cedric. S.]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4) Gecko/20030624

when accessing files with xhtml extension in the chrome directory (through a
chrome URL), javascript contained in this xhtml file is not executed/loaded.
This seems to come from the rdf/chrome/src/nsChromeProtocolHandler.cpp file line
727.

This code seems to give rights to the file on the basis of their extension
instead of the basis of their MIME type.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Boris Zbarsky [:bzbarsky]]

File handling is what happens once the data gets out of the network library. 
This is a bug in the protocol handler itself.

caillon, thoughts on this one?  Why nt set the principal regardless of
extension/type?

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

For reference, this behavior was initially introduced as a sandbox patch to bug
41876, and expanded on (by adding more extensions to the whitelist) in bug 56009.  

I think this code exists to limit the files that remote chrome (if we supported
it) would be able to load as chrome.  However, I think that reason is kind of
bogus.  If we support one file extension as chrome, and a malicious site wants
to do evil things, all they have to do is just use the one extension we permit.

CC'ing others who may have opinions.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We should resolve this bogosity for 2.0.  Do we have a tracking bug for that yet?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

OK, people keep popping up on IRC using .htm or .xhtml files in chrome.  I
really see no reason to limit the principal to these three extensions, or to
any list of extensions.  Unless someone can, I would say we should just remove
this check.

Is there anyone who should be cced on this bug and isn't?

Comment 6

[Benjamin Smedberg]

I'm investigating possible use of javascript (or data?) URIs in chrome
stylesheets included by untrusted content (can content manipulate chrome
stylesheets through the DOM after loading them?). Although giving them the
system principal probably won't affect that particular kind of exploit.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> can content manipulate chrome stylesheets through the DOM after loading them?

Yes.  We should consider fixing that (since content can't _read_ the rules, but
can add new ones).

Note that giving the stylesheet channel a system owner may not affect this case,
since the javascript: channel won't get an owner (currently).  But our other
work on tagging URIs with principals may matter here.

Another thing that was raised on irc: perhaps skin packages should NOT get
system principals.  There's no good reason for them to.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note that this is now breaking extensions pretty badly (bug 297603) instead of
just breaking them a little bit (bug 138850).

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 184424 [details] [diff] [review]
Patch

>-        nsCAutoString fileExtension;
>-        rv = url->GetFileExtension(fileExtension);
>-        if (PL_strcasecmp(fileExtension.get(), "xul") == 0 ||
>-            PL_strcasecmp(fileExtension.get(), "html") == 0 ||
>-            PL_strcasecmp(fileExtension.get(), "xml") == 0)
>-        {

Would anyone object if we changed this to limit system principal to
"content" packages?

	  nsCAutoString dir;
	  rv = url->GetDirectory(dir);
	  if (StringBeginsWith(dir, NS_LITERAL_CSTRING("/content/")))
	  {
	     ... original code...

Comment 10

[Daniel Veditz [:dveditz]]

Darin says this isn't networking. There doesn't seem to be a good place for
chrome registry type bugs.

Comment 11

[Benjamin Smedberg]

Comment on attachment 184424 [details] [diff] [review]
Patch

We should limit this to content/locale packages using the StringBeginsWith code
mentioned above.

Furthermore, unless there is a really good reason to put this on the branches,
I think it should be trunk-only.

Comment 12

[Daniel Veditz [:dveditz]]

Attachment: empower content in general

Comment 13

[Benjamin Smedberg]

Fixed on trunk.

Comment 14

[jag (Peter Annema)]

This seems to have made Ts go up, e.g. comet went from ~1262ms to ~1286ms,
monkey went from 3250ms to 3540ms, luna went up from 3450ms to 3550ms. Txul
remained relatively stable.

Is this because the StringBeginsWith test is much slower than the three
|PL_strcasecmp|s, or are we setting the system principal more often than we
really need to now?