Discussion: Prefer passing MethodHandles.Lookup over @CallerSensitive

[Johannes Kuhn]

Hi,

to reiterate: I do not want to change existing APIs.
Instead having a discussion that can guide the introduction of new APIs 
that may want to be @CS (but don't need to be).

On 21-Apr-23 21:30, mandy.chung at oracle.com wrote:
> 
> On 4/21/23 6:44 AM, Johannes Kuhn wrote:
>> Hi,
>>
>> Instead of making new and existing methods @CallerSensitive, such 
>> methods could instead take a MethodHandles.Lookup as method argument.
>>
>> Library authors are often told that they could ask their users to pass 
>> a MethodHandles.Lookup to obtain internal access instead of using 
>> things like AccessibleObject::setAccessible.
>>
>> I hope OpenJDK would follow that advice as well, IMHO the broader use 
>> of MethodHandles.Lookup would make things easier for both OpenJDK 
>> deverlopers & library authors.
>>
>> tl;dr: Have one @CallerSensitive method to rule them all: 
>> MethodHandles.lookup()
>>
> Short answer is: It depends.  The platform already has such an example: 
> ConstantDesc::resolveConstantDesc takes the Lookup parameter as the 
> resolution and access control context.
> 
> Long answer is: caller-sensitiveness of @CS methods is used in several 
> categories.  It all depends.
> 
> 1. security permission check: bypass SecurityManager permission checks 
> depending on the immediate caller's class loader.  Majority of @CS 
> methods are in this category.   This includes 3-arg Class::forName, 
> Class::getClassLoader, ClassLoader::getSystemClassLoader, 
> java.lang.reflect APIs as well as APIs that call these @CS methods on 
> behalf of the caller such as 
> javax.sql.rowset.serial.SerialJavaObject::getFields. >
> We should not introduce any new APIs in this category.

I agree on this.

> 
> 2. Access check: core reflection API e.g. Field::setInt
> 
> 3. Caller's context as the default: this includes 1-arg Class::forName, 
> Resource::getBundle, ClassLoader::registerAsParallelCapable, 
> Package::getPackages
> 
> a) Existing APIs already take a context parameter.   For example, non 
> 1-arg variants of Class::forName and Resource::getBundle already take 
> the ClassLoader parameter to provide the context.   A variant to take 
> Lookup parameter for this category is unnecessary and redundant.
> 
> b) For APIs that don't provide a variant to take the context parameter, 
> it may be reasonable to define a variant to take a context parameter 
> (Lookup or other type more appropriate and align with the existing APIs).

Again, it is not my goal to change existing behavior - instead having a 
discussion about this to guide *new* APIs if they should be @CS or take 
the context as parameter.

> 
> For this subcategory, I agree the APIs should take the context 
> parameter.   Lookup object would be appropriate if it's for access 
> control context.   In addition, defining an API deriving the context 
> from the caller should carefully be evaluated and justified.
> 
> 4. Validate APIs being called by a valid caller: IllegalCallerException 
> thrown if an illegal caller is detected. This includes Module::addReads, 
> addExports, and java.lang.foreign APIs etc.  Making these APIs to take a 
> Lookup parameter is less easier to use.
> 

Those are already existing APIs.

As example, John Rose suggested in his "Value type companions, 
encapsulated"[2] the addition of more @CS methods - this is where I see 
the value of a discussion beforehand.

>> -----
>>
>> 1. Conceptually a @CallerSensitive method has a hidden parameter - the 
>> caller class, which does not appear in source- or bytecode. A 
>> MethodHandles.Lookup parameter is explicit.
>>
>> 2. Most @CallerSensitive methods are listed in the Secure Coding 
>> Guidelines for Java SE[1]. When adding new @CallerSensitive methods it 
>> should always be considered if those methods need to be listed in that 
>> document as well.
>> This list should also not increase indefinitely, as each use of a 
>> method listed there needs to be carefully checked to avoid security 
>> vulnerabilities.
>> Using a method taking a MethodHandles.Lookup needs the same scrutiny - 
>> but because of the explicit nature this is evident from looking at 
>> use-site.
> 
> 
> These @CS methods are the ones that bypass SecurityManager permission 
> checks depending on the immediate caller's class loader.  We should not 
> add any new CS method adding to this list.

There are several categories, for ClassLoader, Module...
The SecurityManager is only related for a subset of them.
(Strictly speaking, restricted methods from panama may also fall under 
ACCESS-14)

> 
>> 3. Methods that take an explicit MethodHandles.Lookup compose better 
>> with other methods that do the same - for example, a library could 
>> pass the lookup it received from its client to a core API, thereby 
>> acting on behalf of the client.
>>
>> 4. When a @CallerSensitive method is looked up from 
>> MethodHandles.Lookup, it needs to be bound to the caller.
>> This binding can be expensive in the worst case (load of an hidden 
>> class + erasing the signature to ([Ljava/lang/Object;)Ljava/lang/Object;)
>>
> Yes but one hidden class per each caller class.  This overhead only 
> applies to MethodHandle::invokexxx and Method::invoke on a @CS method 
> which does not have an CS adaptor (see @CallerSensitiveAdaptor).
> 
> This overhead is not relevant to the bytecode invocation of @CS methods 
> such as Class::forName.
> 

Yes, this is all true - but there is still overhead.
It is simply an argument why @CS is more complicated (and maybe should 
be avoided for future APIs?)

>> 5. Making existing methods @CallerSensitive can lead to small 
>> backwards compatibility issues - as the public lookup can't lookup 
>> those methods anymore.
>>
> Can you explain what you observe about this?   Public lookup has access 
> to public members of public classes in packages that are exported 
> unconditionally.    @CS should have no impact to public lookup.
> 
> 

This line works with Java 7 - 16:

     MethodHandles.publicLookup().findStatic(System.class, 
"setSecurityManager", MethodType.methodType(void.class, 
SecurityManager.class));

In Java 17 System::setSecurityManager is now @CS - for a good reason - 
but such a change can break existing code, which should be considered.

>> -----
>>
>> A @CallerSensitive method have one benefit over passing a 
>> MethodHandles.Lookup: It has a nicer API.
>> This might be fine for the restricted methods that Panama introduces.
>>
>> New methods that need to take the caller into account could for 
>> example add instead an overload that takes an explicit 
>> MethodHandles.Lookup as argument. The one without that argument could 
>> work as if `MethodHandles.publicLookup()` was passed to the other.
>>
>> -----
>>
>> Closing words:
>> This is my personal opinion, and might be totally wrong.
>> For now, my goal is just to start a discussion.
>>
>> I explicitly don't ask about changing existing @CallerSensitive methods.
> 
> 
> Good.  The categories I described above hope will give more context and 
> explain the considerations to have for new APIs.
> 
> Mandy
> 
>> - Johannes
>>
>> [1]: 
>> https://www.oracle.com/java/technologies/javase/seccodeguide.html#9-8

[2]: https://cr.openjdk.org/~jrose/values/encapsulating-val.html