A Bug involving MethodHandles, Nestmates, Reflection and @CallerSensitive

[Mandy Chung]

Thanks David.  I was about to create one.

This is indeed a gap in injecting this trampoline class for supporting 
@CallerSensitive methods.  The InjectedInvoker ensures that the 
InjectedInvoker class has the same class loader as the lookup class.  
W.r.t. your patch, it seems okay but I have to consider and think 
through the security implication carefully.

You mentioned "Some old software loves to set static final fields 
through reflection" - can you share some example libraries about this?   
This is really ugly hack!!

Mandy

On 12/7/20 8:13 PM, David Holmes wrote:
> Hi Johannes,
>
> I've filed this bug report for you:
>
> https://bugs.openjdk.java.net/browse/JDK-8257874
>
> Thanks,
> David
>
> On 8/12/2020 11:20 am, Johannes Kuhn wrote:
>> Let's start with the reproducer:
>>
>>      public class NestmateBug {
>>          private static int field = 0;
>>          public static void main(String[] args) throws Throwable {
>>              Lookup l = MethodHandles.lookup();
>>              Field f = NestmateBug.class.getDeclaredField("field");
>>              MethodHandle mh = l.findVirtual(Field.class, "setInt", 
>> methodType(void.class, Object.class, int.class));
>>              int newValue = 5;
>>              mh.invokeExact(f, (Object) null, newValue);
>>          }
>>      }
>>
>> This throws a IAE in the last line:
>>
>> class test.se15.NestmateBug$$InjectedInvoker/0x0000000800bb5840 (in 
>> module test.se15) cannot access a member of class 
>> test.se15.NestmateBug (in module test.se15) with modifiers "private 
>> static"
>>      at 
>> java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:385) 
>>
>>      at 
>> java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:693) 
>>
>>      at java.base/java.lang.reflect.Field.checkAccess(Field.java:1096)
>>      at java.base/java.lang.reflect.Field.setInt(Field.java:976)
>>      at test.se15/test.se15.NestmateBug.main(NestmateBug.java:17)
>>
>> The reason for this behaviour is:
>> * Field.setInt (without setAccessible) requires that the caller class 
>> is in the same nest for private members.
>> * Field.setInt is @CallerSensitive
>> * MethodHandles will bind to the caller by injecting an invoker for 
>> @CallerSensitive methods.
>> * The injected invoker is NOT a nestmate of the original lookup class.
>> * The access check of Field.setInt fails - as the injected invoker is 
>> now the caller.
>>
>> This is important because:
>> * Some old software loves to set static final fields through reflection.
>> * To do that, it usually hacks into Field.modifiers. Which is 
>> filtered iirc since Java 12.
>> * I write Java agents to fix such things - removing final from static 
>> fields, and intercept Class.getDeclaredField and Field.setInt by 
>> replacing it with an invokedynamic instruction.
>> * The original target is passed as a MethodHandle bootstrap argument. 
>> In the case of Field.setInt, it is guarded with a 
>> MethodHandles.guardWithTest() - calling the original target if the 
>> Field is not my canary.
>>
>> Suggested fix:
>> * Make the injected invoker a nestmate of lookup class.
>>
>> I did prepare a fix for that [1], but AFAIK the bug first needs to be 
>> filled in the bug tracker.
>> And I don't have an account.
>>
>> - Johannes
>>
>>
>> [1]: 
>> https://github.com/DasBrain/jdk/commit/3c4bb20c8e4cd9086e934128e5cb085a5cfbdb94