122846 - unprivileged access to rdf from scripts

Status: RESOLVED INCOMPLETE

(Core Graveyard :: RDF, defect)

Description

[Axel Hecht]

<Pike> waterson: is there a plan to get something like js> foo = new
RDFService(); working? So scripts don't need XPConnect privs to a rdf service?
<waterson> Pike: we need to split the RDF service into two parts to do that
<waterson> Pike: the first (which will be accessible w/o restriction) would
manage getting resources, literals, etc.
<waterson> Pike: the second (which will require restriction) would handle
GetDataSource
<waterson> Pike: we could create new objects, two interfaces, and then let the
old nsIRDFService interface be a shell that interacts with both for
compatibility
<Pike> waterson: how would a unpriviledged js get to a rdf datasource for a file
from the same server?
<waterson> Pike: need to think that one through. possibly by creating an RDF/XML
datasource
<waterson> Pike: I'd just as soon let GetDataSource die.
<waterson> Pike: GetDataSource preceded the XPCOM service manager, which really
does a better job of managing singletons.

I guess RDFContainerUtils is of general interest, too. Others?

Comment 1

[Chris Waterson]

Probably the RDF/XML datasource, serializer, parser, too. This is definitely
worth doing, but probably post mozilla-1.0.

Comment 2

[Axel Hecht]

I was thinking again about nsIRDFService::GetDataSource, I was wondering
whether the right thing to do were to add a [noscript] to this. Possibly
RegisterDataSource and UnregisterDataSource need to be priviledged, too.

Comment 3

[Martin Kutschker]

What about the remoteDatasource interface? Getting the datasource is nice, but
one would want to query the loaded property and/or attach an observer to the
datasource.

And it would be nice to get a datasource synchronously. AFAIK, you can do this
with remote datasources though it won't be loaded synchronous if the source is
already being loaded.

Comment 4

[Martin Kutschker]

Another one:

The ability to create a new in-memory datasource possibly instantiated with a
string (ie via the rdf string parser).

Comment 5

[Axel Hecht]

initializing from a string should be done with nsIRDFXMLParser::parseString.
IMHO, in-memory datasources have no link to a xml representation.

Comment 6

[Martin Kutschker]

AFAIK the rdf/xml parser parses into a datasource. Since we are talking about
capsulating XPCOM components for JS I thought about the follwoing interfaces

JS>remote_ds = new RDF(url[,sync])

This gets you an arbitray datasource via url (possibly snchronously).

JS>in_memory_ds = new InMemoryRDF([string])

This gets you an new data source. If you use a string argument, a (new) parser
will parse into the created datasource.

Ok, it's more about convenience that about security ;-)

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Ping.  Any action on this?  It's not just an academic question.  I've got a much
improved XUL-based interface to Bugzilla's duplicates report (bug 156548) just
waiting on a fix for this bug.

http://bugzilla.mozilla.org/duplicates.xul

Right now you have to add this line to your prefs.js file to make it work:

user_pref("signed.applets.codebase_principal_support", true);

The other workaround is to bundle the files into a signed jar, but that's a
royal pain in the butt.  Any chance this bug will be fixed anytime soon?

Comment 8

[Alex Vincent [:WeirdAl]]

Would be nice to get this fixed for documentation purposes too (converting
catalog to RDF)

Comment 9

[Axel Hecht]

I have been looking into this, and I even started doing DOMCI for RDFLiteral.
Next would be the friends in nsIRDFLiteral.idl and the container utils.
RDFResource is probably a stopper already, as that has GetDelegate, which may be
bad.

I was thinking about sneaking nsIRDFScriptableFoo interfaces between those
interfaces that should have chrome privs and nsIRDFNode or nsISupports.
Would that be ok, or evil?
Stuff like GetDataSource would be ok with a security check, but stuff like 
GetDelegate might be used at too many places to put the workload on it.

Chris?

Comment 10

[Chris Waterson]

Is your fear that placing a security check in GetDelegate would be a performance
problem?

Comment 11

[Chris Waterson]

[From a private email exchange with Axel...]

On Thu, 6 Feb 2003, Axel Hecht wrote:

> Exactly. I didn't investigate how ldap uses this. Mailnews does this for
> filters and smtp servers, AFAICT, which doesn't sound like too much of a
> problem.
  
Phear premature optimization! :-)  IOW, I'd give it a try and then ask    
people how to verify that it was/wasn't a performance problem.  If it is
a problem, then I think your solution is a reasonable alternative.  (But
see below!)

> I really think that we do need to worry, though. Exposing GetDelegate as
> is would make filter settings public, and you could probably get
> information in the news servers of the user. Sounds like a bad privacy
> issue, at least.
   
Not sure I understand this.  If there was a security check in GetDelegate,
and the check failed, wouldn't you just throw an exception before handing
back the delegate object?

---

There is another problem though, which could be exploitable.  Specifically, the
fact that "resource factories" can be registered with the RDF service.  In this
case, nsIRDFService::GetResource delegates creation of the resource object to an
external factory.  This was a precursor to the GetDelegate mechanism that we
used this to create resource objects that were immediately QI()-able to an
application-specific interface.

For example:

  var RDF = Components.classes["..."].getService(...);
  var res = RDF.GetResource("imap://mail.foo.com/...");
  var msg = res.QueryInterface(Components.interfaces.nsIMsgMailMessage);
  document.write("the message body is " + msg.body);

Zoiks!  Clearly, you can see the security implications here.  At best, somebody
gets away with creating an object and no more.  But even in this case, the
creation of the object could lead to unexpected information leaks. For example,
what if creation of an IMAP server object leads to establishing a connection to
that server?  It may be possible use this as a covert back-channel,
instantiation of the session may result in username/password exchange, who knows.

In the worst case, someone is able to QI() the object to a scriptable interface
(mail server, LDAP server, etc.) and begin unrestricted use of that object.

So, I'm concerned that you may need to eliminate this mechanism *altogether* and
fix implementations that rely upon it before you can safely dole out
nsIRDFResource objects via nsIRDFService::GetResource.

Comment 12

[Chris Waterson]

(FYI, this is one reason why we created GetDelegate.  We just never got around
to getting rid of the other mechanism!  What a bunch of slackers!)

Comment 13

[Axel Hecht]

point by point:
I'm all for "giving it a try", but I'm usually stuck with overthinking stuff 
first.

*If* GetDelegate had a security check, it would throw. I just wasn't sure if it
really needs one. The use-case I found indicates that it does.

about the resource factories, should we have a security check for resources that
trigger factories? not sure if that's feaseble at all.
Anyway, resources that don't come from the default resource factory will have a
different implementation, and thus not the classinfo to come. That info is 
bound to a specific binary implementation. So once you come up with one of 
those, you can't go anywhere without QI, or somebody implementing classinfo
on those object.
QI is assumed to be safe, as we use that at quite a few places to hide private
implementation details from public scriptable stuff.

Comment 14

[Chris Waterson]

> about the resource factories, should we have a security check for resources
> that trigger factories? not sure if that's feaseble at all.

This would stop untrusted script from creating new implementations, which may be
sufficient.  N.B., implementations are cached by the RDF service once created,
so that it would be possible to use the RDF service to get at an "already
created" implementation.  (But it seems like this is not a concern, based on
what you say below...)

> Anyway, resources that don't come from the default resource factory will
> have a different implementation, and thus not the classinfo to come. That
> info is  bound to a specific binary implementation. So once you come up
> with one of those, you can't go anywhere without QI, or somebody
> implementing classinfo on those object.
>
> QI is assumed to be safe, as we use that at quite a few places to hide
> private implementation details from public scriptable stuff.

You probably know more about this stuff than I do.  If I understand what you're
saying:

1. Individual implementations are marked as "safe from untrusted code" versus
"unsafe from untrusted code"

2. A scriptable wrapper is only created for "safe" objects.  (Alternatively, the
scriptable wrapper only allows interaction with an "unsafe" object from trusted
code.)

(As an aside, one problem I see here is that QI() from script could cause a
"safe" native object to create an aggregate as a side effect.  Creation of the
aggregate may have further unexpected side effects that would need to be audited
for security problems.  But that's probably not relevant to the discussion at hand.)

Anyway, if this is the case, then you'd only need a security check at the point
in nsRDFResource::GetDelegate where it's about to create the delegate (to avoid
the "object creation as an attack" problem.)  Since delegate creation is an
expensive process anyway (component manager, etc.), adding a security check here
seems reasonable, and probably preferrable to splitting the interface.

Comment 15

[Axel Hecht]

Attachment: add class info to rdf, take 1

