Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove all accessClassInPackage permissions for java 9 compatibility #13444

Open
rmuir opened this issue Sep 9, 2015 · 40 comments
Open

remove all accessClassInPackage permissions for java 9 compatibility #13444

rmuir opened this issue Sep 9, 2015 · 40 comments
Labels
>bug :Core/Infra/Core Core issues without another label help wanted adoptme Team:Core/Infra Meta label for core/infra team

Comments

@rmuir
Copy link
Contributor

rmuir commented Sep 9, 2015

With jigsaw this stuff is not really an option anymore of security manager or not (try a build: http://jdk9.java.net/jigsaw)

We have quite a few of these in master, they all need to be removed, so things work on java 9:


  // needed by aws core sdk (TODO: look into this)
  permission java.lang.RuntimePermission "accessClassInPackage.sun.security.ssl";

I opened a PR for this (aws/aws-sdk-java#432) but they have gone silent. Maybe they do not know, aws-sdk will not work on java 9 if it tries to do this (and I don't see why they should be doing it at all).

Removed.


  // needed by groovy engine
  permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";

This one needs looking into, I don't know why groovy needs this.


  // needed for mock filesystems in tests (to capture implCloseChannel)
  permission java.lang.RuntimePermission "accessClassInPackage.sun.nio.ch";

This one becomes more fine-grained to just the lucene test-framework jar in #13439. Additionally I raised a thread on nio-dev about it: http://mail.openjdk.java.net/pipermail/nio-dev/2015-September/003321.html

Removed.


grant codeBase "${es.security.jar.lucene.core}" {
  // needed to allow MMapDirectory's "unmap hack"
  permission java.lang.RuntimePermission "accessClassInPackage.sun.misc";
};

This one is currently being discussed on the jdk lists: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-September/035055.html

However if its not available, lucene will fall back to not unmapping, so things will still "work", you just have disk usage, address space, etc tied to the garbage collector.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 10, 2015

sun.nio.ch can be removed completely after we upgrade lucene again.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 10, 2015

lucene tests are passing with the jigsaw EA now: https://issues.apache.org/jira/browse/LUCENE-6795

i will bump the lucene version in #13439 and see how ES looks.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 10, 2015

The answer is that nothing compiles, all tests fail, and ES won't start, of course :)

@rmuir
Copy link
Contributor Author

rmuir commented Sep 10, 2015

@uschindler opened a bug at groovy that might be related to our issues: https://issues.apache.org/jira/browse/GROOVY-7587

@uschindler
Copy link
Contributor

However if its not available, lucene will fall back to not unmapping, so things will still "work", you just have disk usage, address space, etc tied to the garbage collector.

The Lucene MMapDirectory unmapping is part of the whitelisted APIs in JIGSAW, so we can still call it: http://openjdk.java.net/jeps/260

Thanks to our work in arguing about sun.misc.Cleaner :-)

@jprante
Copy link
Contributor

jprante commented Sep 13, 2015

sun.nio.ch and sun.nio.fs (at least) are used by Groovy when using NIO, e.g. when looking for sun.nio.fs.UnixPath methods dynamically.

Example:

Exception in thread "main" java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessClassInPackage.sun.nio.fs")
    at java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
    at java.security.AccessController.checkPermission(AccessController.java:884)
    at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
    at java.lang.SecurityManager.checkPackageAccess(SecurityManager.java:1564)
    at java.lang.Class.checkPackageAccess(Class.java:2372)
    at java.lang.Class.checkMemberAccess(Class.java:2351)
    at java.lang.Class.getMethod(Class.java:1783)
    at org.codehaus.groovy.reflection.stdclasses.CachedSAMClass.hasUsableImplementation(CachedSAMClass.java:130)
    at org.codehaus.groovy.reflection.stdclasses.CachedSAMClass.getSAMMethod(CachedSAMClass.java:191)
    at org.codehaus.groovy.reflection.ClassInfo.isSAM(ClassInfo.java:359)
    at org.codehaus.groovy.reflection.ClassInfo.createCachedClass(ClassInfo.java:349)
    at org.codehaus.groovy.reflection.ClassInfo.access$700(ClassInfo.java:41)
    at org.codehaus.groovy.reflection.ClassInfo$LazyCachedClassRef.initValue(ClassInfo.java:497)
    at org.codehaus.groovy.reflection.ClassInfo$LazyCachedClassRef.initValue(ClassInfo.java:488)
    at org.codehaus.groovy.util.LazyReference.getLocked(LazyReference.java:49)
    at org.codehaus.groovy.util.LazyReference.get(LazyReference.java:36)
    at org.codehaus.groovy.reflection.ClassInfo.getCachedClass(ClassInfo.java:111)
    at org.codehaus.groovy.reflection.ReflectionCache.getCachedClass(ReflectionCache.java:110)
    at org.codehaus.groovy.reflection.CachedClass$4.initValue(CachedClass.java:141)
    at org.codehaus.groovy.reflection.CachedClass$4.initValue(CachedClass.java:138)
    at org.codehaus.groovy.util.LazyReference.getLocked(LazyReference.java:49)
    at org.codehaus.groovy.util.LazyReference.get(LazyReference.java:36)
    at org.codehaus.groovy.reflection.CachedClass.getCachedSuperClass(CachedClass.java:248)
    at org.codehaus.groovy.reflection.CachedClass$8.initValue(CachedClass.java:214)
    at org.codehaus.groovy.reflection.CachedClass$8.initValue(CachedClass.java:200)
    at org.codehaus.groovy.util.LazyReference.getLocked(LazyReference.java:49)
    at org.codehaus.groovy.util.LazyReference.get(LazyReference.java:36)
    at org.codehaus.groovy.reflection.CachedClass.getInterfaces(CachedClass.java:252)
    at org.codehaus.groovy.reflection.CachedClass.<init>(CachedClass.java:238)
    at org.codehaus.groovy.reflection.ClassInfo.createCachedClass(ClassInfo.java:352)
    <<<truncated>>>

@rmuir
Copy link
Contributor Author

rmuir commented Sep 13, 2015

That is not allowed. See https://issues.apache.org/jira/browse/GROOVY-7587

@uschindler
Copy link
Contributor

The whole thing is already investigated by both Oracle and also Groovy people. This is no 1 issue on the jigsaw-dev mailing list :-)

The issue has nothing to do with this it accessing stuff inside nio. The problem is: Theoretically Groovy can handle that, it is just "surprised" by a new Exception type it cannot handle. Oracle now thinks about making this new Exception a subclass of SecurityException to work around issues like that (setAccessible was only documented to throw SecurityException and now it suddenly throws another runtime exception. Because of that Groovy cannot handle that (and hide the non-accessible method, it just fails). By default it simply ignores fields it cannot make accessible.

@jprante
Copy link
Contributor

jprante commented Sep 14, 2015

I'm confused, because I get the security exception in both JDK 8 and JDK 9.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 14, 2015

You get the SecurityException because we don't allow it by policy in elasticsearch.

On java 9 even if there is no securitymanager, you will still get InaccessibleObjectException.

@jprante
Copy link
Contributor

jprante commented Sep 14, 2015

FWIW, using

java version "1.9.0-ea"
Java(TM) SE Runtime Environment (build 1.9.0-ea-b81)
Java HotSpot(TM) 64-Bit Server VM (build 1.9.0-ea-b81, mixed mode)

and a command line like

./bin/elasticsearch -Des.cluster.name=mycluster -Dsecurity.manager.enabled=false

then ES 2.0.0-beta1 with my groovy plugin which uses Paths.get(...) does not abort with an exception.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 14, 2015

You need to use a jigsaw build.

@jprante
Copy link
Contributor

jprante commented Sep 14, 2015

Ok, I missed that.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 14, 2015

You can download one from here: http://jdk9.java.net/jigsaw

Master is fully functional on that build but only after some battles this weekend :)

