Open Bug 122846 Opened 18 years ago Updated Last year
unprivileged access to rdf from scripts
<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?
Probably the RDF/XML datasource, serializer, parser, too. This is definitely worth doing, but probably post mozilla-1.0.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → Future
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.
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.
Another one: The ability to create a new in-memory datasource possibly instantiated with a string (ie via the rdf string parser).
initializing from a string should be done with nsIRDFXMLParser::parseString. IMHO, in-memory datasources have no link to a xml representation.
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 ;-)
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?
Would be nice to get this fixed for documentation purposes too (converting catalog to RDF)
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?
Is your fear that placing a security check in GetDelegate would be a performance problem?
[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.
(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!)
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.
> 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.
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.
Assignee: waterson → axel
Status: ASSIGNED → NEW
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
adding from my smoketesting, I also created a new account, configured the sidebar and navigated using the history window
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 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.
Attachment #120440 - Flags: review?(mstoltz) → review+
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
Heikki, jst intended to CC you about a security review for this bug. Actually doing that.
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...
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 on attachment 120440 [details] [diff] [review] add class info to rdf, take 1 new patch needed per security review
This bug would be easier if bug 211801 was fixed, but it doesn't really depend.
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.
(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.
Assignee: axel → nobody
QA Contact: nobody → core.rdf
Summary: unpriviledged access to rdf from scripts → unprivileged access to rdf from scripts
Assignee: nobody → nobody
Priority: P2 → --
Target Milestone: Future → ---
You need to log in before you can comment on or make changes to this bug.