Load plugins into classpath in bootstrap

[rmuir]

This can be done on startup, then we restore setAccessible(false) and engage security.

This also removes uber-dangerous global sun.misc permission (so only special jars have it).

Its not a true fix to #11917 but its progress and improves security.

Note: I had to remove some "integration-like" tests. In this simpler world all plugins are "classpath plugins" so testing is already more realistic (and we can followup by removing that confusing parameter) If we want some more integration tests around it I think those should be in a separate place? Anyway i feel bad about not being able to preserve all the tests, but I think its important to lock this down.

[rmuir]

I discussed with @uschindler (he inspected source code), we don't need the try-finally block since the Method does not escape. I will simplify this one.

[uschindler]

Hi,
you dont need to do setAccessible. The JVM allows setAccessible only per-instance (the Javadoc is a bit vague about this). The instances are never cached. Each call to Class.getMethod() returns a new instance (the internal cached one is cloned/copied). You can follow this in the JDK source code (grepcode.com). In fact the Method instances cannot really be cached for outside callers, because they are @CallerSensitive
Uwe

[rmuir]

Thanks for the investigation! I cleaned this up

[uschindler]

By the way here is the Method.copy() which is used by Java's cache. This one has the nice statement that confirms all of this: http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8u40-b25/java/lang/reflect/Method.java#Method.copy%28%29

        // This routine enables sharing of MethodAccessor objects
        // among Method objects which refer to the same underlying
        // method in the VM. (All of this contortion is only necessary
        // because of the "accessibility" bit in AccessibleObject,
        // which implicitly requires that new java.lang.reflect
        // objects be fabricated for each reflective call on Class
        // objects.)

[rjernst]

How can this be null except in the case the classloader is Object (impossible)? Won't each iteration of the loop have a non-null addURL (since setAccessible is called)?

[rmuir]

i just moved this code from where it was. I didn't write it. Can we defer cleanup to another issue?

[rmuir]

(the answer is that NoSuchMethodException can strike there)

[rjernst]

Disregard for now, I can see this code is just moved,

[nik9000]

In the event that the ClassLoader doesn't contain an addURL method this will be tripped. I think this is more of a WARNING level or and ERROR than a debug. Right?

[nik9000]

ClassLoader itself doesn't have an addURL method, but SecureClassLoader does, and I suspect other subclasses will as well.

[rmuir]

again, i just moved this code from where it was. lets clean it up separately.

[rjernst]

LGTM

[uschindler]

We should really fix the addURL shit in later versions. It is horrible to add URLs to SystemClassLoader. The correct way to do this would be to make Bootstrap to create a new ClassLoader with all URLs and let the application classloader (which is != SystemClassloader, so this is also a security issue!!!) just be parent.

[rmuir]

@uschindler yes, the long term fix is #11917

This issue is to improve security around this more, by doing this up-front in bootstrap. Its just a step. It removes access to sun.misc.Unsafe for example, except where explicitly granted.

If i go off on tangents and try to fix everything about some of this stuff, then we will make no progress. It remains the same stuff as it was before.

[uschindler]

OK, thanks! I just wanted to point out that modifying the system classloader is a huge security issue. Nobody ever should do that! It would be OK if it s the application classloader, but this variant is a desaster! Really! That sucks horribly. I have to say that Apache Solr does a much better job here! :-)

[uschindler]

Who wrote that code should be nailed against a wall and the Forbidden Policeman would probe his gun at him/her :-)

[uschindler]

Was a joke, of course :-) Something for @UweSays :-)

[uschindler]

I would also see this as huge security issue in any previous Elasticsearch version! You should patch earlier versions to not use System classloader and instead (if you dont fix it completely like in 2.0), change the code to use: Bootstrap.class.getClassLoader() instead of ClassLoader.getSystemClassLoader so it uses the application classloader and does not modify the JVM internals bypassing any security checks!

[rmuir]

Hi Uwe, again the code was simply moved here, so securitymanager can blockaccess to sun.misc. That's all this issue is about. I did not write this code here.

If you want to improve the code, please spend efforts helping me with #11917, then its gone completely. We don't need to improve it. We need to remove it.

[uschindler]

I was talking about 1.6 and earlier! I will open issue. This cannot stay like this!

[rmuir]

1.6 isn't even on my radar. it doesn't even run with a security manager

[uschindler]

It still breaks the whole JVM internals!