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

Kernel#system fails to execute if SecurityManager denies access to a $PATH entry even if it permits a later one #3983

Closed
trejkaz opened this Issue Jun 28, 2016 · 13 comments

Comments

Projects
None yet
3 participants
@trejkaz

trejkaz commented Jun 28, 2016

Environment

JRuby 9.0.5.0 core + stdlib (stopped using complete because it bundled extra things which clashed with other dependencies).
OS here is Mac OS X 10.11.5.

We run tests with the security manager enabled, mostly to stop people writing files all over the computer.

One such test I tried to write was when someone reported that the system function didn't work.

Expected Behavior

I expect this to pass:

    @Test
    public void testSystem() throws Exception {
        ScriptEngine engine = createEngine();

        ScriptContext context = new SimpleScriptContext();
        StringWriter writer = new StringWriter();
        context.setWriter(writer);

        Object result = engine.eval("system('java -version')", context);
        assertThat(result, is(true));
    }

Our security policy does allow executing java:

  permission java.io.FilePermission "${java.home}", "read, execute";
  permission java.io.FilePermission "${java.home}/-", "read, execute";

Actual Behavior

ShellLauncher gets to the catch block here:

            tuple = ShellLauncher.runAndWaitPid(runtime, args);
        } catch (Exception e) {
            tuple = new long[] {127, -1};   // <-- here
        }

Exception is:

org.jruby.exceptions.RaiseException: (SecurityError) access denied ("java.io.FilePermission" "/Users/tester/bin/echo" "read")

Stack for that:

java.security.AccessControlException: access denied ("java.io.FilePermission" "/Users/tester/bin/echo" "read")
    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.checkRead(SecurityManager.java:888)
    at java.io.File.exists(File.java:814)
    at org.jruby.util.ShellLauncher.tryFile(ShellLauncher.java:340)
    at org.jruby.util.ShellLauncher.isValidFile(ShellLauncher.java:372)
    at org.jruby.util.ShellLauncher.findPathFile(ShellLauncher.java:398)
    at org.jruby.util.ShellLauncher.findPathExecutable(ShellLauncher.java:426)
    at org.jruby.util.ShellLauncher$LaunchConfig.verifyExecutable(ShellLauncher.java:1296)
    at org.jruby.util.ShellLauncher$LaunchConfig.verifyExecutableForShell(ShellLauncher.java:1256)
    at org.jruby.util.ShellLauncher.run(ShellLauncher.java:1447)
    at org.jruby.util.ShellLauncher.run(ShellLauncher.java:1401)
    at org.jruby.util.ShellLauncher.runAndWaitPid(ShellLauncher.java:595)
    at org.jruby.util.ShellLauncher.runAndWaitPid(ShellLauncher.java:434)
    at org.jruby.RubyKernel.systemCommon(RubyKernel.java:1594)
    at org.jruby.RubyKernel.system19(RubyKernel.java:1571)

So it seems like ShellLauncher has some logic where it programmatically searches $PATH for the executable. $HOME/bin is the first entry in my path and the security policy does not allow reading from there, so an AccessControlException is thrown. This then doesn't get caught, and for some reason ShellLauncher immediately aborts. Perhaps what it should be doing is catching that and trying the next location.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jul 5, 2016

Member

you should try doing so - catching SecurityException on org.jruby.util.ShellLauncher.tryFile
its fairly easy to fix, but I'm afraid you might run into another exception elsewhere ... due SecurityManager

Member

kares commented Jul 5, 2016

you should try doing so - catching SecurityException on org.jruby.util.ShellLauncher.tryFile
its fairly easy to fix, but I'm afraid you might run into another exception elsewhere ... due SecurityManager

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 11, 2016

Member

@trejkaz Yes, your theory sounds right. We do have PATH-searching logic, and if it gets a security error on one entry it should proceed to the next.

Member

headius commented Jul 11, 2016

@trejkaz Yes, your theory sounds right. We do have PATH-searching logic, and if it gets a security error on one entry it should proceed to the next.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Jul 11, 2016

Member

@trejkaz Can you try the following patch on your system, please?

diff --git a/core/src/main/java/org/jruby/util/ShellLauncher.java b/core/src/main/java/org/jruby/util/ShellLauncher.java
index 528dab2..655027d 100644
--- a/core/src/main/java/org/jruby/util/ShellLauncher.java
+++ b/core/src/main/java/org/jruby/util/ShellLauncher.java
@@ -398,9 +398,14 @@ public class ShellLauncher {
                 // NOTE: Jruby's handling of tildes is more complete than
                 //       MRI's, which can't handle user names after the tilde
                 //       when searching the executable path
-                pathFile = isValidFile(runtime, fdir, fname, isExec);
-                if (pathFile != null) {
-                    break;
+                try {
+                    pathFile = isValidFile(runtime, fdir, fname, isExec);
+                    if (pathFile != null) {
+                        break;
+                    }
+                } catch (SecurityException se) {
+                    // Security prevented accessing this PATH entry, proceed to next
+                    continue;
                 }
             }
         } else {
Member

headius commented Jul 11, 2016

@trejkaz Can you try the following patch on your system, please?

diff --git a/core/src/main/java/org/jruby/util/ShellLauncher.java b/core/src/main/java/org/jruby/util/ShellLauncher.java
index 528dab2..655027d 100644
--- a/core/src/main/java/org/jruby/util/ShellLauncher.java
+++ b/core/src/main/java/org/jruby/util/ShellLauncher.java
@@ -398,9 +398,14 @@ public class ShellLauncher {
                 // NOTE: Jruby's handling of tildes is more complete than
                 //       MRI's, which can't handle user names after the tilde
                 //       when searching the executable path
-                pathFile = isValidFile(runtime, fdir, fname, isExec);
-                if (pathFile != null) {
-                    break;
+                try {
+                    pathFile = isValidFile(runtime, fdir, fname, isExec);
+                    if (pathFile != null) {
+                        break;
+                    }
+                } catch (SecurityException se) {
+                    // Security prevented accessing this PATH entry, proceed to next
+                    continue;
                 }
             }
         } else {

@headius headius added this to the JRuby 9.1.3.0 milestone Jul 11, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 15, 2016

Member

@trejkaz We'd still like to include this in 9.1.3.0 if it works properly for you.

Note to others: we'll need to look at adding a bit of security magic to the native process logic.

Member

headius commented Aug 15, 2016

@trejkaz We'd still like to include this in 9.1.3.0 if it works properly for you.

Note to others: we'll need to look at adding a bit of security magic to the native process logic.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 15, 2016

Member

We should add a test for this as well...something that has to traverse a directory with no permissions to find one that does before executing the command.

Member

headius commented Aug 15, 2016

We should add a test for this as well...something that has to traverse a directory with no permissions to find one that does before executing the command.

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz Aug 16, 2016

Argh, I knew I forgot to do something. I will try and get onto this tomorrow.

trejkaz commented Aug 16, 2016

Argh, I knew I forgot to do something. I will try and get onto this tomorrow.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 17, 2016

Member

@trejkaz ping :-)

Member

headius commented Aug 17, 2016

@trejkaz ping :-)

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz Aug 18, 2016

I'm trying. I guess this is the typical experience when I try to build something based on Maven, but it's like no version will work. :(

  • 9.0.3.0:

    [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project jruby-core: Compilation failure -> [Help 1]
    
  • 9.0.0.0 (which I swear we used to be building ourselves): same as 9.0.3.0

  • 9.0.5.0 (what we're currently on, though we pull the upstream artifact now): same as 9.0.3.0

What I hate the most about Maven is that when it fails, it gives absolutely no hint to allow me to go and fix it. "Compile failure" doesn't really cut it and they should feel bad for not at least showing what the failure was. I love how they even say "After correcting the problems", as if they have provided enough information to do so.

  • 9.1.2.0: Builds successfully but I get a runtime error which it looks like others have been reporting on other tickets.

    Caused by: java.lang.RuntimeException: Could not load platform constants for OpenFlags
    
  • Current master: same as 9.1.2.0

trejkaz commented Aug 18, 2016

I'm trying. I guess this is the typical experience when I try to build something based on Maven, but it's like no version will work. :(

  • 9.0.3.0:

    [ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project jruby-core: Compilation failure -> [Help 1]
    
  • 9.0.0.0 (which I swear we used to be building ourselves): same as 9.0.3.0

  • 9.0.5.0 (what we're currently on, though we pull the upstream artifact now): same as 9.0.3.0

What I hate the most about Maven is that when it fails, it gives absolutely no hint to allow me to go and fix it. "Compile failure" doesn't really cut it and they should feel bad for not at least showing what the failure was. I love how they even say "After correcting the problems", as if they have provided enough information to do so.

  • 9.1.2.0: Builds successfully but I get a runtime error which it looks like others have been reporting on other tickets.

    Caused by: java.lang.RuntimeException: Could not load platform constants for OpenFlags
    
  • Current master: same as 9.1.2.0

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz Aug 18, 2016

OK, I finally got to the bottom of this. JNR has some magic it's doing with OpenFlags which seemingly parses the class file or something, perhaps as an alternative to using reflection(?). That trips up the security manager, and then URLClassPath masks the error and returns null, which was very hard to diagnose until I got into a debugger. It seems a little illogical that findResource(String) would return null when getClass(String) for the exact same class was able to resolve the class, though. Maybe they should have found a way to do it without this magic...

Anyway, this only tripped up the security manager because I was loading the JRuby jar from a file. The actual security policy is that reading from any jar on the classpath is OK, so I temporarily added the JRuby jar to the policy while testing.

Now it goes through, I see it catch the exception and it then finds /bin/echo, and the test passes. So I think it's OK to call this issue fixed.

I do get some warning on stderr:

     but: was "<script>:1: warning: executable? does not in this environment and will return a dummy value"

I'm not calling executable? myself, so I think the warning is questionable. (Also, how does one parse "does not in this environment"..? It looks like they accidentally the verb.)

trejkaz commented Aug 18, 2016

OK, I finally got to the bottom of this. JNR has some magic it's doing with OpenFlags which seemingly parses the class file or something, perhaps as an alternative to using reflection(?). That trips up the security manager, and then URLClassPath masks the error and returns null, which was very hard to diagnose until I got into a debugger. It seems a little illogical that findResource(String) would return null when getClass(String) for the exact same class was able to resolve the class, though. Maybe they should have found a way to do it without this magic...

Anyway, this only tripped up the security manager because I was loading the JRuby jar from a file. The actual security policy is that reading from any jar on the classpath is OK, so I temporarily added the JRuby jar to the policy while testing.

Now it goes through, I see it catch the exception and it then finds /bin/echo, and the test passes. So I think it's OK to call this issue fixed.

I do get some warning on stderr:

     but: was "<script>:1: warning: executable? does not in this environment and will return a dummy value"

I'm not calling executable? myself, so I think the warning is questionable. (Also, how does one parse "does not in this environment"..? It looks like they accidentally the verb.)

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 18, 2016

Member

So I think it's OK to call this issue fixed.

Ok, that's good to hear.

I do get some warning on stderr

Yeah that's kinda goofy. We should find that warning and fix it so it actually makes sense :-)

I'll call this fixed and at least patch up the wording of that warning.

Member

headius commented Aug 18, 2016

So I think it's OK to call this issue fixed.

Ok, that's good to hear.

I do get some warning on stderr

Yeah that's kinda goofy. We should find that warning and fix it so it actually makes sense :-)

I'll call this fixed and at least patch up the wording of that warning.

@headius headius closed this Aug 18, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 18, 2016

Member

Oh sorry, you meant it's fixed with the patch I provided above, yes? I had not yet committed that to master.

Member

headius commented Aug 18, 2016

Oh sorry, you meant it's fixed with the patch I provided above, yes? I had not yet committed that to master.

headius added a commit that referenced this issue Aug 18, 2016

Allow graceful failure while path searching if security disallows.
This patch allows the PATH searching logic for non-native process
launching to proceed past paths which trigger security exceptions,
such as any path not explicitly permitted for reading by JVM
security policies.

Fixes #3983.
@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Aug 18, 2016

Member

I'd like to see tests for this, but I'm not sure what they'd look like.

Member

headius commented Aug 18, 2016

I'd like to see tests for this, but I'm not sure what they'd look like.

@headius headius added the needs tests label Aug 18, 2016

@trejkaz

This comment has been minimized.

Show comment
Hide comment
@trejkaz

trejkaz Aug 19, 2016

Yeah, I meant after applying the patch myself and running that version, it's fixed. :)

trejkaz commented Aug 19, 2016

Yeah, I meant after applying the patch myself and running that version, it's fixed. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment