Java: Make ExecTainted easier to extend #5746
Conversation
To add a method that executes a command, you can now define a class extending ExecMethod.
There was a problem hiding this comment.
You should generalise this a little: let ExecCallable extend Callable instead of Method (then ProcessBuilder's constructor can be an instance), and give it an abstract getAnExecutedArgument, which all overrides will currently define as getArgument(0), though e.g. myExec(int flags, String command) would define as getArgument(1).
java/ql/src/semmle/code/java/JDK.qll
Outdated
| @@ -180,21 +180,36 @@ class TypeFile extends Class { | |||
| /** | |||
| * Any of the methods named `command` on class `java.lang.ProcessBuilder`. | |||
| @@ -6,16 +6,20 @@ library class TypeCommandLine extends Class { | |||
| TypeCommandLine() { hasQualifiedName("org.apache.commons.exec", "CommandLine") } | |||
| } | |||
|
|
|||
| library class MethodCommandLineParse extends ExecMethod { | |||
| library class MethodCommandLineParse extends Method, ExecCallable { | |||
There was a problem hiding this comment.
Let's get rid of the archaic library annotations while we're editing this file. And a few qldoc additions seems like easy inclusions as well.
There was a problem hiding this comment.
Shall I replace library with private?
There was a problem hiding this comment.
If the class isn't referenced anywhere else (i.e. things still compile), then yes, I think that's fine (even though it technically is a breaking change).
| @@ -6,7 +6,9 @@ import semmle.code.java.frameworks.apache.Exec | |||
| /** | |||
| * A method that executes a command. | |||
| execCall.getArgument(i) = this and | ||
| execCallable = execCall.getCallee() and | ||
| i = execCallable.getAnExecutedArgument() |
There was a problem hiding this comment.
Please check locally that the join-order of this looks reasonable, i.e. execute a query that depends on this predicate and check in the log that it doesn't start the pipeline with a join on i.
There was a problem hiding this comment.
You're right, it joins on i first. Adding pragma[only_bind_into] in the expected place fixed it.
We want to avoid joining on `i` first.
owen-mc
force-pushed
the
java/refactor-exec-tainted
branch
from
e856868 to
4b8d4f5
Compare
Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>
owen-mc
changed the title
No change in behaviour - this is just a refactoring. To add a method that executes a command, you can now define a class
extending ExecMethod.