Michael Krax Reporter
	Description • 19 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

The XUL error pages are a chrome: url. While the error page itself isn't
privileged, it can redirect to privileged chrome files which allows to execute
arbitrary code.

When the XUL error page is embedded in an iframe it is only protected from the
parent content by the cross site scripting restrictions. This automaticly makes
all cross site scripting bugs (like the currently unpatched 290982) automaticly
an arbitrary code execution bug.

Reproducible: Always

Steps to Reproduce:
1. Open http://bugzilla:Jq6w2Rt@www.mikx.de/firepaging/
2. Follow instructions




Since XUL error pages are going to be default in 1.1 (afaik) it should be made
sure, that they can not be used for privilege elevation attacks under any
condition. Cross site scripting is only one option. If the propagation of the
drag and drop event ever changes in a way that allows to drop a link to an
embedded frame instead of the top document, a single drag operation (e.g. faked
scrolling) would be enough to exploit the error page.

	Michael Krax Reporter
	Comment 1 • 19 years ago

Further testings seem to show that this bug got introduced with 1.0.3. I can
repro on 1.0.3 Win32 and 1.0.3 Mac. But not on 0.9.2 / 1.0 / 1.0.2 Win32 or 1.0
/ 1.0.2 Mac.

	Daniel Veditz [:dveditz]
	Comment 2 • 19 years ago

Probably don't need for 1.0.x since this is not the default setting (and no
user-facing UI to switch it). Definitely a problem on the trunk. Uses
view-source:javascript: so might get fixed anyway.

	Daniel Veditz [:dveditz]
	Comment 3 • 19 years ago

Not blocking branch release (feature not on, view-source:javascript defanged).

	Benjamin Smedberg Assignee
	Comment 4 • 19 years ago

Taking: I'm hoping to use a separate protocol "-x-moz-errorpage:" to avoid
privilege escalation.

	Darin Fisher
	Comment 5 • 19 years ago

An URL scheme cannot begin with a hyphen.  It must begin with an ASCII letter.

	Darin Fisher
	Comment 6 • 19 years ago

Aren't there some about: pages that run with chrome privs?  about:plugins?

We currently prevent sites from loading a file:// URL into an <iframe>, so why
don't we similarly restrict chrome:// and resource://?  Why do we need another
URL protocol for error pages?

	Daniel Veditz [:dveditz]
	Comment 7 • 19 years ago

We do restrict chrome from being loaded -- assuming someone doesn't find a way
around it which they occassionally have in the past. We think we've plugged all
the holes, but have we?

Merely using a different protocol isn't going to help. If the template/code file
is in the chrome jar as it is now then the attacker wouldn't use the new
protocol, they'd still be able to open the file using the chrome: protocol. The
current error pages accept arguments on the chrome: url which makes it
particularly prone to external attack. If it required arguments[] like all the
other dialogs it'd be a lot safer.

Unless you're talking about using a new protocol, and then generating the page
completely dynamically with no .xul found in a .jar anywhere.

	Benjamin Smedberg Assignee
	Comment 8 • 19 years ago

Bumping to b4, as there are not known attack vectors.

	Benjamin Smedberg Assignee
	Comment 9 • 19 years ago

I discovered all sorts of nasty corners or our security system while doing this
patch: the bit about originalURI in nsDocument was particularly annoying, as I
don't understand why we can't use originalURI consistently and need that
protocol whitelist.

	Benjamin Smedberg Assignee
	Comment 10 • 19 years ago

Note: this patch should also remove docshell/resources/content/jar.mn and
docshell/resources/content/netError.js

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 11 • 19 years ago

why are you removing the pref? (are embeddors now supposed to override the
protocol handler impl if they want their own error page? (which may actually be
trivial for some))

	Benjamin Smedberg Assignee
	Comment 12 • 19 years ago

If we really want the pref, it cannot be in nsDocshell, it must be in
nsErrorPageProtocolHandler.cpp. *However*, the pref *must not* point to a chrome
URL, or else the channel gets the system principal anyway.

	Darin Fisher
	Comment 13 • 19 years ago

bsmedberg: why is a new protocol scheme the best solution here?

	Benjamin Smedberg Assignee
	Comment 14 • 19 years ago

Darin, the protocol change is IMO the best solution because

1) we don't want to give the page system privs
2) we always want the page to be able to run script

Plus it seems like a fairly simple solution. I'm open to other solutions but I
can't think of any offhand.

	Darin Fisher
	Comment 15 • 19 years ago

How about "about:neterror?params"

nsScriptSecurityManager.cpp already allows about: pages to run script when
script is enabled.  They are not given system privs by default (AFAIK), and it
is really easy to define a new about: page via the about redirector.  Why not
this solution?

	Darin Fisher
	Comment 16 • 19 years ago

I meant "when script is _disabled_"

nsAboutRedirector.cpp allows you to specify the about pages that have chrome
privs and those that do not.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 17 • 19 years ago

