228727 - don't allow untrusted to implement some interfaces

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Benjamin Smedberg]

spinoff from bug 228341 and some thoughts I've had regarding making the RDF
interfaces available to untrusted code: there are various interfaces that
javascript code should be able to *use* but never to *implement*. Important ones
that come to mind:

nsIAtom
nsIRDF(Node|Literal|Resource|Container)
nsIURI
nsIClassInfo (this is probably already prevented somewhere)
nsIVariant
nsI(Local)?File

And maybe others (especially in the DOM) I haven't thought of. So I propose a
new XPIDL attribute [scriptable, noscriptimplement] (anyone have a better
suggestion for the name? it seems very long). I thought about just changing
[scriptable] to [script-read] but that would break the XPT fileformat in an
incompatible manner.

Comment 1

[Mike Shaver (:shaver emeritus)]

Why mustn't JS implement those?  Language choice is orthogonal to trust, and
this seems like something that we should solve with caps.  What if I want to
implement a new RDF container in Venkman or Chatzilla or one of the other handy
JS-only components?  Should I need to write C++ for that?

Comment 2

[Brendan Eich [:brendan]]

Shaver's right, this bug is invalid.  Trust has nothing to do with
implementation language preference.

/be

Comment 3

[Benjamin Smedberg]

OK, let's go back then... I need a way to prevent "non-universalxpconnect"
script from implementing various interfaces; if we're gonna make parts of RDF
available to untrusted script (a goal Pike has been looking through), we can't
let untrusted script implement nsIRDFNode and its derivatives, they must be
retrived from the designated factory. I don't see any xpconnect/caps system to
prevent this; AFAIK all our CAPS system does is allow/deny script access to an
existing object.

(btw, nsIAtom is a special case. since it breaks the XPCOM rules and uses
pointer identity to check string equality; it should probably not be scriptable).

Comment 4

[Alec Flett]

I can't help but wonder if solving the RDF thing is going to be an RDF-specific
problem.

I don't see a way of preventing non-core code from implementing some arbitrary
interface. If anything, it seems like some ugly check like some internal
static PRBool nsIRDFService::IsNode(nsIRDFNode* node);

which would check the validity of the pointer by checking some hash set, and be
used inside the RDF library. 

Maybe we need something similar on nsIAtomService?

the only real generic way I can think to do this is to somehow be able to sign
libraries/js/etc and have xpconnect be aware of pointers that come in/out of
those libraries... though that sounds really ugly and frankly almost impossible.

Comment 5

[Brendan Eich [:brendan]]

This bug is still mis-summarized.

> I don't see any xpconnect/caps system to prevent this; AFAIK all our CAPS
> system does is allow/deny script access to an existing object.

If someone did implement an nsIFoo that the proposed RDF code would call
trustfully, caps could be used to check the principals of the implementation. 
Again, declaring the ability to script is not a statement of trust, because
there is no subject/object relation in the static IDL source files.

In the RDF case, the subject would be the trusted system RDF code, and the
object would be the other subject (i.e., code) implementing the interface that
RDF might call through.  This is a job for caps.

/be

Comment 6

[Brendan Eich [:brendan]]

As alecf points out, the onus of checking falls on any RDF extension that might
call untrusted code.

When I wrote "this is a job for caps", I did not mean to suggest that CAPS
automagically check on behalf of RDF, although XPConnect could do some such
check, given a typelib annotation on the interface (but we don't want XPConnect
depending on caps directly).

So I agree with alecf: RDF has to do the checking, calling CAPS code as needed.

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

The XPConnect code that wraps JS objects in their vtable trappings can, I
believe, find the principal for the object in question, and almost certainly for
the calling code (called code, in the case of a return value or out-param which
provides an object to be interpreted as an nsIRestrictedThing).  Given that, I
think it could require that the script has system principals, if an appropriate
bit is set in the interface descriptor for the interface in question.

A more general approach would be to allow [needsPrivilege("rdfImplementation")]
in the interface declaration, which the XPConnect wrapping code could then check
against the privileges currently granted to the script/object/whatever in
question.  I don't think we need that generality at present, but I haven't
looked in detail.

Comment 8

[Brendan Eich [:brendan]]

> the only real generic way I can think to do this is to somehow be able to sign
> libraries/js/etc and have xpconnect be aware of pointers that come in/out of
> those libraries... though that sounds really ugly and frankly almost impossible.

We did that for Nav4.x, and it supposedly was made to work in Mozilla, although
I bet it's broken.

But the underlying principals support is there in the JS engine.  We know where
a given script came from, or who signed it.  If the user makes a statement of
trust, we can give script from a given "codebase principal" or signed by a given
"certificat principal" any power we choose.

I just mid-air'd with shaver's latest comment, and we seem to be agreeing that a
general, IDL-stated way of specifying the capability target is not needed yet. 
Let's not go overboard without <= 1 basis case!

/be

Comment 9

[[e-mail]]

While certainly nsIAtom and nsIRDFNode and its descendants have issues I thought
I should point out that it is impossible for untrusted content to use a custom
tree view without implementing nsIClassInfo.

Comment 10

[Brendan Eich [:brendan]]

This bug is about RDF; maybe it should be assigned there.

Really, you're proposing opening a security hole to-do with valuables that the
XPConnect subsystem knows nothing about, and (in this bug's initial statement
and comments) expecting a low-level subsystem to "fix" that hole.  What you
should be doing is designing whatever mandatory access controls are needed to
make the RDF functionality you want to expose be "safe".

/be

Comment 11

[Andrew McCreight [:mccr8]]

"some thoughts I've had regarding making the RDF interfaces available to untrusted code"
That sounds like a bad idea.