RFR: 8013527: calling MethodHandles.lookup on itself leads to errors

[Johannes Kuhn]

On Wed, 3 Feb 2021 01:50:36 GMT, Mandy Chung <mchung at openjdk.org> wrote:

> JDK-8013527: calling MethodHandles.lookup on itself leads to errors
> JDK-8257874: MethodHandle injected invoker doesn't have necessary private access
> 
> Johannes Kuhn is also a contributor to this patch.
> 
> A caller-sensitive method can behave differently depending on the identity
> of its immediate caller.  If a method handle for a caller-sensitive method is
> requested, this resulting method handle behaves as if it were called from an
> instruction contained in the lookup class.  The current implementation injects
> a trampoline class (aka the invoker class) which is the caller class invoking
> such caller-sensitive method handle.  It works in all CSMs except `MethodHandles::lookup`
> because the caller-sensitive behavior depends on the module of the caller class,
> the class loader of the caller class, the accessibility of the caller class, or
> the protection domain of the caller class.  The invoker class is a hidden class
> defined in the same runtime package with the same protection domain as the
> lookup class, which is why the current implementation works for all CSMs except
> `MethodHandles::lookup` which uses the caller class as the lookup class.
> 
> Two issues with current implementation:
> 1.  The invoker class only has the package access as the lookup class.  It cannot
>     access private members of the lookup class and its nest members.
> 
>     The fix is to make the invoker class as a nestmate of the lookup class.
> 
> 2.  `MethodHandles::lookup` if invoked via a method handle produces a `Lookup`
>     object of an injected invoker class which is a bug.
> 
> There are two alternatives:
> - define the invoker class with the lookup class as the class data such that
>   `MethodHandles::lookup` will get the caller class from the class data if
>   it's the injected invoker
> - Johannes' proposal [1]: detect if an alternate implementation with an additional
>   trailing caller class parameter is present, use the alternate implementation
>   and bind the method handle with the lookup class as the caller class argument.
> 
> There has been several discussions on the improvement to support caller sensitive
> methods for example the calling sequences and security implication.   I have
> looked at how each CSM uses the caller class.  The second approach (i.e. 
> defining an alternate implementation for a caller-sensitive method taking
> an additional caller class parameter) does not work for non-static non-final
> caller-sensitive method.  In addition, it is not ideal to pollute the source
> code to provide an alternatve implementation for all 120+ caller-sensitive methods
> whereas the injected invoker works for all except `MethodHandles::lookup`.
> 
> I propose to use both approaches.   We can add an alternative implementation for
> a caller-sensitive method when desirable such as `MethodHandles::lookup` in
> this PR.  For the injected invoker case, one could extract the original lookup
> class from class data if needed.
> 
> test/jdk/jdk/internal/reflect/CallerSensitive/CheckCSM.java ensures that
> no new non-static non-final caller-sensitive method will be added to the JDK.
> I extend this test to catch that non-static non-final caller-sensitive method
> cannot have the alternate implementation taking the additional caller class
> parameter.
> 
> This fix for JDK-8013527 is needed by the prototype for JDK-6824466 I'm working on.
> 
> [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-January/073184.html

Thanks Mandy.

Looks good, except the possibility for an attacker to teleport within the same nest.

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1205:

> 1203:             Class<?> invokerClass = new Lookup(targetClass)
> 1204:                     .makeHiddenClassDefiner(name, INJECTED_INVOKER_TEMPLATE, Set.of(NESTMATE))
> 1205:                     .defineClass(true, targetClass);

Using the target class directly could lead to some unintended problems.

An attacker can define it's own hidden class as nestmate and with a name ending in `$$InjectedInvoker`.  
This allows the attacker to "teleport" into a nestmate with full privileges.  
An attacker could then access `MethodHandles.classData`.

Suggested remedy: Create a holder that is only visible to `java.lang.invoke`:

/* package-private */ static class OriginalCallerHolder {
    final Class<?> originalCaller;
    OriginalCallerHolder(Class<?> originalCaller) {
        this.originalCaller = originalCaller;
    }
}

As this type is only visible inside `java.lang.invoke`, it can't be created without hacking into `java.lang.invoke`, at which point all bets are off anyway.

(A previous commit was even more dangerous, as you can force `jlr.Proxy` to inject a class into your package with a `null`-PD)

-------------

Changes requested by DasBrain at github.com (no known OpenJDK username).

PR: https://git.openjdk.java.net/jdk/pull/2367