On Thu, 6 Apr 2023 19:33:51 GMT, Johannes Kuhn <jk...@openjdk.org> wrote:
>> This is good work. This needs some careful study on the security aspect. >> I see the proxy class is in the same module of the interface, is non-public, >> and it has a no-arg non-public constructor. >> >> Defining the proxy class in the same module would grant this class >> reflective access to any class in this module. This seems a concern if the >> target method handle should not have access to the class in that module. >> >> W.r.t. the no-arg constructor, I would consider another level of defense to >> make the constructor to take `Lookup` of itself and throw an IAE if the >> lookup class is not itself or it does not have `ORIGINAL` access. > >> This seems a concern if the target method handle should not have access to >> the class in that module. > > MethodHandle access is checked when the MethodHandle is created. > For `@CallerSensitive` methods, the MethodHandle is additionally bound to the > lookup class. > Also see > [`java.lang.invoke.ConstantBootstraps.invoke`](https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/lang/invoke/ConstantBootstraps.html#invoke%28java.lang.invoke.MethodHandles.Lookup,java.lang.String,java.lang.Class,java.lang.invoke.MethodHandle,java.lang.Object...%29). > If calling an arbitrary MethodHandle from a module could be a problem, then > that is an easier target. > >> W.r.t. the no-arg constructor, I would consider another level of defense to >> make the constructor to take Lookup of itself and throw an IAE if the lookup >> class is not itself or it does not have ORIGINAL access. > > Without that defense you can create a new instance (if you have a reference > to the hidden class) that does exactly the same thing as the original > instance. > Other than `getClass` on the original object, `StackWalker` comes to mind as > a way to obtain the class reference. This patch is ready for review. WrapperInstance change was to address the annotation security concerns raised by @DasBrain. If there's more security implications with null classloader, I will swiftly update this patch as well. ------------- PR Comment: https://git.openjdk.org/jdk/pull/13197#issuecomment-1499781366