if you use about:foo then there must be a pref. embeddors (epiphany) want to
override the error page.

	Darin Fisher
	Comment 18 • 19 years ago

> if you use about:foo then there must be a pref. embeddors (epiphany) want to
> override the error page.

I don't understand.  The about:foo would be redirector to some file, either in a
chrome package or in the res folder.  Epiphany could just override that file
instead of the URL used to indirectly load it.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 19 • 19 years ago

I'm not sure I understand what you mean, are you suggesting that epiphany
changes the system gecko?

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 20 • 19 years ago

Comment on attachment 187314 [details] [diff] [review]
moz-neterror protocol handler, rev. 1

+nsErrorPageProtocolHandler::NewChannel(nsIURI* aURI, nsIChannel* *aResult)
+{
+  nsCAutoString spec;
+  aURI->GetSpec(spec);

this variable looks unused

	Darin Fisher
	Comment 22 • 19 years ago

Comment on attachment 187532 [details] [diff] [review]
Use about:neterror unprivileged, rev. 1

Hrm... the #-mark checking is not really needed, right?  You just added that
for good measure, right?  The about: protocol doesn't support nsIURL, so it
sort of doesn't support reference fragments.  Hrm... ok.

r=darin

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 23 • 19 years ago

so what's the solution for embeddors?

	Benjamin Smedberg Assignee
	Comment 24 • 19 years ago

Comment on attachment 187532 [details] [diff] [review]
Use about:neterror unprivileged, rev. 1

the #-mark checking code was copied directly from nsAboutProtocolHandler, so
that nsAboutRedirector and nsAboutProtocolHandler had the same parsing rules.

biesi, nsAboutRedirector does not have any pref-overrides: I wouldn't mind a
secondary patch to add such.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 25 • 19 years ago

> the #-mark checking code was copied directly from nsAboutProtocolHandler

how about moving the code in a common function? these two files end up in the
same DSO...

	Benjamin Smedberg Assignee
	Comment 26 • 19 years ago

This combines the hash-and-query search in one (very simple, surprisingly)
utility function.

	Christian :Biesinger (don't email me, ping me on IRC)
	Comment 27 • 19 years ago

ah, actually, embeddors could probably just override the nsIAboutModule for
about:neterror.

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 19 years ago

I'll try to get to this in the next day or two...  I do think we want a separate
bug (blocking 1.8) on having a simpler solution here for embeddors than
overriding the about module.

	Darin Fisher
	Comment 29 • 19 years ago

Comment on attachment 188431 [details] [diff] [review]
Use about:neterror unprivileged, rev. 2

Looks good.

I wonder if it wouldn't make sense to give script execution privs to all about:
URLs except about:blank.  Maybe better safe than sorry, ok.

	Benjamin Smedberg Assignee
	Comment 30 • 19 years ago

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

	Daniel Veditz [:dveditz]
	Comment 31 • 19 years ago

Comment on attachment 188431 [details] [diff] [review]
Use about:neterror unprivileged, rev. 2

sr=dveditz

	Benjamin Smedberg Assignee
	Comment 32 • 19 years ago

Comment on attachment 188431 [details] [diff] [review]
Use about:neterror unprivileged, rev. 2

first-review was from darin, switching products cleared the flag

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 33 • 19 years ago

I've done a lot of work for bug 280190. With this patch we aren't allowed to use
any referenced css and image files? This would be very annoying, because the
unstyled version doesn't look well. Inserting the css and image files into the
netError.xhtml file will disallow themes to bring their own style. What's happen
with the access to netError.dtd? Is this allowed? For the last patch v10 I
changed the code to use a StringBundle. Was this work really useless?

Benjamin, if you could give me (hopeful) answers, I would be glad.

	Benjamin Smedberg Assignee
	Comment 34 • 19 years ago

> I've done a lot of work for bug 280190. With this patch we aren't allowed to use
> any referenced css and image files? This would be very annoying, because the

Why do you say that?

> netError.xhtml file will disallow themes to bring their own style. What's 

I'm not sure I care all that much about theming this page.

> changed the code to use a StringBundle. Was this work really useless?

Yes, unfortunately.

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 35 • 19 years ago

(In reply to comment #34)
> > netError.xhtml file will disallow themes to bring their own style. What's 
> 
> I'm not sure I care all that much about theming this page.

I reverted my changes and it possible should work to include external CSS and
images.

So just one question. Benjamin, you moved all code from netError.js into
netError.xhtml. What was your reason in attachment 188431 [details] [diff] [review]? Do you think we could
still hold this file to swap out the processing script?

	Benjamin Smedberg Assignee
	Comment 36 • 19 years ago

Fixed on trunk. I'm pretty sure that we can clear the security flag on this,
because it's not on by default on the branches and there are no known trunk
exploits.

	shutdown
	Comment 37 • 19 years ago

The essential problem is not fixed yet. see bug 300830.