VirtualMethod does unprivileged reflection access

[reta]

Description

In OpenSearch, we've updated to the recent Apache Lucene 9.7 snapshots and got an number of tests failing with java.security.AccessControlException. The culprit is org.apache.lucene.util.VirtualMethod class that does a number of unprivileged calls to Reflection APIs without using AccessController.doPrivileged, a sample stack trace is below:

Caused by: java.security.AccessControlException: access denied (\"java.lang.RuntimePermission\" \"accessDeclaredMembers\")
	at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
	at java.base/java.security.AccessController.checkPermission(AccessController.java:897)
	at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:322)
	at java.base/java.lang.Class.checkMemberAccess(Class.java:2847)
	at java.base/java.lang.Class.getDeclaredMethod(Class.java:2471)
	at org.apache.lucene.util.VirtualMethod.reflectImplementationDistance(VirtualMethod.java:139)
	at org.apache.lucene.util.VirtualMethod$1.computeValue(VirtualMethod.java:78)
	at org.apache.lucene.util.VirtualMethod$1.computeValue(VirtualMethod.java:75)
	at java.base/java.lang.ClassValue.getFromHashMap(ClassValue.java:228)
	at java.base/java.lang.ClassValue.getFromBackup(ClassValue.java:210)
	at java.base/java.lang.ClassValue.get(ClassValue.java:116)
	at org.apache.lucene.util.VirtualMethod.getImplementationDistance(VirtualMethod.java:111)
	at org.apache.lucene.util.VirtualMethod.compareImplementationDistance(VirtualMethod.java:168)
	at org.apache.lucene.search.Query.<init>(Query.java:54)
	at org.opensearch.join.query.HasChildQueryBuilder$LateParsingQuery.<init>(HasChildQueryBuilder.java:402)
	at org.opensearch.join.query.HasParentQueryBuilder.doToQuery(HasParentQueryBuilder.java:207)
	at org.opensearch.index.query.AbstractQueryBuilder.toQuery(AbstractQueryBuilder.java:117)
	at org.opensearch.index.query.QueryShardContext.lambda$toQuery$3(QueryShardContext.java:466)
	at org.opensearch.index.query.QueryShardContext.toQuery(QueryShardContext.java:478)

I surely know that SecurityManager & co are deprecated but Apache Lucene / OpenSearch / Elasticsearch are still relying on it for the time being.

@uschindler @jpountz the fix is simple (happy to take it and submit a pull request) if there are no objections to make this change in general, cc @nknize

Sample failing tests:

• https://build.ci.opensearch.org/job/gradle-check/15584/testReport/junit/org.opensearch.join/ParentChildClientYamlTestSuiteIT/test__yaml__11_parent_child_Parent_child_inner_hits_with_seq_no__3/
• https://build.ci.opensearch.org/job/gradle-check/15584/testReport/junit/org.opensearch.join/ParentChildClientYamlTestSuiteIT/test__yaml__11_parent_child_Parent_child_inner_hits__2/

Version and environment details

Latest 9.7.0 snapshots (built of branch_9x)

[uschindler]

Hi, VirtualMethod is stone-aged class which was recently introduced again to handle the rewrite(IndexReader) vs rewrite(IndexSearcher) backwards compatibility.

Yes it may miss doPrivileged(). In fact the missing permission is granted by Opensearch (it's needed for other cases), but the doPrivileged is missing.

Nowadays I would implement this differently. Maybe fix it without permissions using lookups?

This is new in 9.7, the recently released 9.6 does not use this. If I provide a PR for 9.x can you test my "better fix"? I am not 100% sure it can be fixed without permissions, but give me a try.

[reta]

This is new in 9.7, the recently released 9.6 does not use this. If I provide a PR for 9.x can you test my "better fix"? I am not 100% sure it can be fixed without permissions, but give me a try.

Thanks @uschindler , sure, happy to assist with testing

[uschindler]

Thanks for finding the issue so early (before a release). Backport #12197 uses VirtualMethod class for the first time since.... Very long time. It was heavily used at Lucene 3 and 4 times. But back at that time we did not care about security manager.

[uschindler]

Hi,
I did some investigation. Actually it can't be done better with MethodHandles.Lookup (which inherits the actual visibility privileges by the caller; my idea was to require VirtualMethod v2.0 to take a MethodHandles.Lookup instance passed as ctor parameter, so it gets a "handle" to access the private methods of the class). Unfortunately this works the other way round, but VirtualMethod is about figuring out if a subclass override a method (and in addition how far the override is down the chain), to correctly delegate the calls of the backwards layer betwen the two implementations. Unfortunately in Java a superclass does not know of their subclasses and the superclass has no more rights to look into them. So a lookup does not help.

So we need to use doPrivileged. But here the important limitation: It is NOT the responsibility of VirtualMethod to use AccessController, it must be done by the caller. In the current case here it is not an issue as both are in same module, but for example use of VirtualMethod in another module would break if VirtualMethod does the doPrivileged. So the caller (here oal.search.Query) has to do it.

I will provide a PR.

[uschindler]

See this PR: #12308

Actually to reduce impact of this more we could add an if statement in the Query constructor to not do the override/distance check for Lucene's own classes (as we have all ported to use new API). The problem only affects external classes (and the corresponding Test). We could add a !this.getClass().getName().startsWith("org.apache.lucene.") && AccessController(...) check there.

[benwtrent]

@reta thank you for the early testing and finding this bug! Definitely my fault 🤦

[reta]

@reta thank you for the early testing and finding this bug! Definitely my fault facepalm

Not a problem at all, thanks for quick fix folks!

[uschindler]

@benwtrent I don't think it is your fault. The documentation did not even suggest to use AccessController.