Closed Bug 32150 Opened 25 years ago Closed 24 years ago

Scriptable Plugin methods/members need to be accessable from JavaScript

Categories

(Core Graveyard :: Plug-ins, defect, P3)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Other
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED DUPLICATE of bug 38495

People

(Reporter: rusty.lynch, Assigned: mike+mozilla)

References

Details

(Whiteboard: [nsbeta2-])

Attachments

(3 files)

zip with 2 new files and a diff for plugin/test sample
3.76 KB, application/octet-stream
Details
Teach XPConnect to look up the proto chain
985 bytes, patch
Details | Diff | Splinter Review
magic GetScriptObject for EmbedElement; assumes an nsIScriptablePlugin interface
4.97 KB, patch
Details | Diff | Splinter Review 
The new XPCom plugins need to make their scriptable methods/members directly
accessable from JavaScript.  If I understand correctly, the following should work:
----------
<embed name="foo" type="application/X-foo" width="100" height="100">
<script>
// to get to the foo object...
var myfoo = document.getElementById('foo');

// to call a scriptable bar() method
myfoo.bar();

-----------------
I have personally been getting around this in my in-house plugins by doing
a little mojo in my plugin instance's factory, and then doing something like:

----------
<embed name="foo" type="application/X-foo" width="100" height="100">
<script>
var myfoo = Components.classes["components://some/path/foo-control"]
                      .createInstance();
myfoo = myfoo.QueryInterface( Components.interfaces.snIPluginInstance );

// and now ...
myfoo.bar();
-----------

This requires security to be turned off for checkxpconnect and a few unhealthy
assumptions in the factory. (In short, it was just a temp work around.)

So, is someone already working on this?  Am I correct in how plugin methods
should be accessed?
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.
The problem with that solution is that it's not backward compatible with 
existing JavaScript APIs for plugins, which is an installed base problem. All 
pages that used the Beatnik plugin would need to be revised to a new (and much 
clunkier) API version. A way to fix this and preserve existing APIs needs to be 
worked out.
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 checked in the DOM changes I proposed above.

I also just a attached a zip to this bug containing updates for the sample in 
plugin/test. I don't want to check this in right away because I know 
av@netscape.com is working on this sample code stuff. I'm attaching it in case 
anyone wants to give it a try. I sent off the attachement and the following note 
to av to request his checkin approval...

av,

Attached is a zip with a diff (-u) and two new files that I'd
like to checkin to plugin/test.

This demostrates calling the scriptable interfaces on the sample
plugin from JavaScript. It adds an idl file and a new .html
sample file. It makes the SimplePluginInstance implement
nsIPluginInstance and the new nsISimplePluginInstance (declared
in the .idl file in the zip).

To run it: build it, copy the .dll to the plugins dir and the
.xpt file created by the build from _xpidlgen to the
bin/components directory, and then load the new html file as a
file:/// url in mozilla.

Can I check this in or will it conflict with the work you are
doing?

Thanks,

John.
With the flattening described in 24695, this should do most of what we need for 
the Beatnik plugin, which is heavily JavaScript dependent. Two problems remain, 
though.

First, security certificates would be very problematic. If users get a security 
certificate warning every time they go into a Beatnik-enabled web page, that 
will cause our use to plummet. Access to methods exposed by plugins does not 
require any special security measures now and adding them would cause all 
scriptable plugins to suffer.

(If the certificate needs to be provided by the adopting web site rather than 
our central web site, that would be even worse. Our developer adoption would 
essentially stop cold due to the high barriers to providing security 
certificates.)

Second, XPIDL does not seem to have the full set of overriding behaviors 
supported by LiveConnect, and our existing JavaScript interface for both 
existing plugins and ActiveX makes heavy use of method overloading. Without 
that, we will have to revise our JavaScript implementations to support Mozilla, 
which requires each and every outside developer who has adopted Beatnik to 
revise their web pages.

The steps already taken look excellent but I would like to urge you to shoot as 
close as possible to 100% compatibility with the existing plugin scriptability 
due to these usage and adoption issues.

Thanks,
Tim Maroney
Beatnik 
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. tim@maroney.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.

It appears to me from jband's comments that this core bug (plug-in methods not 
accessible from JavaScript) is now FIXED by his check-ins. I'm marking this bug 
FIXED on that basis; engineering please correct me if I'm wrong on this.

Also nominating nsbeta2 because if this bug *isn't* FIXED, it's gotta be by 
nsbeta2.

Braden, feel free to open a separate enhancement request bug for each of the 
security enhancements you requested in this description. Please mark each one 
severity enhancement, keyword helpwanted, and milestone FUTURE if you do, as 
Netscape doesn't have the resources to commit to provide enhancements to the 
plug-in security model by FCS beyond what was in Nav4.x.
Status: NEW → RESOLVED
Closed: 24 years ago
Keywords: nsbeta2
Resolution: --- → FIXED
Reopening.

We're not there yet for backward compatibility; plugins need to be scriptable
from content javascript via the legacy plugin scripting API, without any
intervening nsIFoo accesses.

The current plan is to discover the scriptable interface exposed in the plugin
(via 39911, which this bug depends on) and then expose the plugin to JavaScript
with that interface, via XPConnect.

An earlier problem with this approach is that we don't want to lose the
node/element methods exposed on the plugin object in the DOM.  Vidur suggested
following the example of Liveconnect, and setting the original DOM node as the
javascript .prototype object of the new XPConnect-reflected object.  Both sets
of methods are then visible on the object from JavaScript.

Attachments to follow.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Setting 39911 as a dependency, reassigning to myself.
Assignee: rusty.lynch → mccabe
Status: REOPENED → NEW
Depends on: 39911
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-]
Whiteboard: [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
Closed: 24 years ago → 24 years ago
Resolution: --- → DUPLICATE
verified dup

mass duplicate verifications . For filtering purposes, pls use keywd
"massdupverification"
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: