228341 - nsTreeStyleCache::GetStyleContext potentially uses an untrusted array of nsIAtom*

Status: RESOLVED INVALID

(Core :: XUL, defect)

Description

[Benjamin Smedberg]

This code:
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#153

Assumes that we're passing it an nsIAtom*. However, this array can be passed in
from untrusted code through 

nsTreeBodyFrame::GetPseudoStyleContext
nsTreeBodyFrame::GetCoordsForCellItem
which calls
mView->GetCellProperties(aRow, currCol->GetID().get(), mScratchArray); to a
potentially-untrusted source

I can't see a way to cause a predictable security problem with this code, but it
can easily crash the browser (DOS attack).

Continuing thinking about the nsITreeView API, [e-mail] want to be
able to use this API from untrusted code, but we currently don't have an
untrusted API to nsIAtom. In particular, xpconnect must not allow scripts to
implement the nsIAtom API, which would defeat the whole purpose of atoms
pointer-equality. Perhaps we need a new XPCOM interface attribute
[scriptable,script-noimplement] or somesuch?

Comment 1

[Boris Zbarsky [:bzbarsky]]

This code should use existing facilities on the nsISupportsArray that ensure it
gets the right type (QueryElementAt).

And as I recall, atoms are not now nor ever will be for use by untrusted code;
last I checked there was talk of making nsIAtom not scriptable.

The proper solution, imo, is to allow the array to contain either nsIAtom or
nsISupportsString.  The code can use QueryElementAt to try one or the other and
go with the one that works (try atom first).

ccing some people who may be able to comment as to whether the nsITreeView API
is supposed to be usable from untrusted code.

Comment 2

[Benjamin Smedberg]

re: atoms used by untrusted code, I don't see why we shouldn't allow it, as long
as JS can't *implement* nsIAtom. When RDF becomes scriptable we're gonna have
the same problems with nsIRDF(Node|Literal|Resource)... now filed as bug 228727.

We do need a decision on whether nsITreeView is something useful for untrusted
code. IMO it probably should be allowed, as long as security and stability
aren't compromised. (recognizing that nsITreeView is not frozen and will change).

Comment 3

[[e-mail]]

False alarm - untrusted JS isn't allowed to call methods on the
nsISupportsArray. (Trusted JS could still crash the browser though.)

Comment 4

[Jan Varga [:janv]]

I agree, these methods should be revised. We could create a new interface, e.g.
nsITreeProperty having atom and string representation of the property.

I use a similar principle in my development tree for columns:
getCellText(in long row, in wstring col) -> getCellText(in long row, in
nsITreeColumn col)
nsITreeColumn interface is more complex of course

Comment 5

[Alec Flett]

remember, nsISupportsArray will never be frozen either.

I'm still not 100% clear why we need to prevent nsIAtom from being implemented
by someone - I mean, if they implement a new object, then it isn't going to
match any existing atoms by definition (i.e. pointer inequality) Do we think
there will be some kind of attack that relies on 

(atom1 != atom2 && atom1.toString() == atom2.toString()) 

being true? 

But even not understanding that myself, is denying scriptability of nsIAtom is 
enough to guarantee this security? I mean a C++ component could implement
nsIAtom too...I guess it would just be a little harder to make the user download
a DLL?

Again assuming this level of security which I don't quite understand, if we
rework to use nsITreeProperty, we probably want to dump nsIAtom support in that
interface entirely, making nsITreeProperty a drop-in replacement for nsIAtom.
And in that case, if it isn't scriptable, does it matter if we can or can't get
the string value out of it? Who would even need the string value?

Comment 6

[[e-mail]]

IMHO the way to inflict the least damage would be to have nsITreeProperties be a
wrapper around (say) nsISupportsArray that allows the adding of atoms (for
compatibility) and strings (which would get atomized ;-)

Comment 7

[Alec Flett]

Well, don't forget we have nsIArray, a drop-in, soon-to-be-frozen replacement
for nsISupportsArray.

Comment 8

[timeless]

well. nsIAtom says that it gives you a pointer. Things in js aren't guaranteed
to have constant addresses (they happen to today but that may change), would
that affect the poor nsIAtom?

Comment 9

[Brendan Eich [:brendan]]

I invalidated bug 228727.  The XPIDL [scriptable] attribute is not a designation
of what untrusted code can call, or we wouldn't need mozilla/caps.

I don't know what comment 0 means here: "However, this array can be passed in
from untrusted code through [a bunch of C++ that might call through nsIFoo]." 
Untrusted web scripts cannot call through nsIFoo even if it's scriptable, right?
UniversalXPConnect needs to be granted first, doesn't it?

Anyway, someone should say what new level of trust is meant here.  We have sound
mechanisms for the web page trust domains vs. chrome JS.

/be

Comment 10

[Brendan Eich [:brendan]]

And D.O.S. attacks aren't generally treatable with mandatory access control, so
I'm not sure what that part of comment 0 is about, either.  OTOH, a direct crash
caused by evil untrusted code is not a DOS attack.  What's the scoop?

/be

Comment 11

[Benjamin Smedberg]

I'm sorry, I filed this bug for Steve based on an IRC conversation that was only
half-baked... I thought he was able to access the nsISupportsArray, but that's
not true; untrusted code cannot cause problems because it can't add anything to
the array.

Steve: feel free to file new bugs about making nsITreeView usable for untrusted
code (no atoms, etc). I'm gonna close this one because it's too vague.

btw: how is a crash not DOS?

Comment 12

[Brendan Eich [:brendan]]

A crash is worse than DOS -- it is potentially exploitable (buffer overrun).  In
my vocabulary, anyway.  Sure, crashing denies service, but it's a different kind
of animal from a DDOS or a near-iloop.

/be