349807 - Would be nice to have a way to annotate interface methods as requiring privs

Status: NEW

(Core :: XPConnect, defect)

Description

[Boris Zbarsky [:bzbarsky]]

What I would basically like to do is write the following in my IDL:

[UniversalXPConnect] void init(in nsIPrincipal principal,
                               in nsIURI documentURI,
                               in nsIURI baseURI);

And have XPConnect do the relevant security check when calling this method.  Then I can have callers from C++ not get security-checked, while JS callers get security-checked.  I'm basically faking this with classinfo and a noscript idl method, but that requires ~100 lines of code and doesn't work in xpcshell.

I can almost do this with nsISecurityCheckedComponent except my class is a DOM class (which I could change, I suppose) and with nsISecurityCheckedComponent there is no way to do sameOrigin (which is a deal-breaker).  Though maybe we should add the ability to do that, but then I still need almost as much code as I have now, because I have to implement all of nsISecurityCheckedComponent just to control calling of one single method....

Comment 1

[Johnny Stenback (:jst)]

I think flagging capability requirements (or any security related requirements) in an interface is the wrong place to do it. An interface is only an interface, it says very little about the various implementations and specific requirements thereof. I don't off the top of my head have a clean alternative for how to do this w/o doing what we have now, or what you're proposing, but it feels wrong to me to stick this in the IDL.

This seems to be something that belongs in classinfo, as it's the class that dictates what's safe and what's not. Maybe additional functionality in nsIClassInfo2 or whatnot would be a better way to go here?

Comment 2

[Benjamin Smedberg]

Wow, I couldn't disagree more completely. Flagging at least that a method does or doesn't do security checks should be part of the declared contract of the method (on the interface), instead of an implementation detail. Otherwise you get into a situation where every time you call a method, you have to decide whether to push an appropriate security context based on the concrete type of the object you're calling. This has consistently caused problems where we assume that something does or doesn't do security checks.

I'm not sure I agree that xpconnect should be asked to do the security checks (instead of having a common method of passing security info around), but it might be expedient. I can't think of a good way to annotate this info in XPT without doing a major XPT version number change.

Comment 3

[Boris Zbarsky [:bzbarsky]]

While in many cases this would be an implementation issue, in this case it really is the interface that wants to advertise that the method requires expanded permissions.  Anything else would allow immediate privilege escalation if one gets one's hands on an nsIPrincipal.

Note that XPConnect _already_ does a security check here (CheckPropertyAccessImpl, with CALL as the access type).  The problem is that we have no way to express the setup we want here: we have no way of restricting access on DOM classes more than sameOrigin but less than noAccess, and no way of doing sameOrigin access on non-DOM classes.

If people want to wontfix this bug and have me file a new one requesting a way to do that in general, I can do that.  But we should really solve this problem.  We have at least 3 methods around (and at least one interface, possibly more) that we added as workarounds for this issue.

Again, what I want is a very painless way to take any DOM class and restrict access to one or more methods of it to something stricter than sameOrigin.  I feel that it's reasonable to document any such restrictions in the interface (just like various W3C DOM APIs document security restrictions in the interface).

Comment 4

[Johnny Stenback (:jst)]

Too late to start working on this now, no? If we'd take this it should be done early in a product cycle IMO.

Comment 5

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

We could still make the annotation work, and not act on it in XPConnect or wherever.  It would be a form of documentation, possibly suitable for warnings or other diagnostics if we have time to turn them on.