The remaining internal packages that we use (for mmap unmapping and reflection for scripting and mocks) are "ok for now" because they are "critical internal apis" (http://openjdk.java.net/jeps/260). But these are deprecated and going away eventually.

@uschindler
Copy link
Contributor

Hi,
actually Robert's and your problem are 2 different ones: https://issues.apache.org/jira/browse/GROOVY-7587 talks about a new exception which solely happens on setAccessible. This will only affect Java 9 Jigsaw builds. This is a real bug.

The problem here is a different one and has to do with reflection in general. The problem is that Java returns package internal classes which implement a public interface. Because groovy needs to call methods on it, it must reflect on them. So we must allow that otherwise you cannot even find out which methods are. Reflecting internal packages is not necessarily bad, that is something every scripting language must be able to do. Java 8's internal Javascript can do this, the problem is external scripting engines. You must just disallow to call seAccessible()!

In fact Groovy does not want to make them accessible, it just needs to find out what methods are on the object - it does not want to make them accessible or call them. These are 2 different issues. The latter will also work with Java 9!

@jprante
Copy link
Contributor

jprante commented Sep 14, 2015

Yes, I got confused because I thought JDK 9 early access include jigsaw but there are separate builds.

The issues are indeed different. Just worrying about how Groovy will work in the JDK 9 future, and with or without security manager. It's a bit too early to see the light at the end of the tunnel.

Robert is doing amazing work!

1 similar comment
@jprante
Copy link
Contributor

jprante commented Sep 14, 2015

Yes, I got confused because I thought JDK 9 early access include jigsaw but there are separate builds.

The issues are indeed different. Just worrying about how Groovy will work in the JDK 9 future, and with or without security manager. It's a bit too early to see the light at the end of the tunnel.

Robert is doing amazing work!

@uschindler
Copy link
Contributor

Just worrying about how Groovy will work in the JDK 9 future, and with or without security manager

It does not work at all. The most funny fact is: They cannot fix (and more important test the bugfix) because of a chicken-egg problem: "For Groovy we have a chicken and egg problem for testing, because this change breaks Groovy, and Groovy uses Gradle to build. Since Gradle itself uses Groovy, we have no compatible build tool to test the fix... So it's very problematic." (http://mail.openjdk.java.net/pipermail/jigsaw-dev/2015-September/004517.html)

So they cannot even build Groovy with Java 9 - LOL! Personally, I would nuke Groovy in ES and use Nashorn only.

@jprante
Copy link
Contributor

jprante commented Sep 14, 2015

It's more complex and not only Groovy - all platforms with dynamic class loading and inspection with setAccessible tweaks are affected by the new jigsaw puzzle, where Guava is one prominent example (I just worked through the openjdk thread). And Guava is also an ES dependency. In the end it's the question whether only JDK code will be able to control the setup of a JVM module structure at JVM startup time or non-JDK code too. Let's hope Oracle will not ignore these projects.

@uschindler
Copy link
Contributor

bq. And Guava is also an ES dependency.

That is not a problem, because ES does not use the reflection stuff in Guava. This is like commons-lang + commons-collections + commons-io, a large library.

@uschindler
Copy link
Contributor

Let's hope Oracle will not ignore these projects

It is also vice versa. Like Lucene and ES, projects should look into the issues and fix their code. As seen from my work yesterday, most stuff can be done without making anything accesible by just refectoring your code. I did a short conculsion to the lucene-dev mailing list: http://mail-archives.apache.org/mod_mbox/lucene-dev/201509.mbox/%3C014801d0ee23%245c8f5df0%2415ae19d0%24%40thetaphi.de%3E

@s1monw
Copy link
Contributor

s1monw commented Sep 14, 2015

And Guava is also an ES dependency.

@jprante we are getting rid of guava for 3.x here #13224

@rmuir
Copy link
Contributor Author

rmuir commented Sep 14, 2015

t's more complex and not only Groovy - all platforms with dynamic class loading and inspection with setAccessible tweaks

And setAccessible is not a "tweak", its not ok to use at all. I am working to ban it completely with the security policy as soon as possible.

this thread says it nicely: http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-June/019277.html

On 06/29/2015 10:47 PM, John Rose wrote:
> I assume you are thinking about setAccessible, but if that's allowed there's nothing to prevent people from taking whole system down in a hundred different ways.  

Ok, thanks for the clarification John. I’ll admit to occasionally forgetting that setAccessible is as unsafe as, well, Unsafe.

@uschindler
Copy link
Contributor

Nice post!

I think we have 2 issues here: setAccessible must be banned anyways. The problem with groovy is also another problem: Dynamic scripting languages have to figure out which methods they can call on, so they have to "inspect" the class. As you see from stack trace above, Groovy did not want to call setAccessible. The issue was it was not even able to do Class#getMethod() which retrieves only public methods. This is needed to call the function dynamically. The problem is that we forbid refelction at all!

So forbidding setAccesible is fine, but for dynamic scripting languages you should allow "inspection" of those classes, otherwise they will never be able to call any method.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 14, 2015

So forbidding setAccesible is fine, but for dynamic scripting languages you should allow "inspection" of those classes, otherwise they will never be able to call any method.

That is not going to happen. it is too much of a security risk. For scripts in ES, this is not needed (and by the way, groovy scripts run with zero permissions so there is no sense in them getting a Path or File or anything like that).

@rmuir
Copy link
Contributor Author

rmuir commented Sep 14, 2015

And its not allowed in java 9 anyway, so good time to stop dreaming.

@uschindler
Copy link
Contributor

C:\Users\Uwe Schindler\Desktop\robert>cat Test.java
import java.nio.file.*;
import java.lang.reflect.Method;
public class Test {
 public static void main(String... args) throws Exception {
  Path path = Paths.get(".");
  Class<?> clazz = path.getClass();
  System.out.println(clazz.getName());
  Method m = clazz.getMethod("resolve", String.class);
  System.out.println(m);
 }
}
C:\Users\Uwe Schindler\Desktop\robert>javac Test.java

C:\Users\Uwe Schindler\Desktop\robert>java -version
java version "1.9.0-ea"
Java(TM) SE Runtime Environment (build 1.9.0-ea-jigsaw-nightly-h3337-20150908-b80)
Java HotSpot(TM) 64-Bit Server VM (build 1.9.0-ea-jigsaw-nightly-h3337-20150908-b80, mixed mode)

C:\Users\Uwe Schindler\Desktop\robert>java Test
sun.nio.fs.WindowsPath
public default java.nio.file.Path java.nio.file.Path.resolve(java.lang.String)

C:\Users\Uwe Schindler\Desktop\robert>

I was just talking about "inspection" not calling or making stuff accessible! :-)

@rmuir
Copy link
Contributor Author

rmuir commented Sep 14, 2015

Public methods are no problem, we don't do anything to prevent that.

As far as accessDeclaredMembers we don't prevent that either: that is not low hanging fruit at the moment.

@jprante
Copy link
Contributor

jprante commented Sep 14, 2015

Yes, I create a jar for ES plugin zip with gradle. Compiled groovy classes are bytecode for the JVM but they come with a load of extra language features, dynamic type checking at run time, compilation on demand, class caching, method / operator overloading, meta object protocol (MOP) etc. It's the Groovy runtime that executes reflection.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 15, 2015

@jprante I wish I had an easy answer for you. We know groovy has challenges here (hence the java 9 problems), but I don't know about the issues besides our scripting integration, I just have not looked into it. That one is simpler because of what that functionality is designed to do, vs writing entire plugin in it.

I do know it has the option for static compilation, which could be an interim workaround, until they solve these issues (it claims direct method calls which should bypass the issue). I have heard it takes away a lot of the grooviness, sorry I'm just not knowledgeable on groovy and trying to brainstorm solutions that do not involve disabling security.

@jprante
Copy link
Contributor

jprante commented Sep 16, 2015

@rmuir thanks, I appreciate your help. I will disable security manager for my Groovy plugins for now.

Unfortunately static compilation does not make a difference. It removes dynamic typing, which is good for faster execution time, but does not make the Groovy runtime obsolete, which is invoked since all Groovy classes extend from groovy.lang.GroovyObject. The Groovy meta programming feature executes the reflection API.

import groovy.transform.CompileStatic

@CompileStatic
class HelloWorld {

   HelloWorld(msg) {
     println 'Hello World, ' + msg
   }

   static void main(String[] args) {
       new HelloWorld('Jörg')
   }
}
javap -cp . HelloWorld
Compiled from "HelloWorld.groovy"
public class HelloWorld implements groovy.lang.GroovyObject {
  public static transient boolean __$stMC;
  public HelloWorld(java.lang.Object);
  public static void main(java.lang.String...);
  protected groovy.lang.MetaClass $getStaticMetaClass();
  public groovy.lang.MetaClass getMetaClass();
  public void setMetaClass(groovy.lang.MetaClass);
  public java.lang.Object invokeMethod(java.lang.String, java.lang.Object);
  public java.lang.Object getProperty(java.lang.String);
  public void setProperty(java.lang.String, java.lang.Object);
}

From my little knowledge, if people try to create ES JVM plugins with Scala, or Clojure, they may encounter similar issues, because these languages also offer meta programming and bring their own language runtime, which uses Java platform reflection, e.g.

http://scalamacros.org/paperstalks/2012-04-28-MetaprogrammingInScala210.pdf

http://clojure-doc.org/articles/language/macros.html

This is clearly not an issue that can be solved by ES alone, so my message is only meant to rise some attention of possible security manager side effects. I will follow the Java 9 jigsaw process to see what the future will bring for Groovy, Scala, Clojure et.al.

@rmuir
Copy link
Contributor Author

rmuir commented Sep 16, 2015

I will follow the Java 9 jigsaw process to see what the future will bring for Groovy, Scala, Clojure et.al.

Interestingly, clojure reported today that they are functional on jigsaw: http://mail.openjdk.java.net/pipermail/jigsaw-dev/2015-September/004609.html

I don't know what they are doing differently... and I plan to look into these more anyway, to at least improve ES scripting integration, but have not had the chance yet. Thanks for trying compilestatic, i was really hoping that would play better... grrr

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@rjernst rjernst added the Team:Core/Infra Meta label for core/infra team label May 4, 2020
@rjernst rjernst added the needs:triage Requires assignment of a team area label label Dec 3, 2020
@jaymode jaymode added help wanted adoptme and removed needs:triage Requires assignment of a team area label v6.0.3 labels Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label help wanted adoptme Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests

10 participants