Discussion: Prefer passing MethodHandles.Lookup over @CallerSensitive

[mandy.chung at oracle.com]

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.

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).

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.

> -----
>
> 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.

> 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.

> 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.


> -----
>
> 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