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

Fix CVE-2024-5979 AstRunTool #16351

Closed
wendycwong opened this issue Aug 1, 2024 · 6 comments · Fixed by #16366
Closed

Fix CVE-2024-5979 AstRunTool #16351

wendycwong opened this issue Aug 1, 2024 · 6 comments · Fixed by #16366

Comments

@wendycwong
Copy link
Contributor

wendycwong commented Aug 1, 2024

AstRunTool can crash H2O if bad inputs are provided. This is a subtask of #16339

@wendycwong
Copy link
Contributor Author

image

wendycwong pushed a commit that referenced this issue Aug 8, 2024
wendycwong pushed a commit that referenced this issue Aug 8, 2024
@wendycwong
Copy link
Contributor Author

After using @tomasfryda suggestion, I was able to reproduce the error using his code:

curl http://localhost:54321/99/Rapids --data-urlencode 'ast=(run_tool "MojoConvertTool" ["","","",""])'

However, this is due to the following code in MojoConvertTool.java:

    File mojoFile = new File(args[0]);
    if (!mojoFile.isFile()) {
        System.err.println("Specified MOJO file (" + mojoFile.getAbsolutePath() + ") doesn't exist!");
        System.exit(2);
    }

Note the System.exit(2) is what causes our H2O-3 cluster to exit.

After I change the System.exit(2) to

    if (!mojoFile.isFile()) {
        System.err.println("Specified MOJO file (" + mojoFile.getAbsolutePath() + ") doesn't exist!");
        throw new IOException("*********  Specified MOJO file (" + mojoFile.getAbsolutePath() + ") doesn't exist!" +
                " ***********");
        //System.exit(2);
    }

I ran the same code and H2O-3 did not crash.

Hence, any time we run a command that call System.ext() instead of throwing an error will always cause H2O-3 to crash.

@wendycwong
Copy link
Contributor Author

wendycwong commented Aug 8, 2024

However, there is a way to prevent this here: https://www.quora.com/What-is-the-best-way-to-prevent-JVM-from-exiting-after-calling-System-exit-in-Java-programming-language

In Java, when you call System.exit() method, it directly terminates the JVM (Java Virtual Machine) and shuts down the program. This behavior is by design and is intended to forcefully terminate the JVM and end the program.

If you want to prevent the JVM from exiting after calling System.exit(), you can use a SecurityManager to intercept the call to System.exit() and prevent it from shutting down the JVM. Here's a basic example of how you can do this:

public class NoExitSecurityManager extends SecurityManager {
@OverRide
public void checkExit(int status) {
throw new SecurityException("System.exit() is not allowed");
}
}

public class Main {
public static void main(String[] args) {
System.setSecurityManager(new NoExitSecurityManager());

    // Your code here 

    // Call System.exit() here 
    System.exit(0); 
} 

}
In this example, we create a custom SecurityManager called NoExitSecurityManager that overrides the checkExit() method to throw a SecurityException instead of allowing the JVM to exit. By setting this custom SecurityManager using System.setSecurityManager(), you can prevent the JVM from exiting when System.exit() is called.

Please note that using a SecurityManager to prevent the JVM from exiting after calling System.exit() is not a common practice and should be used with caution. It can have unexpected consequences and may not work as expected in all scenarios.

Additionally, it's generally recommended to avoid using System.exit() for normal program flow control. It's considered a harsh way to terminate a program and should be used sparingly, typically only for exceptional cases. Instead, you should design your program flow in a way that does not rely on calling System.exit() to exit the program.

Profile photo for Syed Mohsin Javed
Syed Mohsin Javed
BS in Mechancial Engineering & Marketing, University of Lahore (Graduated 2020)Author has 394 answers and 123.2K answer views1y
One way to prevent the JVM from exiting after calling System.exit() is to use a Shutdown Hook. A Shutdown Hook is a thread that is registered with the JVM and is executed when the JVM is shutting down. The hook can be used to perform cleanup activities, such as closing files or database connections.

Another way to prevent the JVM from exiting is to use the Java SecurityManager. The SecurityManager can be used to restrict access to certain system resources, such as the System.exit() method. By default, the SecurityManager allows all actions, but this can be changed by overriding the checkExit() method.

yet another way is to set the java.lang.SecurityManager property to "SecurityManager" in the java.security file. This will cause the JVM to exit if the SecurityManager is not properly configured.

The best way to prevent the JVM from exiting after calling System.exit() is to use a combination of the Shutdown Hook and the SecurityManager. The Shutdown Hook can be used to perform cleanup activities, such as closing files or database connections. The SecurityManager can be used to restrict access to certain system resources, such as the System.exit() method. By combining these two techniques, you can ensure that the JVM will not exit unless you explicitly allow it to.

@wendycwong
Copy link
Contributor Author

A workaround proposed by @mmalohlava is as follows:

  1. In MojoConvertTool.java, add a function main_internal which will not call System.exit() but will throw exceptions. The main function will call main_internal and call System.exit() as it wishes.
  2. We will add documentation for AstRunTool and tell people to put their System.exit() in their main function and not in their main_internal. For commands written with this implementation will not run into the problem of H2O-3 shutting down when using AstRunTool.
  3. Back at H2O-3 in AstRUnTool, we can call main_internal if it exists and if not call main. Worst case scenario will be commands not having main_internal and only have main which have System.exit() will crash as we predicted in point 2. The fault will be on the user and not us since we warn them and they refuse to listen.

@tomasfryda
Copy link
Contributor

@wendycwong The issue is not just in MojoConvertTool.java so do you plan to create main_internal for all the tools that call System.exit? ($ grep -R System.exit | wc -l # => 73)

I'd be in favor of doing the modification (add main_internal) for all the tools that call System.exit.

@wendycwong
Copy link
Contributor Author

Yes, we should add that to our own tools main_internal.

@wendycwong wendycwong assigned krasinski and unassigned wendycwong Aug 12, 2024
@krasinski krasinski added this to the 3.46.0.6 milestone Oct 21, 2024
wendycwong pushed a commit that referenced this issue Oct 23, 2024
* [GH-16351] Do not call System.exit from water.tools

* add mainInternal to EncryptionTool

* make the err msg short

* make the err msg short - part 2

* improve error handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants