Add new bundled signatures for disallowing internal runtime APIs + fix class loading order bugs

[rmuir]

Problem 1.
If you name a class sun.misc.Unsafe and put it in your classpath, it does not change the fact, any code calling that will still be calling the real Unsafe. But it does hide it from forbidden-apis!!!

This is because classloader order, when the application is actually used, is Bootstrap->Extensions->System (App), as explained here: http://docs.oracle.com/javase/tutorial/ext/basics/load.html

But forbidden-apis checks the wrong order, it checks the ones you provide first, because it uses lookupRelatedClass():

forbidden-apis/src/main/java/de/thetaphi/forbiddenapis/Checker.java

Lines 303 to 306 in 9de5e1f

	ClassSignature c = classesToCheck.get(internalName);
	if (c == null) try {
	// use binary name, so we need to convert:
	c = getClassFromClassLoader(type.getClassName());

This causes the interesting scenario when trying to cleanup crazy classpaths, where removing a jar can cause new violations in your build :)

Problem 2:
The current isRuntimeClass() does not seem to check for extensions at all, but only against bootstrap classpath. This hides additional internal accesses, e.g. jdk.nashorn.internal, which will cause a SecurityException if you try to use it.

So can we use the following code on pre-jigsaw, to identify extensions jars and treat them as "internal" too? I think this extensions idea goes away with jigsaw, and everything is just modules, so it should not be a problem that we can't get extensions jars/directories there.

// of course with proper checks and best-effort, not guaranteed but works.
URLClassLoader loader = (URLClassLoader) ClassLoader.getSystemClassLoader().getParent();
URL extensions[] = loader.getURLs();

Problem 3:
Internal checking has a hardcoded list of simple patterns:

forbidden-apis/src/main/java/de/thetaphi/forbiddenapis/AsmUtils.java

Line 33 in f7dce21

