From 6bfc0afaeb1cfc0e11911197e82c9c9bf4cce35d Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 17 Sep 2020 15:01:04 +0100 Subject: [PATCH 1/9] Java: Improve the ExecTainted query --- .../src/Security/CWE/CWE-078/ExecCommon.qll | 7 +- .../src/Security/CWE/CWE-078/ExecTainted.ql | 2 +- .../code/java/security/CommandArguments.qll | 214 ++++++++++++++++++ 3 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 java/ql/src/semmle/code/java/security/CommandArguments.qll diff --git a/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll b/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll index 4d6bf8c848af..c0025043fcec 100644 --- a/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll +++ b/java/ql/src/Security/CWE/CWE-078/ExecCommon.qll @@ -1,5 +1,6 @@ import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.ExternalProcess +import semmle.code.java.security.CommandArguments private class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration { RemoteUserInputToArgumentToExecFlowConfig() { @@ -11,7 +12,11 @@ private class RemoteUserInputToArgumentToExecFlowConfig extends TaintTracking::C override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec } override predicate isSanitizer(DataFlow::Node node) { - node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType + node.getType() instanceof PrimitiveType + or + node.getType() instanceof BoxedType + or + isSafeCommandArgument(node.asExpr()) } } diff --git a/java/ql/src/Security/CWE/CWE-078/ExecTainted.ql b/java/ql/src/Security/CWE/CWE-078/ExecTainted.ql index 353df33afed4..7b191e762410 100644 --- a/java/ql/src/Security/CWE/CWE-078/ExecTainted.ql +++ b/java/ql/src/Security/CWE/CWE-078/ExecTainted.ql @@ -17,7 +17,7 @@ import semmle.code.java.security.ExternalProcess import ExecCommon import DataFlow::PathGraph -from DataFlow::PathNode source, DataFlow::PathNode sink, StringArgumentToExec execArg +from DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg where execTainted(source, sink, execArg) select execArg, source, sink, "$@ flows to here and is used in a command.", source.getNode(), "User-provided value" diff --git a/java/ql/src/semmle/code/java/security/CommandArguments.qll b/java/ql/src/semmle/code/java/security/CommandArguments.qll new file mode 100644 index 000000000000..f0b94d7b482c --- /dev/null +++ b/java/ql/src/semmle/code/java/security/CommandArguments.qll @@ -0,0 +1,214 @@ +import java +import semmle.code.java.dataflow.SSA +import semmle.code.java.Collections + +/** + * Holds if `ex` is used safely as an argument to a command; + * i.e. it's not in the first position and it's not a shell command. + */ +predicate isSafeCommandArgument(Expr ex) { + exists(ArrayInit ai, int i | + ex = ai.getInit(i) and + i > 0 and + not isShell(ai.getInit(0)) + ) + or + exists(CommandArgumentList cal | + cal.isNotShell() and + ex = cal.getASubsequentAdd().getArgument(0) + ) + or + exists(CommandArgumentArray caa | + caa.isNotShell() and + ex = caa.getAWrite(any(int i | i > 0)) + ) +} + +/** + * Holds if the given expression is the name of a shell command such as bash or python + */ +private predicate isShell(Expr ex) { + exists(string cmd | cmd = ex.(StringLiteral).getValue() | + cmd.regexpMatch(".*(sh|javac?|python[23]?|osascript|cmd)(\\.exe)?$") + ) + or + exists(SsaVariable ssa | + ex = ssa.getAUse() and + isShell(ssa.getAnUltimateDefinition().(SsaExplicitUpdate).getDefiningExpr()) + ) + or + isShell(ex.(Assignment).getRhs()) + or + isShell(ex.(LocalVariableDeclExpr).getInit()) +} + +/** + * A type that could be a list of strings. Includes raw `List` types. + */ +private class ListOfStringType extends CollectionType { + ListOfStringType() { + this.getSourceDeclaration().getASourceSupertype*().hasQualifiedName("java.util", "List") and + this.getElementType().getASubtype*() instanceof TypeString + } +} + +/** + * A variable that could be used as a list of arguments to a command. + */ +private class CommandArgumentList extends SsaExplicitUpdate { + CommandArgumentList() { + this.getSourceVariable().getType() instanceof ListOfStringType and + forex(CollectionMutation ma | ma.getQualifier() = this.getAUse() | + ma.getMethod().getName().matches("add%") + ) + } + + /** Gets a use of the veriable for which the list could be empty. */ + private RValue getAUseBeforeFirstAdd() { + result = getAFirstUse() + or + exists(RValue mid | + mid = getAUseBeforeFirstAdd() and + adjacentUseUse(mid, result) and + not exists(MethodAccess ma | + mid = ma.getQualifier() and + ma.getMethod().hasName("add") + ) + ) + } + + /** + * Gets an addition to this list, i.e. a call to an `add` or `addAll` method. + */ + MethodAccess getAnAdd() { + result.getQualifier() = getAUse() and + result.getMethod().getName().matches("add%") + } + + /** Gets an addition to this list which could be its first element. */ + MethodAccess getAFirstAdd() { + result = getAnAdd() and + result.getQualifier() = getAUseBeforeFirstAdd() + } + + /** Gets an addition to this list which is not the first element. */ + MethodAccess getASubsequentAdd() { + result = getAnAdd() and + not result = getAFirstAdd() + } + + /** Holds if the first element of this list isn't a shell command. */ + predicate isNotShell() { + forex(MethodAccess ma | ma = getAFirstAdd() | not isShell(ma.getArgument(0))) + } +} + +/** + * The type `String[]`. + */ +private class ArrayOfStringType extends Array { + ArrayOfStringType() { this.getElementType() instanceof TypeString } +} + +private predicate arrayLValue(ArrayAccess acc) { exists(Assignment a | a.getDest() = acc) } + +/** + * A variable that could be an array of arguments to a command. + */ +private class CommandArgumentArray extends SsaExplicitUpdate { + CommandArgumentArray() { + this.getSourceVariable().getType() instanceof ArrayOfStringType and + forall(ArrayAccess a | a.getArray() = getAUse() and arrayLValue(a) | + a.getIndexExpr() instanceof CompileTimeConstantExpr + ) + } + + /** Gets an expression that is written to the given index of this array at he given use. */ + Expr getAWrite(int index, RValue use) { + exists(Assignment a, ArrayAccess acc | + acc.getArray() = use and + use = this.getAUse() and + index = acc.getIndexExpr().(CompileTimeConstantExpr).getIntValue() and + acc = a.getDest() and + result = a.getRhs() + ) + } + + /** Gets an expression that is written to the given index of this array. */ + Expr getAWrite(int index) { result = getAWrite(index, _) } + + predicate isNotShell() { + exists(Expr e | e = this.(CommandArgArrayImmutableFirst).getFirstElement() | + not isShell(e) + ) + } +} + +/** + * A `CommandArgArray` whose element at index 0 is never written to, except possibly once to initialise it. + */ +private class CommandArgArrayImmutableFirst extends CommandArgumentArray { + CommandArgArrayImmutableFirst() { + this.getDefiningExpr() instanceof ImmutableFirstArrayExpr and + forall(RValue use | exists(this.getAWrite(0, use)) | use = this.getAFirstUse()) + } + + /** Gets the first element of this array. */ + Expr getFirstElement() { + result = getAWrite(0) + or + not exists(getAWrite(0)) and + result = getDefiningExpr().(ImmutableFirstArrayExpr).getFirstElement() + } +} + +/** + * An expression that evaluates to an array of strings whose first element is immutable. + */ +private class ImmutableFirstArrayExpr extends Expr { + ImmutableFirstArrayExpr() { + this.getType() instanceof ArrayOfStringType and + ( + this.(Assignment).getRhs() instanceof ImmutableFirstArrayExpr + or + this.(LocalVariableDeclExpr).getInit() instanceof ImmutableFirstArrayExpr + or + this instanceof ArrayInit + or + this instanceof ArrayCreationExpr + or + this.(RValue) = any(CommandArgArrayImmutableFirst caa).getAUse() + or + exists(MethodAccess ma, Method m | + ma.getMethod() = m and + m.getDeclaringType().hasQualifiedName("java.util", "Arrays") and + m.hasName("copyOf") and + ma.getArgument(0) instanceof ImmutableFirstArrayExpr + ) + or + exists(Field f | this = f.getAnAccess() and + f.isFinal() and + f.getInitializer() instanceof ImmutableFirstArrayExpr + ) + ) + } + + /** Gets the first element of this array. */ + Expr getFirstElement() { + result = this.(Assignment).getRhs().(ImmutableFirstArrayExpr).getFirstElement() + or + result = this.(LocalVariableDeclExpr).getInit().(ImmutableFirstArrayExpr).getFirstElement() + or + exists(CommandArgArrayImmutableFirst caa | this = caa.getAUse() | + result = caa.getFirstElement() + ) + or + result = this.(MethodAccess).getArgument(0).(ImmutableFirstArrayExpr).getFirstElement() + or + result = this.(FieldAccess).getField().getInitializer() + or + result = this.(ArrayInit).getInit(0) + or + result = this.(ArrayCreationExpr).getInit().getInit(0) + } +} From b6cf1cce207c32f28092f82454c6ae9d17441886 Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 17 Sep 2020 15:53:04 +0100 Subject: [PATCH 2/9] Java: Make the equivalent changes to ExecTaintedLocal --- java/ql/src/Security/CWE/CWE-078/ExecTaintedLocal.ql | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-078/ExecTaintedLocal.ql b/java/ql/src/Security/CWE/CWE-078/ExecTaintedLocal.ql index d809f1bb5dd0..f7c49dbd4f6e 100644 --- a/java/ql/src/Security/CWE/CWE-078/ExecTaintedLocal.ql +++ b/java/ql/src/Security/CWE/CWE-078/ExecTaintedLocal.ql @@ -14,6 +14,7 @@ import semmle.code.java.Expr import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.ExternalProcess +import semmle.code.java.security.CommandArguments import DataFlow::PathGraph class LocalUserInputToArgumentToExecFlowConfig extends TaintTracking::Configuration { @@ -24,12 +25,16 @@ class LocalUserInputToArgumentToExecFlowConfig extends TaintTracking::Configurat override predicate isSink(DataFlow::Node sink) { sink.asExpr() instanceof ArgumentToExec } override predicate isSanitizer(DataFlow::Node node) { - node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType + node.getType() instanceof PrimitiveType + or + node.getType() instanceof BoxedType + or + isSafeCommandArgument(node.asExpr()) } } from - DataFlow::PathNode source, DataFlow::PathNode sink, StringArgumentToExec execArg, + DataFlow::PathNode source, DataFlow::PathNode sink, ArgumentToExec execArg, LocalUserInputToArgumentToExecFlowConfig conf where conf.hasFlowPath(source, sink) and sink.getNode().asExpr() = execArg select execArg, source, sink, "$@ flows to here and is used in a command.", source.getNode(), From fcfc8367203b0eee0977efc49b975d9f331a5020 Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 17 Sep 2020 16:47:55 +0100 Subject: [PATCH 3/9] Java: Add tests for ExecTainted --- .../security/CWE-078/ExecRelative.expected | 1 + .../security/CWE-078/ExecRelative.qlref | 1 + .../CWE-078/ExecTaintedLocal.expected | 27 ++++++++ .../security/CWE-078/ExecTaintedLocal.qlref | 1 + .../security/CWE-078/ExecUnescaped.expected | 2 + .../security/CWE-078/ExecUnescaped.qlref | 1 + .../query-tests/security/CWE-078/Test.java | 64 +++++++++++++++++++ 7 files changed, 97 insertions(+) create mode 100644 java/ql/test/query-tests/security/CWE-078/ExecRelative.expected create mode 100644 java/ql/test/query-tests/security/CWE-078/ExecRelative.qlref create mode 100644 java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.expected create mode 100644 java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.qlref create mode 100644 java/ql/test/query-tests/security/CWE-078/ExecUnescaped.expected create mode 100644 java/ql/test/query-tests/security/CWE-078/ExecUnescaped.qlref create mode 100644 java/ql/test/query-tests/security/CWE-078/Test.java diff --git a/java/ql/test/query-tests/security/CWE-078/ExecRelative.expected b/java/ql/test/query-tests/security/CWE-078/ExecRelative.expected new file mode 100644 index 000000000000..066a36c66c87 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-078/ExecRelative.expected @@ -0,0 +1 @@ +| Test.java:50:46:50:49 | "ls" | Command with a relative path 'ls' is executed. | diff --git a/java/ql/test/query-tests/security/CWE-078/ExecRelative.qlref b/java/ql/test/query-tests/security/CWE-078/ExecRelative.qlref new file mode 100644 index 000000000000..42aa816c1772 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-078/ExecRelative.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-078/ExecRelative.ql diff --git a/java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.expected b/java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.expected new file mode 100644 index 000000000000..4a29161c63c1 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.expected @@ -0,0 +1,27 @@ +edges +| Test.java:6:35:6:44 | arg : String | Test.java:7:44:7:69 | ... + ... | +| Test.java:6:35:6:44 | arg : String | Test.java:10:29:10:74 | new String[] | +| Test.java:6:35:6:44 | arg : String | Test.java:18:29:18:31 | cmd | +| Test.java:6:35:6:44 | arg : String | Test.java:24:29:24:32 | cmd1 | +| Test.java:28:38:28:47 | arg : String | Test.java:29:44:29:64 | ... + ... | +| Test.java:57:27:57:39 | args : String[] | Test.java:60:20:60:22 | arg : String | +| Test.java:57:27:57:39 | args : String[] | Test.java:61:23:61:25 | arg : String | +| Test.java:60:20:60:22 | arg : String | Test.java:6:35:6:44 | arg : String | +| Test.java:61:23:61:25 | arg : String | Test.java:28:38:28:47 | arg : String | +nodes +| Test.java:6:35:6:44 | arg : String | semmle.label | arg : String | +| Test.java:7:44:7:69 | ... + ... | semmle.label | ... + ... | +| Test.java:10:29:10:74 | new String[] | semmle.label | new String[] | +| Test.java:18:29:18:31 | cmd | semmle.label | cmd | +| Test.java:24:29:24:32 | cmd1 | semmle.label | cmd1 | +| Test.java:28:38:28:47 | arg : String | semmle.label | arg : String | +| Test.java:29:44:29:64 | ... + ... | semmle.label | ... + ... | +| Test.java:57:27:57:39 | args : String[] | semmle.label | args : String[] | +| Test.java:60:20:60:22 | arg : String | semmle.label | arg : String | +| Test.java:61:23:61:25 | arg : String | semmle.label | arg : String | +#select +| Test.java:7:44:7:69 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:7:44:7:69 | ... + ... | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value | +| Test.java:10:29:10:74 | new String[] | Test.java:57:27:57:39 | args : String[] | Test.java:10:29:10:74 | new String[] | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value | +| Test.java:18:29:18:31 | cmd | Test.java:57:27:57:39 | args : String[] | Test.java:18:29:18:31 | cmd | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value | +| Test.java:24:29:24:32 | cmd1 | Test.java:57:27:57:39 | args : String[] | Test.java:24:29:24:32 | cmd1 | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value | +| Test.java:29:44:29:64 | ... + ... | Test.java:57:27:57:39 | args : String[] | Test.java:29:44:29:64 | ... + ... | $@ flows to here and is used in a command. | Test.java:57:27:57:39 | args | User-provided value | diff --git a/java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.qlref b/java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.qlref new file mode 100644 index 000000000000..18f968747e92 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-078/ExecTaintedLocal.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-078/ExecTaintedLocal.ql diff --git a/java/ql/test/query-tests/security/CWE-078/ExecUnescaped.expected b/java/ql/test/query-tests/security/CWE-078/ExecUnescaped.expected new file mode 100644 index 000000000000..5058f1bb26e9 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-078/ExecUnescaped.expected @@ -0,0 +1,2 @@ +| Test.java:7:44:7:69 | ... + ... | Command line is built with string concatenation. | +| Test.java:29:44:29:64 | ... + ... | Command line is built with string concatenation. | diff --git a/java/ql/test/query-tests/security/CWE-078/ExecUnescaped.qlref b/java/ql/test/query-tests/security/CWE-078/ExecUnescaped.qlref new file mode 100644 index 000000000000..1ee86c5e76ab --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-078/ExecUnescaped.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-078/ExecUnescaped.ql diff --git a/java/ql/test/query-tests/security/CWE-078/Test.java b/java/ql/test/query-tests/security/CWE-078/Test.java new file mode 100644 index 000000000000..6e185b86bbd0 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-078/Test.java @@ -0,0 +1,64 @@ +import java.lang.ProcessBuilder; +import java.util.List; +import java.util.ArrayList; + +class Test { + public static void shellCommand(String arg) { + ProcessBuilder pb = new ProcessBuilder("/bin/bash -c echo " + arg); + pb.start(); + + pb = new ProcessBuilder(new String[]{"/bin/bash", "-c", "echo " + arg}); + pb.start(); + + List cmd = new ArrayList(); + cmd.add("/bin/bash"); + cmd.add("-c"); + cmd.add("echo " + arg); + + pb = new ProcessBuilder(cmd); + pb.start(); + + String[] cmd1 = new String[]{"/bin/bash", "-c", ""}; + cmd1[1] = "echo " + arg; + + pb = new ProcessBuilder(cmd1); + pb.start(); + } + + public static void nonShellCommand(String arg) { + ProcessBuilder pb = new ProcessBuilder("./customTool " + arg); + pb.start(); + + pb = new ProcessBuilder(new String[]{"./customTool", arg}); + pb.start(); + + List cmd = new ArrayList(); + cmd.add("./customTool"); + cmd.add(arg); + + pb = new ProcessBuilder(cmd); + pb.start(); + + String[] cmd1 = new String[]{"./customTool", ""}; + cmd1[1] = arg; + + pb = new ProcessBuilder(cmd1); + pb.start(); + } + + public static void relativeCommand() { + ProcessBuilder pb = new ProcessBuilder("ls"); + pb.start(); + + pb = new ProcessBuilder("/bin/ls"); + pb.start(); + } + + public static void main(String[] args) { + String arg = args.length > 1 ? args[1] : "default"; + + shellCommand(arg); + nonShellCommand(arg); + relativeCommand(); + } +} \ No newline at end of file From 810baad63ffce5d94eab63ae6553629fbd8b1a4d Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 17 Sep 2020 17:13:55 +0100 Subject: [PATCH 4/9] Java: Fix formatting --- .../semmle/code/java/security/CommandArguments.qll | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/CommandArguments.qll b/java/ql/src/semmle/code/java/security/CommandArguments.qll index f0b94d7b482c..fa659d1694c0 100644 --- a/java/ql/src/semmle/code/java/security/CommandArguments.qll +++ b/java/ql/src/semmle/code/java/security/CommandArguments.qll @@ -137,11 +137,9 @@ private class CommandArgumentArray extends SsaExplicitUpdate { /** Gets an expression that is written to the given index of this array. */ Expr getAWrite(int index) { result = getAWrite(index, _) } - predicate isNotShell() { - exists(Expr e | e = this.(CommandArgArrayImmutableFirst).getFirstElement() | - not isShell(e) - ) - } + predicate isNotShell() { + exists(Expr e | e = this.(CommandArgArrayImmutableFirst).getFirstElement() | not isShell(e)) + } } /** @@ -186,7 +184,8 @@ private class ImmutableFirstArrayExpr extends Expr { ma.getArgument(0) instanceof ImmutableFirstArrayExpr ) or - exists(Field f | this = f.getAnAccess() and + exists(Field f | + this = f.getAnAccess() and f.isFinal() and f.getInitializer() instanceof ImmutableFirstArrayExpr ) From 69fd579dfd35fa638f3f5629095c4c0c7473ccf0 Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 17 Sep 2020 17:37:16 +0100 Subject: [PATCH 5/9] Java: Fix QLDoc --- java/ql/src/semmle/code/java/security/CommandArguments.qll | 3 +++ 1 file changed, 3 insertions(+) diff --git a/java/ql/src/semmle/code/java/security/CommandArguments.qll b/java/ql/src/semmle/code/java/security/CommandArguments.qll index fa659d1694c0..6a3b224f4083 100644 --- a/java/ql/src/semmle/code/java/security/CommandArguments.qll +++ b/java/ql/src/semmle/code/java/security/CommandArguments.qll @@ -1,3 +1,6 @@ +/** + * Definitions for reasoning about lists and arrays that are to be used as arguments to an external process. + */ import java import semmle.code.java.dataflow.SSA import semmle.code.java.Collections From 9c643ec1cdb9bf2a5068ce5638d0a216e1c94b64 Mon Sep 17 00:00:00 2001 From: Joe Date: Thu, 17 Sep 2020 17:46:05 +0100 Subject: [PATCH 6/9] Java: Fix formatting --- java/ql/src/semmle/code/java/security/CommandArguments.qll | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/java/ql/src/semmle/code/java/security/CommandArguments.qll b/java/ql/src/semmle/code/java/security/CommandArguments.qll index 6a3b224f4083..709a687cf7dc 100644 --- a/java/ql/src/semmle/code/java/security/CommandArguments.qll +++ b/java/ql/src/semmle/code/java/security/CommandArguments.qll @@ -1,6 +1,7 @@ -/** +/** * Definitions for reasoning about lists and arrays that are to be used as arguments to an external process. */ + import java import semmle.code.java.dataflow.SSA import semmle.code.java.Collections From 3cc38feebca8016c152ebe4c80af963c9d024ac1 Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 18 Sep 2020 14:51:38 +0100 Subject: [PATCH 7/9] Fix a couple of typos in QLDoc comments --- java/ql/src/semmle/code/java/security/CommandArguments.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/CommandArguments.qll b/java/ql/src/semmle/code/java/security/CommandArguments.qll index 709a687cf7dc..595a1517db78 100644 --- a/java/ql/src/semmle/code/java/security/CommandArguments.qll +++ b/java/ql/src/semmle/code/java/security/CommandArguments.qll @@ -67,7 +67,7 @@ private class CommandArgumentList extends SsaExplicitUpdate { ) } - /** Gets a use of the veriable for which the list could be empty. */ + /** Gets a use of the variable for which the list could be empty. */ private RValue getAUseBeforeFirstAdd() { result = getAFirstUse() or @@ -127,7 +127,7 @@ private class CommandArgumentArray extends SsaExplicitUpdate { ) } - /** Gets an expression that is written to the given index of this array at he given use. */ + /** Gets an expression that is written to the given index of this array at the given use. */ Expr getAWrite(int index, RValue use) { exists(Assignment a, ArrayAccess acc | acc.getArray() = use and From abb1731be77cdfb4105fe471b3519d88b8783739 Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 18 Sep 2020 15:21:03 +0100 Subject: [PATCH 8/9] Java: Simplify the implementation of ExecTainted --- .../code/java/security/CommandArguments.qll | 88 +++++++------------ 1 file changed, 32 insertions(+), 56 deletions(-) diff --git a/java/ql/src/semmle/code/java/security/CommandArguments.qll b/java/ql/src/semmle/code/java/security/CommandArguments.qll index 595a1517db78..ebc30eb22df1 100644 --- a/java/ql/src/semmle/code/java/security/CommandArguments.qll +++ b/java/ql/src/semmle/code/java/security/CommandArguments.qll @@ -18,12 +18,12 @@ predicate isSafeCommandArgument(Expr ex) { ) or exists(CommandArgumentList cal | - cal.isNotShell() and + not cal.isShell() and ex = cal.getASubsequentAdd().getArgument(0) ) or - exists(CommandArgumentArray caa | - caa.isNotShell() and + exists(CommandArgArrayImmutableFirst caa | + not caa.isShell() and ex = caa.getAWrite(any(int i | i > 0)) ) } @@ -101,9 +101,9 @@ private class CommandArgumentList extends SsaExplicitUpdate { not result = getAFirstAdd() } - /** Holds if the first element of this list isn't a shell command. */ - predicate isNotShell() { - forex(MethodAccess ma | ma = getAFirstAdd() | not isShell(ma.getArgument(0))) + /** Holds if the first element of this list is a shell command. */ + predicate isShell() { + exists(MethodAccess ma | ma = getAFirstAdd() and isShell(ma.getArgument(0))) } } @@ -140,10 +140,6 @@ private class CommandArgumentArray extends SsaExplicitUpdate { /** Gets an expression that is written to the given index of this array. */ Expr getAWrite(int index) { result = getAWrite(index, _) } - - predicate isNotShell() { - exists(Expr e | e = this.(CommandArgArrayImmutableFirst).getFirstElement() | not isShell(e)) - } } /** @@ -151,7 +147,7 @@ private class CommandArgumentArray extends SsaExplicitUpdate { */ private class CommandArgArrayImmutableFirst extends CommandArgumentArray { CommandArgArrayImmutableFirst() { - this.getDefiningExpr() instanceof ImmutableFirstArrayExpr and + (exists(getAWrite(0)) or exists(firstElementOf(this.getDefiningExpr()))) and forall(RValue use | exists(this.getAWrite(0, use)) | use = this.getAFirstUse()) } @@ -160,58 +156,38 @@ private class CommandArgArrayImmutableFirst extends CommandArgumentArray { result = getAWrite(0) or not exists(getAWrite(0)) and - result = getDefiningExpr().(ImmutableFirstArrayExpr).getFirstElement() + result = firstElementOf(getDefiningExpr()) } -} -/** - * An expression that evaluates to an array of strings whose first element is immutable. - */ -private class ImmutableFirstArrayExpr extends Expr { - ImmutableFirstArrayExpr() { - this.getType() instanceof ArrayOfStringType and - ( - this.(Assignment).getRhs() instanceof ImmutableFirstArrayExpr - or - this.(LocalVariableDeclExpr).getInit() instanceof ImmutableFirstArrayExpr - or - this instanceof ArrayInit - or - this instanceof ArrayCreationExpr - or - this.(RValue) = any(CommandArgArrayImmutableFirst caa).getAUse() - or - exists(MethodAccess ma, Method m | - ma.getMethod() = m and - m.getDeclaringType().hasQualifiedName("java.util", "Arrays") and - m.hasName("copyOf") and - ma.getArgument(0) instanceof ImmutableFirstArrayExpr - ) - or - exists(Field f | - this = f.getAnAccess() and - f.isFinal() and - f.getInitializer() instanceof ImmutableFirstArrayExpr - ) - ) - } + /** Holds if the first element of this array is a shell command. */ + predicate isShell() { isShell(getFirstElement()) } +} - /** Gets the first element of this array. */ - Expr getFirstElement() { - result = this.(Assignment).getRhs().(ImmutableFirstArrayExpr).getFirstElement() +/** Gets the first element of an imutable array of strings */ +private Expr firstElementOf(Expr arr) { + arr.getType() instanceof ArrayOfStringType and + ( + result = firstElementOf(arr.(Assignment).getRhs()) or - result = this.(LocalVariableDeclExpr).getInit().(ImmutableFirstArrayExpr).getFirstElement() + result = firstElementOf(arr.(LocalVariableDeclExpr).getInit()) or - exists(CommandArgArrayImmutableFirst caa | this = caa.getAUse() | - result = caa.getFirstElement() - ) + exists(CommandArgArrayImmutableFirst caa | arr = caa.getAUse() | result = caa.getFirstElement()) or - result = this.(MethodAccess).getArgument(0).(ImmutableFirstArrayExpr).getFirstElement() + exists(MethodAccess ma, Method m | + ma.getMethod() = m and + m.getDeclaringType().hasQualifiedName("java.util", "Arrays") and + m.hasName("copyOf") and + result = firstElementOf(ma.getArgument(0)) + ) or - result = this.(FieldAccess).getField().getInitializer() + exists(Field f | + f.isStatic() and + arr.(FieldRead).getField() = f and + result = firstElementOf(f.getInitializer()) + ) or - result = this.(ArrayInit).getInit(0) + result = arr.(ArrayInit).getInit(0) or - result = this.(ArrayCreationExpr).getInit().getInit(0) - } + result = arr.(ArrayCreationExpr).getInit().getInit(0) + ) } From 9baf2b9eff89bfa3dad5d88ee3bbc655be5ed6bc Mon Sep 17 00:00:00 2001 From: Joe Date: Fri, 18 Sep 2020 15:42:03 +0100 Subject: [PATCH 9/9] Fix cartesian product --- java/ql/src/semmle/code/java/security/CommandArguments.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/java/ql/src/semmle/code/java/security/CommandArguments.qll b/java/ql/src/semmle/code/java/security/CommandArguments.qll index ebc30eb22df1..c0934e49411c 100644 --- a/java/ql/src/semmle/code/java/security/CommandArguments.qll +++ b/java/ql/src/semmle/code/java/security/CommandArguments.qll @@ -174,6 +174,7 @@ private Expr firstElementOf(Expr arr) { exists(CommandArgArrayImmutableFirst caa | arr = caa.getAUse() | result = caa.getFirstElement()) or exists(MethodAccess ma, Method m | + arr = ma and ma.getMethod() = m and m.getDeclaringType().hasQualifiedName("java.util", "Arrays") and m.hasName("copyOf") and