Note: There are a few cases of duplicates in user autocompletion which are being worked on.

unprivileged access to rdf from scripts

NEW
Unassigned

Status

()

Core
RDF
16 years ago
9 years ago

People

(Reporter: Axel Hecht, Unassigned)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

16 years ago
<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

16 years ago
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
(Reporter)

Comment 2

16 years ago
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

16 years ago
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

16 years ago
Another one:

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

Updated

16 years ago
Blocks: 133695
(Reporter)

Comment 5

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

Comment 6

16 years ago
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)
(Reporter)

Comment 9

15 years ago
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

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

Comment 11

15 years ago
[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

15 years ago
(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!)
(Reporter)

Comment 13

15 years ago
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

15 years ago
> 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.
(Reporter)

Comment 15

15 years ago
Created attachment 120440 [details] [diff] [review]
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.
(Reporter)

Comment 16

15 years ago
taking
Assignee: waterson → axel
Status: ASSIGNED → NEW
(Reporter)

Comment 17

15 years ago
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
Attachment #120440 - Flags: superreview?(jst)
Attachment #120440 - Flags: review?(mstoltz)
(Reporter)

Comment 18

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

Updated

15 years ago
QA Contact: tever → nobody
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

15 years ago
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+
(Reporter)

Comment 21

15 years ago
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
(Reporter)

Comment 22

15 years ago
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...
(Reporter)

Comment 24

15 years ago
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");
(Reporter)

Comment 25

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

new patch needed per security review
Attachment #120440 - Attachment is obsolete: true
Attachment #120440 - Flags: superreview?(jst)

Updated

14 years ago
Blocks: 202393
(Reporter)

Updated

14 years ago
Depends on: 210141
(Reporter)

Comment 26

14 years ago
This bug would be easier if bug 211801 was fixed, but it doesn't really depend.
(Reporter)

Comment 27

14 years ago
Created attachment 130590 [details] [diff] [review]
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

13 years ago
re comment #26 -- bug 210141 being fixed, does the patch need to be modified?
(Reporter)

Comment 29

13 years ago
(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

Updated

13 years ago
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.