8013527: calling MethodHandles.lookup on itself leads to errors #2367

mlchung · 2021-02-03T01:50:36Z

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:

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issues

JDK-8013527: calling MethodHandles.lookup on itself leads to errors
JDK-8257874: MethodHandle injected invoker doesn't have necessary private access

Reviewers

Johannes Kuhn (@DasBrain - Author)

Contributors

Johannes Kuhn <info@j-kuhn.de>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2367/head:pull/2367
$ git checkout pull/2367

bridgekeeper · 2021-02-03T01:50:53Z

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2021-02-03T01:51:41Z

@mlchung The following labels will be automatically applied to this pull request:

core-libs
hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlchung · 2021-02-03T02:39:52Z

/label remove hotspot-compiler

openjdk · 2021-02-03T02:40:12Z

@mlchung
The hotspot-compiler label was successfully removed.

mlchung · 2021-02-03T02:43:50Z

/contributor add Johannes Kuhn info@j-kuhn.de

openjdk · 2021-02-03T02:44:07Z

@mlchung
Contributor Johannes Kuhn <info@j-kuhn.de> successfully added.

mlbridge · 2021-02-03T03:26:05Z

Webrevs

01: Full - Incremental (228d29f)
00: Full (3764fd0)

mlchung · 2021-02-03T03:32:02Z

/issue add JDK-8257874

mlchung · 2021-02-03T03:32:39Z

/issue add JDK-8013527

openjdk · 2021-02-03T03:32:40Z

@mlchung
Adding additional issue to issue list: 8257874: MethodHandle injected invoker doesn't have necessary private access.

openjdk · 2021-02-03T03:33:30Z

@mlchung This issue is referenced in the PR title - it will now be updated.

mlchung · 2021-02-03T03:34:47Z

/label remove hotspot-compiler

openjdk · 2021-02-03T03:35:05Z

@mlchung The hotspot-compiler label was not set.

DasBrain

Thanks Mandy.

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

DasBrain · 2021-02-03T10:52:41Z

src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java

+            }
+            Class<?> invokerClass = new Lookup(targetClass)
+                    .makeHiddenClassDefiner(name, INJECTED_INVOKER_TEMPLATE, Set.of(NESTMATE))
+                    .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.

I don't follow which previous commit you refer to more dangerous. Please elaborate. I don't see any security concern with class data.

Last night, I had a second thought that the fix for these two JBS issues does not need the class data. It's more for a future use. I plan to revise it and drop class data in this fix anyway.

You are right, got it confused with the future use.

With this fix, MethodHandle -> Method.invoke -> MethodHandles.lookup() will still return a lookup on the injected invoker.
I somehow missed that this is not part of the fix, but for the future use.

MethodHandle -> Method.invoke -> MethodHandles.lookup() is a corner case that can be fixed easily using the class data approach. See the new commit.

The security issue I mentioned was in an other branch, method-invoke.

I used commit mlchung@4a3c914 (i.e. before strengthening the injected invoker checks) to test the my exploit. (Yes, full sandbox escape.)

I hope the same is not possible with the nestmate requirement.

PS.: Hidden Class -> MethodHandle -> Method.invoke -> MethodHandles might break due to mangling of the hidden class name for the injected invoker. Will write a test.

Well, my branch method-invoke is a prototype and work-in-progress. I won't dig too much to it.

Do you reproduce the issue with this patch? The fix will ensure it's the invoker class injected by BindCaller.

DasBrain

Looks good.

bridgekeeper · 2021-03-03T23:55:54Z

@mlchung This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2021-04-21T12:50:06Z

@mlchung This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

mlchung added 2 commits February 2, 2021 10:15

JDK-8013527: calling MethodHandles.lookup on itself leads to errors

c72844b

clean up

cb71b08

openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Feb 3, 2021

mlchung marked this pull request as draft February 3, 2021 02:04

openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 3, 2021

mlchung added 2 commits February 2, 2021 19:16

revise the helper method

cc372b4

clean up whitespace

3764fd0

mlchung marked this pull request as ready for review February 3, 2021 03:22

openjdk bot added the rfr Pull request is ready for review label Feb 3, 2021

DasBrain suggested changes Feb 3, 2021

View reviewed changes

Support MethodHandles::lookup called via Method::invoke via MethodHandle

228d29f

DasBrain approved these changes Feb 3, 2021

View reviewed changes

bridgekeeper bot closed this Apr 21, 2021

8013527: calling MethodHandles.lookup on itself leads to errors #2367

8013527: calling MethodHandles.lookup on itself leads to errors #2367

Uh oh!

Conversation

mlchung commented Feb 3, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Contributors

Download

Uh oh!

bridgekeeper bot commented Feb 3, 2021

Uh oh!

openjdk bot commented Feb 3, 2021

Uh oh!

mlchung commented Feb 3, 2021

Uh oh!

openjdk bot commented Feb 3, 2021

Uh oh!

mlchung commented Feb 3, 2021

Uh oh!

openjdk bot commented Feb 3, 2021

Uh oh!

mlbridge bot commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mlchung commented Feb 3, 2021

Uh oh!

mlchung commented Feb 3, 2021

Uh oh!

openjdk bot commented Feb 3, 2021

Uh oh!

openjdk bot commented Feb 3, 2021

Uh oh!

mlchung commented Feb 3, 2021

Uh oh!

openjdk bot commented Feb 3, 2021

Uh oh!

DasBrain left a comment

Choose a reason for hiding this comment

Uh oh!

DasBrain Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlchung Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlchung Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DasBrain Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

mlchung Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

DasBrain Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

mlchung Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

DasBrain left a comment

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Mar 3, 2021

Uh oh!

bridgekeeper bot commented Apr 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

mlchung commented Feb 3, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 3, 2021 •

edited

Loading

DasBrain Feb 3, 2021 •

edited

Loading

mlchung Feb 3, 2021 •

edited

Loading

mlchung Feb 3, 2021 •

edited

Loading