for (final String pkg : Arrays.asList("sun.", "oracle.", "com.sun.", "com.oracle.", "jdk.", "sunw.")) {

Can we use java.security.Security.getProperty("package.access") instead? That property is set by the JDK, in e.g. the jre/lib/security/java.security configuration file for security checks against internal apis:

#
# List of comma-separated packages that start with or equal this string
# will cause a security exception to be thrown when
# passed to checkPackageAccess unless the
# corresponding RuntimePermission ("accessClassInPackage."+package) has
# been granted.
package.access=sun.,\
               com.sun.xml.internal.,\
               com.sun.imageio.,\
               com.sun.istack.internal.,\
               com.sun.jmx.,\
               com.sun.media.sound.,\
               com.sun.naming.internal.,\
               com.sun.proxy.,\
               ... (many lines) ...

[rmuir]

By the way, i only listed these bugs because i am hitting all of them. Because we are now scanning third-party dependencies too (elastic/elasticsearch#15491), there is a lot more craziness. I am encountering crazy jar hell (with the JDK itself) and other crazy shit, and trying to get it sorted out :)

[rmuir]

I think the simplest and also faster solution is to remove all of the current 'isInternal' boot classpath logic and all of that, and just use matches(Security.getProperty("package.access") as the algorithm here. That is a totally different mechanism unrelated to module protection or anything and I would expect that its kept up to date (in my jigsaw EA builds, I have a huge list, so I think it is). It would be more portable in that sense IMO too.

[uschindler]

Hi,
I have a plan to fix this. As said before, I don't want to depend on the currently executing runtime to detect the classes (e.g. if somebody check forbidden apis with IBM J9 or a new version). The internal runtime checks should be done on the "official released JDK reference implementation" of the java version to check (not dependent on current runtime). So basically the same thing like with deprecated APIs is done here:

• add "ant generate-internal": this works like "ant generate-deprecated" and produces the signatures file. This basically parses the Security.getProperty("package.access") property and generates a simple signatures file by appendig (".**").
• deprecate the old internal-runtime-forbidden setting on forbidden apis. It wll still work like it does currently. Users of forbidden should simply use the new bundled signatures: "jdk-internal". This was already my plan, because the code to forbid packages is already there since we allow glob patterns in signatures files.

A first preview is on this branch: 727db66

[uschindler]

This patch would make #54 obsolete. I think we should simply forbid the packages and not try to detect if a class is coming from the rt.jar/module system. So we can later remove the isRuntime boolean from our class lookup.

[uschindler]

The question I have: Maybe also look at package.definition security property? The current generateor code could parse and merge both... This property has all packages that user code is not allowed to place user classes in. As we also check that our own class name is not on forbidden list, we should add both (although it looks like both flags are mostly identical in recent JVMs).

E.g., Tomcat modifies both properties to prevent bad stuff inside webapps!

[rmuir]

yeah, i know they are mutable (or configurable with simple text file). I agree to just have a baseline list for package.access.

I would not worry about package.definition, that one is totally useless! Does absolutely nothing!!!!!!!!!!!

[uschindler]

I would not worry about package.definition

My idea was to have a check on the class name of the parsed class and match it against package.definition (the same check, just statically, that Classloader does). Any class with a name from this list should not exist outside the runtime. But thats a separate thing, I agree.

[uschindler]

@rmuir about your commit: rmuir/elasticsearch@e59b990

I'd just remove the forbidden setting "internal-runtime-forbidden" (to be deprecated) and just add https://github.com/policeman-tools/forbidden-apis/blob/issues/91/src/main/resources/de/thetaphi/forbiddenapis/signatures/jdk-internal-1.8.txt as a standard signatures file to elasticsearch. So you don't need to parse log output.

This is basically what this branch is doing.

[rmuir]

Good idea! It removes some of the parsing. Unfortunately i still need parsing, for the custom whitelist handling (whitelisting missing classes and also failing the build if a whitelisted class did not have any problems)

[uschindler]

One thing to keep in mind if we do the change proposed here: In Lucene and Elasticsearch we currently don't really want to have code access Oracle-only classes like the com.sun.management.HotSpotDiagnosticMXBean. This package is not on this list (because it is part of public Oracle JDK APIs - but not IBM J9!!!). Previous forbidden code would detect this, the static signatures file won't detect this.

So maybe we should keep internalruntimeforbidden for a while and just add the new signatures packages for now. The idea behind the internal-runtime-forbidden setting was not "correctness", it was meant as a fail fast for anything that looks like internal. The additional isRuntime handling was made extra for that (to not fail on classes named like this in user code).

So basically, ES is misusing the internalruntimeforbidden, which was just a "best guess".

To prevent stuff like com.sun.management.HotSpotDiagnosticMXBean we would need to add a separate signatures file to Lucene or Elasticsearch.

[rmuir]

But that one is not an internal api!

[uschindler]

But that one is not an internal api!

I know, but we decided to not use it - as its not available on IBM J9 and others (the same applies to Nashorn APIs). If you use it, you should use reflection or guard with large try-catch to ensure it also works with JVMs that don't have Nashorn. Maybe naming of the feature is wrong: the idea behind internal-runtime-forbidden was to forbid any use of classes in the JDK runtime that are not in the default packages like java/javax because it might not be portable. This is why it is implemented like it is. And I still think this is correct.

As a side-effect, sun.misc.Unsafe also falls under that, and - of course - it is really internal! If you have a false positive, you can always use @SuppressWarnings in your code.

So we are talking about 2 different things!

[rmuir]

I don't agree, the check is simply buggy to me, and I think this is excuses for it. Its named 'internal runtime api' check, then that is what it should do.

if we want a separate shitty check for some other purpose, then it should have a name to reflect that.

[uschindler]

if we want a separate shitty check for some other purpose, then it should have a name to reflect that.

The check is not shitty, sorry. It is a heuristic (just a best guess) and is therefore valid. You could also say the checks for charset violations are shitty because they may be incomplete, which they aren't.

The name is historical and won't change for backwards compatibility until forbidden 3.0. Please revise your statement.

If you want another check for something that forbiddenapis was not made for (checking external jars), you should implement your own signature file (as said before). I will add the new bundled signatures, but I will not disable the old "best guess" check, which works perfectly fine if you implement code. I don't want any code to use non standardized APIs that you cannot rely on. We can rename the parameter, but the old name will stay until 3.0. People like you can use more strict signatures for scanning JAR files.

[rmuir]

well, its obviously buggy in several ways today, as i already enumerated on this issue.

You insist that my use (scanning our third party jars) is an abuse case, but it is not. Class files are just class files!

The only difference is that I have more of them, so I find all the bugs in forbidden apis.

[uschindler]

The only things that buggy is the isRuntime check because of wrong order. I agree it can be removed, but this is a different issue. The reason why the isRuntime check is there is to make heuristics better. I will improve that check, but this will double memory requirements under some circumstances. Unfortunately, the current code breaks with some crazy libraries (which behave wrong, sorry, just my opinion. The code inside the broken libraries is broken). You just want to filter APIs that may fail with security manager. I WANT TO FILTER EVERYTHING THAT MAY NOT BE STANDARDIZED (and I call this "internal"). These are different tasks. You should simply not use internal-runtim-forbidden and instead use a hardcoded signatures file without heuristics. Problem solved. No need to change the heuristics in forbidden. I will improve them a bit, but I want to have a check for any "non-standardized" APIs in the JDK. And this works quite fine, because you see the failures.

As said before: forbiddenapis is not intended to be 100% correct in its heuristics (also resolving of methods is partly buggy according to java standards, eg. interfaces and classes order). If some failure occurs you can use @SuppressForbidden with a comment. forbiddenapis is just a checker for your own code so you get a hint if something is broken. It is up to you to suppress failures if you think forbiddenapis is wrong in its heuristics. The code is full of heuristics, e.g. to correctly report failures, method names, and line numbers inside lambda expressions.

[rmuir]

Please, rename your feature as 'nonstandard API'. It has a couple practical problems, in that there is a lot of logic looking at bootclasspath JARs and so on.

But the 'internal API' check can be easily made simple, and correct!

[rmuir]

As far as your list, i would implement that one differently anyway. If you want to ban those nonstandard apis, just go through links like this one: https://docs.oracle.com/javase/7/docs/

and you can find pointers to all of the bundled jdk apis (stuff like com.sun.net.httpserver)

[uschindler]

But the 'internal API' check can be easily made simple, and correct!

See the linked branch. It adds a bundled sigantures file jdk-internal already, populated from the list as shipped with jdk.

[rmuir]

We should make a similar list for the nonstandard public oracle extension apis. It is very useful to know which is which... I also want forbidden failing on these but the message should be different and the implementation simpler

[rmuir]

To find these, I think we want to look for this annotation:

https://docs.oracle.com/javase/8/docs/jdk/api/javac/tree/jdk/Exported.html

[uschindler]

Changing the message is easy :-) That's already on my plan. The algorithm is not very complicated: The idea is to have a very simple list of "bad" packages. The isRuntime check is there as "confirmation" that it is really "bad" (e.g. some projects bundle libraries containing "com.sun." or "javax." APIs, e.g. for mailing - also Solr is doing this). Of course the classloader will load those APIs of JDK in preference and fall-back to provided ones if given. So the isRuntime check is needed to differentiate between externally provided and other ones, although package / class name is identical. Unfortunately the world is not back/white here. The bug is with order that needs to be fixed (problem 1 in your list). The fix is not easy without loading everything two times.

The jdk.Exported annotation is useless because not available in Java 6 or Java 7. I was investigating it already.

[rmuir]

I think you miss my suggestion:

• create list A: internal runtime classes. you have already done this.
• create list B: exported runtime classes. we can base this on exported.

in both cases, just .txt file lists: no magic. And no confusion about stuff like com.sun.jersey which is neither.

we don't need the annotation at runtime, we can just use it to generate the list a single time.

[uschindler]

The idea with the heristics is alo quite simple. It only has a small bug I intend to fix. We can also rename the heuristics, so where is your problem.

list A, which is aready done, is easy. The other part is a simple heuristics as it is today. It has proved to be working since version 1.0 of forbiddenapis and has detected many bugs (like stuff using sun.misc.BASE64Encoder). So what's your problem with the heuristics. If they break, you can add @SuppressForbidden - DONE. But I have not yet found any false possitive. What you have done is fine and it found some problems in some JAR files.

The "list a" solution - as discussed here, needs no change in forbiddenapis, just bundle the list with your build system. If you don't agree with the heuristics, don't use them for scanning JAR files!

[rmuir]

I just want forbidden to work well out of box. and the current situation causes confusion with regards to internal vs exported:

• internal: DON'T USE!
• exported: ok to use, but please use reflection in case its another java implementation.

currently it is tricky when reviewing code (example: elastic/elasticsearch#15489)

That is because you must manually inspect javadocs for each thing used and look for Exported if you want to be correct. Especially with the management package, for example most of it is "ok" (exported), but for example com.sun.management.OSMBeanFactory is explicitly not.

http://mail.openjdk.java.net/pipermail/security-dev/2013-October/009082.html

So it would just be easier if forbidden had these encoded in its files to prevent mistakes.

[uschindler]

This would be a separate file (the @exported) we could create for Java 8+ (but can also be used before Java 8, because it would supress non-found method signatures). The code to generate the file is basically the jdk-deprecated code, just looking for another annotation.

Can you open separate issue?

[rmuir]

I am fine with it as a separate issue but it is related to this one: the idea is to remove the heuristics and split into three categories:

• internal (e.g. sun.misc.Unsafe)
• exported (e.g. com.sun.management, JAAS, nashorn public api, ...)
• neither (e.g. com.sun.jna, com.sun.jersey, etc)

For the internal list, we can use package.access as you have done. We could even later split this into two lists "internal" and "critical internal" (based on JEP), if we want.

For the exported list, we can autogenerate it for java 8+, but we can also manually produce this list for older JDKs. E.G. the JAAS api existed and was supported in java 7. Personally, I am not even sure if exported deserves a forbidden warning going forwards. In fact even IBM JDK is using the openjdk class library for java 8+.

Otherwise, we should not issue a warning just because it starts with com.sun.xxx. In many cases that is just what it used for historical reasons (e.g. JNA).

[uschindler]

Personally, I am not even sure if exported deserves a forbidden warning going forwards. In fact even IBM JDK is using the openjdk class library for java 8+.

The example (linked issue about the HotspotMXBean and/or the compressed OOPs) is a typical example. Its exported in OpenJDK, but IBM J9 does not have it. The class name of the bean is different. I f*cked with this in Solr already. :-(

[uschindler]

I just want forbidden to work well out of box. and the current situation causes confusion with regards to internal vs exported:

It works out of the box without confusion. The heuristics are not enabled by default, so it was no issue. It does what it should do by default.

[rmuir]

The example (linked issue about the HotspotMXBean and/or the compressed OOPs) is a typical example. Its exported in OpenJDK, but IBM J9 does not have it. The class name of the bean is different. I f*cked with this in Solr already. :-(

But its not an internal API, and its no reason to have a broken (and crazy) heuristic in forbidden-apis when it can be separate text file lists.

[uschindler]

For the exported list, we can autogenerate it for java 8+, but we can also manually produce this list for older JDKs. E.G. the JAAS api existed and was supported in java 7.

As those signatures files have the internal flag to completely suppress warnings and failures on missing classes/methods (completely silent without warnings since 2.1, still warnings in 2.0), we can just use the 8.0 file also in Java 6 and Java 7. I would like to fix this with a simple import and dummy files.

[uschindler]

The heuritic is not broken and very simple. I cannot and will not remove it, sorry. If somebody wants to use it, its fine to use. I would only like to rename it until 3.0. People that use old name get a deprecation warning.

The heuristics are fine: It just disallows anything in the JDK to be called that is part of the runtime libs (rt.jar or java.* named module) and has a non-standard package name. Nothing special. Very simple. If you want it more fine-granular, use the static signatures. The heuristic code is just detecting maybe broken stuff, e.g., it would also cry if somebody uses a IBM-J9 internal API, which you cannot detect with a static signature file.

The only bug is the isRuntime check using wrong classloader order (which is not easy to fix).

[uschindler]

I fixed problem 1 and problem 2 in the heuristics. The actual "bugs" listed here are fixed now on the branch.

[uschindler]

I opened a new issue/PR to rename the internalRuntimeForbidden (as bundled signature with heuristics that work correct): #95