3.76 KB, application/octet-stream
985 bytes, patch
|Details | Diff | Splinter Review|
4.97 KB, patch
|Details | Diff | Splinter Review|
I think you need <embed id="foo"...> for document.getElementById("foo") to find the plugin. Then we need the DOM glue code that reflects plugin objects to give up an object that can resolve "bar" to the scriptable method you defined with XPIDL and implemented in your plugin. This sounds like XPConnect, not DOM JS/native interconnection; maybe the usual security checks should apply, in general. But as you say, in 4.x and earlier, we supported then-new-style (Java interface presenting) plugins and allowed JS to call their methods without a generic XPConnect-like security check. Jband, any thoughts? My brain is still warming up to this after helping hyatt with XBL and brutal sharing. /be
Status: UNCONFIRMED → NEW
Ever confirmed: true
I've been looking at this a little and have discussed it with various people. It seems like the thing to do is to add a method to the JS DOM reflection of the plugin that will allow calling QueryInterface on the nsIPluginInstance and will then wrap the result using xpconnect So, you could use xpidl to declare a [scriptable] interface and implement that interface in the same object that you use to implement nsIPluginInstance. From JS you could do something like... var iface = Components.interfaces.myInterface; var bar = document.foo.QueryInterface(iface); // [or document.getElementById('foo').QueryInterface(iface); ???] I think this can be done by adding a pretty minimal amount of code to the DOM EmbedElement stuff. http://lxr.mozilla.org/seamonkey/find?string=EmbedElement We'll have to look at the securty implications. I'm thinking that this could only be allowed in scripts that have asked for and recieived the "UniversalXPConnect" privilege. This requires either signed scripts or the enabling of codebase principles. I'll try to get this working. And see where it leads.
I've implemented (but not yet checked in) something like what I suggested above. Trying to expose "QueryInterface" as a DOM property of nsIDOMHTMLEmbedElement confliced badly with the DOM system (since QueryInterface is a method that all these native xpcom objects already have!). I backed off on that and decided to support the kind of syntax that xpconnect will support on flattened wrappers in the future: document.foo.nsISomeInterfaceName So, if your implemntation of nsIPluginInstance supports nsISomeInterfaceName with a method 'bar' then you can do... document.foo.nsISomeInterfaceName.bar(); or var f = document.foo.nsISomeInterfaceName; f.bar(); This is working. When xpconnect has flattening then we can contemplate reflecting the flattened set of names as properties of document.foo via the nsIJSScriptObject methods. At that time we will still have compatibility and consistenecy with what I have done here. See http://bugzilla.mozilla.org/show_bug.cgi?id=24695 for some discussion of exposing interface names as properties of wrapped object for the purpose of doing a 'cleaner' implicit QueryInterface. It is necessary to do the security dance to make these scriptable calls... netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect"); For file:// urls that is enough. For remote urls script signing will be necessary. If this is not exceptable then we'll have to bring it up as a security issue.
I received an OK from av to checkin my changes to the sample in plugin/test. This completes the work that I was intending to do here. firstname.lastname@example.org voiced some important concerns. I don't know if any plan exists to address those issues. Anyone?
I've opened a new bug ( http://bugzilla.mozilla.org/show_bug.cgi?id=36375 ) to address the concerns raised by the solution to this bug. But before this one (32150) is closed, I'd like to get some feedback from jband, av and anyone else about modifying the solution. Currently, the plugin instance needs to be wrapped before every JS method invocation (see http://lxr.mozilla.org/seamonkey/source/layout/html/content/src/nsHTMLEmbedEleme nt.cpp#338 ). I came across this because my plugin also implements nsIXPCScriptable in order to setup JS method overloading (so that our plugin API is backwards-compatible). nsIXPCScriptable::Create() gets called everytime a wrapper gets created. How about if the object gets wrapped once and then gets stored either in the PluginInstance or in the instance's peer? Then in nsHTMLEmbedElement::GetProperty(), the wrapper can be pulled out of the instance (or its peer). If the wrapper exists, done. Otherwise create and save it. That way only one wrapper per instance ever gets created (if/when needed) and its lifetime can be limited by the instance (or peer) itself rather than by the duration of one particular method invocation.
Sean, Thanks for pointing this out. I started to write the following... -------------------------------------------------------------------------- XPConnect does not build a new wrapper for each method invocation. If a wrapper already exists (within that window) for the given object then that wrapper will be reused. However, if no native objects are rooting the wrapper then its lifetime is controlled by the lifetime of its JSObject - and that is controlled by the JS garbage collector which IMO is run too often in mozilla right now. You can see this by doing something like... var foo = document.simple1.nsISimplePluginInstance; if(foo === document.simple1.nsISimplePluginInstance) dump("same wrapper\n"); else dump("different wrapper\n"); I'm not very excited about having the embed mechanism artifically rooting these wrappers. It would then presumably need to cache and track them. This is a repeated source of bugs in other code. I'm not convinced that there is much to gain. -------------------------------------------------------------------------- ...but when I tried that code snippet the results surprized me! There is a bug in the xpconnect's wrapper reuse code that's only manifested in multiple inheritence cases - like this case where the pluginInstance object also implements another interface which does not inherit from the leftmost interface. It happens that this completely legal but rare in [scriptable] xpcom objects right now. I think that with this fixed you'll agree that additional caching of the wrapper is not necessary. The fix is shown below. I'll check it in as soon as the treee is not flaming. Thanks! John. Index: xpcwrappednative.cpp =================================================================== RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp,v retrieving revision 1.32 diff -u -r1.32 xpcwrappednative.cpp --- xpcwrappednative.cpp 2000/03/31 10:31:00 1.32 +++ xpcwrappednative.cpp 2000/04/20 02:14:40 @@ -214,7 +214,7 @@ if(!rootClazz) goto return_wrapper; - root = new nsXPCWrappedNative(xpcc, realObj, aScope, + root = new nsXPCWrappedNative(xpcc, rootObj, aScope, aGlobalObject, clazz, nsnull); NS_RELEASE(rootClazz);
Agree - thanks for the patch.
I checked in the xpconnect change last night. FWIW the oneline change I posted here was not enough - another paramin that call was also wrong. The right change is in: http://bonsai.mozilla.org/cvsquery.cgi?module=SeaMonkeyAll&branch=HEAD&cvsroot=/ cvsroot&date=explicit&mindate=956232900&maxdate=956233200&who=jband%25netscape.c om
As far as security considerations go, I vote that plug-ins be treated as they were for Netscape 4.x. As a user I think that choosing to downloading and install a given plug-in is enough of a security check.
Choosing to download and install a plug-in is *not* a sufficient security check. Plugins that are perfectly benign when used as intended could become hostile if deliberately misused. Consider a plug-in that can do file management. Script could potentially make it delete files. This would be a perfectly acceptable operation for "trusted" scripts (presumably they'd only delete files under the consent of the user), but giving arbitrary scripts access to a plugin like this could be disastrous. If necessary make it a user preference to allow untrusted scripts to control plug-ins, but such a preference definitely needs to be *off* by default. *** Looking at the way IE5 handles this, I see a preference, "Script ActiveX controls marked safe for scripting." So apparently the burden of trust is shifted from the script to the plugin. This seems like a somewhat less flexible solution, as apparently there is a class of "unsafe" plug-ins that cannot be scripted at all. Shifting the burden of trust to the script code means that all plug-ins can be scripting. What about something in between? Have two classes of plugins: those marked safe for scripting by any script, and those marked safe for scripting only by trusted scripts? I think we'd still need a preference to disable scripting of plugins altogether (for the truly paranoid), since a plugin could "lie" (accidentally, perhaps) about its trust state.
I don't think this needs to be over-engineered. Someone wanting to add functionality can do the plugin scheme or just write an xpcom component and put it in the components directory. If they opt to do a plugin then they are saying that the plugin is taking responsibility for either a) not exposing 'dangerous' scriptable methods b) calling the security system itself to checkup on the caller. For some specialized plugins this might be as simple as checking the codebase of the page in which it is embedded. A plugin that exposes dangerous stuff to *any* caller is itself at fault. It has always been so in the past for plugins, no? This is not an opening of a security hole, it is shifting of responsibility from one chunk of native code to another chunk. Dire warnings to plugin developers are in order, but it does not make sense to me that plugins should be relieved of this traditional freedom.
Dire warnings to users are then what's in order. Mozilla needs to let them know that they are trusting plugin developers not to write plugins that can be used maliciously.
Braden makes a good point: "caveat downloader" means not only must you trust a plugin, you must also trust any scripts that can call its DOM-connected methods. This has been true since LiveConnected plugins shipped in Netscape 3.0, IIRC. If a plugin offers a method that deletes files, and that plugin method fails to check the user's access controls regarding the subject principals asking it to delete a particular file, then you should not trust that plugin. /be
If we want to let users control their own fates to a greater degree (permit them to be more wary, when bewaring as a downloader), we could have the UniversalXPConnect check be pref-controlled. Seems like an easy enough change.
I did make a proposal over at http://bugzilla.mozilla.org/show_bug.cgi?id=36375 to add a specific pref to allow scripting of objects that implement nsIPluginInstance. Follow-ups on the security discussion should probably go there.
I think having a plugin (potential evil) itself and allowing scripts to control plugin methods are the berries of the same field. So I essentially agree with what jband said: it would not make any sense to impose restrictions here. And as always available option, user can use preferences to disable XPConnect like it was with LiveConnect.
But they aren't. Scripts and plugins can (and usually do) come from completely different fields, and the farmers in each can have very different intentions. Disabling XPConnect accross the board is the same kind of solution as "don't connect your computer to the network if you're worried about security." It ought to be possible to allow only trusted scripts to script plugins.
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Setting 39911 as a dependency, reassigning to myself.
Assignee: rusty.lynch → mccabe
Status: REOPENED → NEW
Depends on: 39911
Created attachment 9900 [details] [diff] [review] magic GetScriptObject for EmbedElement; assumes an nsIScriptablePlugin interface
Possible-duplication-of-effort alert: we've opened a separate bug to track the need to enable backward compatibility with calls from JS to plug-in Java APIs via the old "LiveConnect." It's bug 38495, "[FEATURE] need to support JS LiveConnect calls in existing content via upgraded plug-in binaries," which jst is working on. mike--jst is swamped and I'm sure would welcome your help. Would you two please coordinate and then close one of these two reports? Thanks!
Bug 39495 is a Plus bug..percieving this is a duplicate bug, yes? Marking [nsbeta2-]
Dup. bug means nsbeta-? Not sure I follow! Eric is right, 38495 does cover this. Marking closed/duplicate and moving discussion there. *** This bug has been marked as a duplicate of 38495 ***
Status: NEW → RESOLVED
Last Resolved: 18 years ago → 18 years ago
Resolution: --- → DUPLICATE
mass duplicate verifications . For filtering purposes, pls use keywd "massdupverification"
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.