292624 - XUL error pages can be used for privilege elevation attacks

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Michael Krax]

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:[e-mail]/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.

Comment 1

[Michael Krax]

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.

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Daniel Veditz [:dveditz]]

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

Comment 4

[Benjamin Smedberg]

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

Comment 5

[Darin Fisher]

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

Comment 6

[Darin Fisher]

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?

Comment 7

[Daniel Veditz [:dveditz]]

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.

Comment 8

[Benjamin Smedberg]

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

Comment 9

[Benjamin Smedberg]

Attachment: moz-neterror protocol handler, rev. 1

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.

Comment 10

[Benjamin Smedberg]

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

Comment 11

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

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

Comment 12

[Benjamin Smedberg]

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.

Comment 13

[Darin Fisher]

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

Comment 14

[Benjamin Smedberg]

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.

Comment 15

[Darin Fisher]

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?

Comment 16

[Darin Fisher]

I meant "when script is _disabled_"

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

Comment 17

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

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

Comment 18

[Darin Fisher]

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

Comment 19

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

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

Comment 20

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

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

Comment 21

[Benjamin Smedberg]

Attachment: Use about:neterror unprivileged, rev. 1

Comment 22

[Darin Fisher]

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

Comment 23

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

so what's the solution for embeddors?

Comment 24

[Benjamin Smedberg]

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.

Comment 25

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

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

Comment 26

[Benjamin Smedberg]

Attachment: Use about:neterror unprivileged, rev. 2

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

Comment 27

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

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

Comment 28

[Boris Zbarsky [:bzbarsky]]

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.

Comment 29

[Darin Fisher]

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.

Comment 30

[Benjamin Smedberg]

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

Comment 31

[Daniel Veditz [:dveditz]]

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

sr=dveditz

Comment 32

[Benjamin Smedberg]

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

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

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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.

Comment 34

[Benjamin Smedberg]

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

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

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

Comment 36

[Benjamin Smedberg]

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.

Comment 37

[shutdown]

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