XUL error pages can be used for privilege elevation attacks

RESOLVED FIXED in mozilla1.8beta4

Status

()

RESOLVED FIXED
14 years ago
12 years ago

People

(Reporter: mikx, Assigned: benjamin)

Tracking

Trunk
mozilla1.8beta4
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0.5 -
blocking1.8b3 -
blocking1.8b5 +
blocking-aviary1.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix][no l10n impact] uses view-source:javascript: url, has patch with review, URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

14 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.
(Reporter)

Comment 1

14 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.
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.
Assignee: nobody → dveditz
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8b3+
Flags: blocking-aviary1.1+
Flags: blocking-aviary1.0.4?
Whiteboard: [sg:fix] uses view-source:javascript: url
Blocks: 256195
Not blocking branch release (feature not on, view-source:javascript defanged).
Flags: blocking-aviary1.0.5? → blocking-aviary1.0.5-
(Assignee)

Updated

14 years ago
Blocks: 216466
(Assignee)

Comment 4

14 years ago
Taking: I'm hoping to use a separate protocol "-x-moz-errorpage:" to avoid
privilege escalation.
Assignee: dveditz → benjamin

Comment 5

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

Comment 6

14 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?
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.
(Assignee)

Comment 8

14 years ago
Bumping to b4, as there are not known attack vectors.
Flags: blocking1.8b4+
Flags: blocking1.8b3-
Flags: blocking1.8b3+
(Assignee)

Comment 9

14 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.
Attachment #187314 - Flags: superreview?(dveditz)
Attachment #187314 - Flags: review?(darin)
(Assignee)

Comment 10

14 years ago
Note: this patch should also remove docshell/resources/content/jar.mn and
docshell/resources/content/netError.js
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))
(Assignee)

Comment 12

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

Comment 13

14 years ago
bsmedberg: why is a new protocol scheme the best solution here?
(Assignee)

Comment 14

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

Comment 15

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

Comment 16

14 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.
if you use about:foo then there must be a pref. embeddors (epiphany) want to
override the error page.

Comment 18

14 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.
I'm not sure I understand what you mean, are you suggesting that epiphany
changes the system gecko?
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
(Assignee)

Comment 21

14 years ago
Attachment #187314 - Attachment is obsolete: true
Attachment #187532 - Flags: review?(darin)

Comment 22

14 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
Attachment #187532 - Flags: review?(darin) → review+
so what's the solution for embeddors?
(Assignee)

Updated

14 years ago
Attachment #187314 - Flags: superreview?(dveditz)
Attachment #187314 - Flags: review?(darin)
(Assignee)

Comment 24

14 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.
Attachment #187532 - Flags: superreview?(bzbarsky)
> 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...
(Assignee)

Comment 26

14 years ago
This combines the hash-and-query search in one (very simple, surprisingly)
utility function.
Attachment #187532 - Attachment is obsolete: true
Attachment #188431 - Flags: superreview?(bzbarsky)
Attachment #188431 - Flags: review?(darin)
ah, actually, embeddors could probably just override the nsIAboutModule for
about:neterror.
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

14 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.
Attachment #188431 - Flags: review?(darin) → review+
(Assignee)

Comment 30

14 years ago
*** Bug 300003 has been marked as a duplicate of this bug. ***
Comment on attachment 188431 [details] [diff] [review]
Use about:neterror unprivileged, rev. 2

sr=dveditz
Attachment #188431 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

14 years ago
Component: General → Embedding: Docshell
Flags: review+
Product: Firefox → Core
Target Milestone: --- → mozilla1.8beta4
Version: unspecified → Trunk
(Assignee)

Comment 32

14 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
Attachment #188431 - Flags: review+
Attachment #188431 - Flags: approval1.8b4?
(Assignee)

Updated

14 years ago
Whiteboard: [sg:fix] uses view-source:javascript: url → [sg:fix] uses view-source:javascript: url, has patch with review
(Assignee)

Updated

14 years ago
Whiteboard: [sg:fix] uses view-source:javascript: url, has patch with review → [sg:fix][no l10n impact] uses view-source:javascript: url, has patch with review
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.
(Assignee)

Comment 34

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

Updated

14 years ago
Attachment #188431 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 36

14 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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 37

14 years ago
The essential problem is not fixed yet. see bug 300830.
Attachment #187532 - Flags: superreview?(bzbarsky)

Updated

13 years ago
Flags: testcase+
Group: security

Updated

12 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.