I can mail with this, I can edit bookmarks, I can read news.
The patch does:
- add class info for RDFContainerUtils, RDFBlob, RDFDate, RDFInt, RDFLiteral,
RDFResource, RDFInMemoryDataSource, RDFXMLDataSource,
RDFXMLCompositeDataSource,
RDFService, RDFXMLParser, RDFXMLSerializer.
(nsIRDFXMLSink is not exposed by RDFXMLDataSource, as that is just for loading 

it internally, AFAICT)
- add js constructors for RDFContainerUtils, RDFInMemoryDatSource,
RDFCompositeDataSource, RDFXMLParser, RDFXMLSerializer and RDFService.
RDFService is a constructor despite the fact that it's a service due to js
clashes between the js prototype and a global property. But the service is 
really a service now.
Added security checks for RDFService::GetDataSource[Blocking] and disabled
those as well as RDFService::RegisterResource and RegisterDataSource in all.js
capability.policy.default.
I didn't use external DOMCI for this, because nsRDFResource is used as static
lib in some parts, which makes it impossible to use the macros. So instead
of rewriting stuff, I went for hacking it into nsDOMClassInfo.cpp.

Comment 16

[Axel Hecht]

taking

Comment 17

[Axel Hecht]

Comment on attachment 120440 [details] [diff] [review]
add class info to rdf, take 1

requesting r/sr by Mitchell for scripting security and jst for class info

Comment 18

[Axel Hecht]

adding from my smoketesting, I also created a new account, configured the 
sidebar and navigated using the history window

Comment 19

[Johnny Stenback (:jst)]

I'm happy to sr this patch, but before this is checked in, we need to have a
clear understanding of the security implications of this change. Cc:ing Heikki.
Should we organize a security review for this change before it's checked in?
What does it mean to make RDF scriptable from webpages from a security point of
view? Axel, would you be available to participate in a security review (over the
phone) if one was to be arranged?

Comment 20

[Chris Waterson]

Comment on attachment 120440 [details] [diff] [review]
add class info to rdf, take 1

r=waterson, for what it's worth.  Definitely worth getting mstoltz to look at
it, too, tho.

Comment 21

[Axel Hecht]

I'd be happy to participate in a security review, given that it should be half
way compatible with european timezones (and jsts, which is gonna be hard ;-))

Issues: loading datasources (file based and rdf: internal)
resources are shared (as are the literals, but I don't see a problem there)
one resource pops up in multiple environments and as nsRDFResource is used all
over in mailnews, bookmarks, file, we need to make sure we don't expose private
data thru those.

XXX review comment to self, I need toblock GetDelegate and ReleaseDelegate in 
all.js

Comment 22

[Axel Hecht]

Heikki, jst intended to CC you about a security review for this bug.
Actually doing that.

Comment 23

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Yes, we definitely will want a security review of this stuff, and the RDF module
in general. I will try to organize something in private emails, hopefully for
next week. I will be including the usualy suspects at Netscape and Axel. If
someone else is interested, please send me private email and I'll include you as
well.

This change looks so big and scary that I doubt we would take it for 1.4, so
might as well do this properly...

Comment 24

[Axel Hecht]

I have these changes to all.js in my tree and smoketested them as before.
(+ testing a filter in news)
sorry for that.
+pref("capability.policy.default.RDFResource.GetDelegate", "noAccess");
+pref("capability.policy.default.RDFResource.ReleaseDelegate", "noAccess");
+pref("capability.policy.default.RDFXMLDataSource.FlushTo", "noAccess");

Comment 25

[Axel Hecht]

Comment on attachment 120440 [details] [diff] [review]
add class info to rdf, take 1

new patch needed per security review

Comment 26

[Axel Hecht]

This bug would be easier if bug 211801 was fixed, but it doesn't really depend.

Comment 27

[Axel Hecht]

Attachment: the state of my tree

This is the patch that I backed out of my tree. I don't see that the security
stuff is moving anywhere, so I get the changes out until I see movement on the
political front again.

Comment 28

[Marius Andreiana]

re comment #26 -- bug 210141 being fixed, does the patch need to be modified?

Comment 29

[Axel Hecht]

(In reply to comment #28)
> re comment #26 -- bug 210141 being fixed, does the patch need to be modified?

comment #26 doesn't have anything to do with bug 210141, and the attached patch is
adjusted to that bug being fixed. Of course, the patch is not at all the state of
my mind anymore. We want to fix the RDF API completely before we attempt to expose
it once again.