Java: Experimental version of Java Command Injection query

[aegilops]

This query is designed to be more sensitive than the existing CodeQL query, java/command-line-injection.

There is a specific GitHub Advanced Security customer who wants this query.

For the case where a dangerous executable is used, such as /bin/sh, the original query tries to determine that the string ends up in the first element of the array used in the call to exec.

In some unusual cases, the approach used cannot track the connection between the static string and its position as the first element of the array, e.g.

String[] args = {"-c", "echo foo"};
Runtime.getRuntime().exec(
    Stream.concat(
        Arrays.stream(new String[]{"/bin/sh"}),
        Arrays.stream(args_2)
    ).toArray(String[]::new)
);

Instead of trying to tell that /bin/sh is in argument 0, we can rule out that it can be anywhere other than 0, which is the approach that this query takes. It is also aware of Stream.concat.

I have some unit tests, but wasn't sure where to put them in the "experimental" hierarchy!

[github-actions]

QHelp previews:

java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExec.qhelp

Command Injection into Runtime.exec() with dangerous command

Code that passes remote user input to an arugment of a call of Runtime.exec that executes a scripting executable will allow the user to execute malicious code.

Recommendation

If possible, use hard-coded string literals to specify the command or script to run, or library to load. Instead of passing the user input directly to the process or library function, examine the user input and then choose among hard-coded string literals.

If the applicable libraries or commands cannot be determined at compile time, then add code to verify that the user input string is safe before using it.

Example

The following example shows code that takes a shell script that can be changed maliciously by a user, and passes it straight to the array going into Runtime.exec without examining it first.

class Test {
    public static void main(String[] args) {
        String script = System.getenv("SCRIPTNAME");
        if (script != null) {
            // BAD: The script to be executed by /bin/sh is controlled by the user.
            Runtime.getRuntime().exec(new String[]{"/bin/sh", script});
        }
    }
}

References

• OWASP: Command Injection.
• SEI CERT Oracle Coding Standard for Java: IDS07-J. Sanitize untrusted data passed to the Runtime.exec() method.
• Common Weakness Enumeration: CWE-78.

java/ql/src/experimental/Security/CWE/CWE-078/CommandInjectionRuntimeExecLocal.qhelp

Command Injection into Runtime.exec() with dangerous command

Code that passes local user input to an arugment of a call of Runtime.exec that executes a scripting executable will allow the user to execute malicious code.

Recommendation

If possible, use hard-coded string literals to specify the command or script to run, or library to load. Instead of passing the user input directly to the process or library function, examine the user input and then choose among hard-coded string literals.

If the applicable libraries or commands cannot be determined at compile time, then add code to verify that the user input string is safe before using it.

Example

The following example shows code that takes a shell script that can be changed maliciously by a user, and passes it straight to the array going into Runtime.exec without examining it first.

class Test {
    public static void main(String[] args) {
        String script = System.getenv("SCRIPTNAME");
        if (script != null) {
            // BAD: The script to be executed by /bin/sh is controlled by the user.
            Runtime.getRuntime().exec(new String[]{"/bin/sh", script});
        }
    }
}

References

• OWASP: Command Injection.
• SEI CERT Oracle Coding Standard for Java: IDS07-J. Sanitize untrusted data passed to the Runtime.exec() method.
• Common Weakness Enumeration: CWE-78.

[aegilops]

I've made all of the changes you suggested @atorralba, except for the varargs one - as I commented above, I don't think exec() can take varargs.

I'm failing to get it to pass the "compile all queries" check, since it thinks the formatting isn't correct. I've run the QL formatter in VSCode, so I'm not sure what's wrong.

[atorralba]

Hey @aegilops, thanks for being so thorough, and you're absolutely right about the varargs thing, I was over-engineering.

I pushed a commit with the formatting changes (my VSCode did format it right 🤷), happy to approve this after you git pull --rebase and push your updates.

[aegilops]

Lots of green ticks ✅, I like the look of that

I spoke too soon, running the whole Java suite take a while! Now we have all green...

[atorralba]

Removing some (I think!) Emacs comments, otherwise LGTM :)

[atorralba]

Thanks for your patience @aegilops!