337723 - Use of non-scriptable interfaces in scriptable interfaces

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[jhp (no longer active)]

While trying to use xpidl for generating Java interfaces for JavaXPCOM, I ran into issues where scriptable interfaces inherit from or use non-scriptable interfaces.  For JavaXPCOM, I only expose scriptable interfaces (exceptions made for nsIAppShell).  But this makes it very hard when using xpidl since it doesn't always know whether a given interface is scriptable.

For now, I get around this by generating the Java interfaces with a utility based around nsIInterfaceInfoManager, which has all the information to determine how to write out all interfaces.  But using xpidl would be more ideal (bug 333618), so it would be good to fix these interfaces.

Here is the current list of grievances:

* imgIDecoderObserver inherits from non-scriptable imgIContainerObserver.
* nsIScriptSecurityManager inherits from non-scriptable nsIXPCSecurityManager.
* nsIWindowCreator2 inherits from non-scriptable nsIWindowCreator.
* An nsIJMVManager method returns non-scriptable nsIPrincipal.
* An nsISocketProviderService method returns non-scriptable nsISocketProvider.
* An nsIWSPInterfaceInfoService method uses non-scriptable nsIInterfaceInfoManager and nsIInterfaceInfo.

There may be more, but that's what I have for now.

Comment 1

[jhp (no longer active)]

I don't think scriptable interfaces should inherit from non-scriptable ones, so either the parent is made scriptable, or the child is made non-scriptable.

Half the methods of nsIScriptSecurityManager are [noscript], and of the remainder, many use the non-scriptable interface nsIPrincipal.  So this interface should be non-scriptable.

nsIWindowCreator2 is used in JavaScript, but only for its constant.  Its only method is [noscript].  bsmedberg suggested that nsIWindowCreator should be scriptable.  But if we do that, then we should drop the [noscript] from the method in nsIWindowCreator2, since the methods of both interfaces are very similar.  Is there any reason to keep these non-scriptable?

Not sure what to do about imgIDecoderObserver and imgIContainerObserver.

nsISocketProviderService is just a getter for nsISocketProvider, so it should probably be made non-scriptable.

nsIWSPInterfaceInfoService takes nsIInterfaceInfoManager as a param and returns nsIInterfaceInfo in its only method, so it too should probably be made non-scriptable.

Comment 2

[jhp (no longer active)]

OK, bz specifically made nsIPrincipal scriptable on the trunk (bug 327242), and changed some methods in nsIScriptSecurityManager, so that should stay scriptable.  But how to fix this on the branch?

Comment 3

[Boris Zbarsky [:bzbarsky]]

On the branch things are hard, because of the no interface changes thing...  :(  Though perhaps just changing whether methods are scriptable or noscript might be ok?

On trunk, please file bugs on the relevant modules and request blocking of 

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er,

On trunk, please file bugs on the relevant modules and request blocking of 1.9a2 or something?  For example, the inheritance of the security manager interfaces should probably just be reversed.  Not sure about the other stuff.

In general, imo xpidl should error, not warn, when a scriptable interface inherits from a non-scriptable one.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> So this interface should be non-scriptable.

You can't do that on branch -- it would break the whole UI.  Sorry.  ;)

> Not sure what to do about imgIDecoderObserver and imgIContainerObserver.

File bugs on imagelib.

Comment 6

[jhp (no longer active)]

Opened bugs for the 3 inheritance issues, and for making xpidl throw an error.  See the bug dependency list.

Comment 7

[jhp (no longer active)]

bz, bug 327242 made nsIPrincipal scriptable, with most methods set as [noscript].  Since that patch didn't actually change the binary layout of the interface, is there any chance we could take the nsIPrincipal.idl changes for the 1.8 branch?

Comment 8

[Boris Zbarsky [:bzbarsky]]

That change changed the IID.  So no.

Quite apart from the fact that the change is not proven completely safe from a security point of view.  I'll make sure to resolve the outstanding issues for 1.9, but I'm not willing to risk things on 1.8.

Comment 9

[jhp (no longer active)]

Actually, only nsIScriptSecurityManager's IID changed.  I was talking about taking only the nsIPrincipal change, which didn't change its IID.  If you still think we shouldn't take it, I'll open a new bug for creating a branch workaround.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> I was talking about taking only the nsIPrincipal change, which didn't change
> its IID.

The very first hunk of https://bugzilla.mozilla.org/attachment.cgi?id=212186 is the IID change in nsIPrincipal.  Same for http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/caps/idl&command=DIFF_FRAMESET&file=nsIPrincipal.idl&rev2=1.35&rev1=1.34

Or am I missing something?

Comment 11

[jhp (no longer active)]

Ah, sorry, I was only talking about the number field, which gets changed when the binary layout of the interface changes.  This goes back to the discussion we're having in one of the other bugs: are we allowed to change the scriptability of an interface on the branch?  In the discussions I've had with bsmedberg and darin, they didn't see a problem with it, as long as the interfaces remain binary compatible, and changing the scriptability didn't cause any other problems.

Comment 12

[Darin Fisher]

non-scriptable -> scriptable (ok)
scriptable -> non-scriptable (not-ok)

in some cases, making something scriptable that was not previously could have some unexpected security concerns if the interface is reachable from content JS.

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #12)
> in some cases, making something scriptable that was not previously could have
> some unexpected security concerns if the interface is reachable from content
> JS.

I don't think we guarantee, when freezing an interface, that we will not add additional callers to it, so this doesn't seem like an issue with the scriptability of the interface as much as with what the behaviour of those new callers are.  And [scriptable] should not be used as a security barrier, to boot!

Comment 14

[Boris Zbarsky [:bzbarsky]]

It should not, I think we all agree on that.  But in the case of nsIPrincipal it most certainly _is_ -- ther are no provisions to prevent random script callers from mutating parts of the principal's identity, for example.  See bug 339821 and bug 339822 (based on about 5 minutes of auditing of the interface and code; there might well be more).