On Wed, 3 Feb 2021 10:52:41 GMT, Johannes Kuhn <github.com+652983+dasbr...@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 > > 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) Only `Lookup` with the original access can access `MethodHandles.classData`. A hidden class `HC$$InjectedInvoker/0x1234` can access private members of another class `C` in the same nest but not `C`'s class data. ------------- PR: https://git.openjdk.java.net/jdk/pull/2367
