error-prone: implement BanClassLoader with forbidden-apis instead

[rmuir]

This rule is attempting to ban dangerous usages of ClassLoader which could be security hazard (IMO a good idea). forbidden APIs is a good tool for banning, just like we use forbidden-apis to ban JNDI and other things.

[github-actions]

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

[rmuir]

@uschindler if you have any ideas, i'd love to fix this. This is just using the same banned list that error-prone uses, except as forbidden signatures.

[uschindler]

Needs to be MethodHandles$Lookup, this is how the real class name looks like.

[uschindler]

In reality most of the signatures here are automatically enabled by default (in a more dynamic way).

• jdk.internal.* is forbidden by default by disallowing all calls to non "public access" modules. See the "jdk-non-portable" bundled signatures: https://github.com/policeman-tools/forbidden-apis/wiki/BundledSignatures
• sun.misc.Unsafe is also forbidden by default (same as before: https://github.com/policeman-tools/forbidden-apis/wiki/BundledSignatures). Adding it here brings more problems than it solves. Because sun.misc.Unsafe get removed around Java 25 (and especailly that method mentioned here), this will cause issues in later Java versions.

Because of that all "internal" and "sun.misc" stuff should be removed here. The other ones are fine, java.lang.invoke.MethodHandles.Lookup#defineClass(**) needs to be java.lang.invoke.MethodHandles$Lookup#defineClass(**)

[rmuir]

thanks for the explanation, I figured there was a way to catch it, the $ is what I was missing, too rusty with java :)

I will remove some of the redundancies, too.

[uschindler]

All fine. I think thats documented somewhere. But basically the signatures files need the "binary name" of all classes.
I will add the wiki page to explicitely give an example (in addition to java.lang.String).

[github-actions]

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

[rmuir]

Source of the check: https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java

It also checks for explicit extends URLClassLoader, which isn't handled here. I'm not sure what special powers subclassing gives, maybe more methods need to be banned.

Also maybe some of these don't need to be banned because they are impossible due to java module system? I think google is still on java 8 :)

[uschindler]

In reality most of the signatures here are automatically enabled by default (in a more dynamic way).

• jdk.internal.* is forbidden by default by disallowing all calls to non "public access" modules. See the "jdk-non-portable" bundled signatures: https://github.com/policeman-tools/forbidden-apis/wiki/BundledSignatures
• sun.misc.Unsafe is also forbidden by default (same as before: https://github.com/policeman-tools/forbidden-apis/wiki/BundledSignatures). Adding it here brings more problems than it solves. Because sun.misc.Unsafe get removed around Java 25 (and especailly that method mentioned here), this will cause issues in later Java versions.

Because of that all "internal" and "sun.misc" stuff should be removed here. The other ones are fine, java.lang.invoke.MethodHandles.Lookup#defineClass(**) needs to be java.lang.invoke.MethodHandles$Lookup#defineClass(**)

[uschindler]

Source of the check: https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java

It also checks for explicit extends URLClassLoader, which isn't handled here. I'm not sure what special powers subclassing gives, maybe more methods need to be banned.

Also maybe some of these don't need to be banned because they are impossible due to java module system? I think google is still on java 8 :)

Exactly, see above. Most of the signatures here can be removed. jdk.internal is unreachable unless you pass open-modules parameters to jvaca or jdk. Nevertheless to SUpport Java 8, the forbiddenapis has a dynamic jdk-non-portable bundled signature which disallows all non-documented classes like sun.misc or those in jdk.* java modules.

About subclassing: A constructor is like a static methos. To forbid a constructur you need to give exact class names and ctors, there's no inheritance.

[uschindler]

In addition the list on errorprone is incomplete. Since Java 16 there's Lookup#defineHiddenClass(**) and Lookup#defineHiddenClassWithClassData(**) (see expressions module that needs a suppress then).

Actually the BanClassLoader error-prone check is a mess and outdated.

• https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#defineHiddenClass(byte%5B%5D,boolean,java.lang.invoke.MethodHandles.Lookup.ClassOption...)
• https://docs.oracle.com/en/java/javase/24/docs/api/java.base/java/lang/invoke/MethodHandles.Lookup.html#defineHiddenClassWithClassData(byte%5B%5D,java.lang.Object,boolean,java.lang.invoke.MethodHandles.Lookup.ClassOption...)

[uschindler]

Final note abozut subclasses: You need to add all ctors of sublasses of Classloader which reside in JDK. Now you might argue that one could define a subclass in user code (extending URLClassLoader) to circumvent the check. But this is differeent because when you declare that "user defined" classloader you have to add a ctor which calls super -> then it detect calling the call to superclass' ctor which was forbidden.

[github-actions]

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

[github-actions]

This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR.

[rmuir]

With help of @uschindler, this check is a bit different than how the error-prone rules implement it, but more thorough.

The stated goal of the error-prone check is to prevent turning input into code, but it did not really work for us: it only introduced a false positive in Luke but did not find any issue with expressions!

So these rules instead target methods that define new code.

[uschindler]

Thanks Robert! As always: nice discussions. I will improve the signatures file documentation to give a hint how the inner class binary names look like (give an example).