diff --git a/change-notes/1.21/analysis-java.md b/change-notes/1.21/analysis-java.md index fbe4fe029852..2b71148b3a47 100644 --- a/change-notes/1.21/analysis-java.md +++ b/change-notes/1.21/analysis-java.md @@ -22,6 +22,11 @@ methods. This means that more guards are recognized yielding precision improvements in a number of queries including `java/index-out-of-bounds`, `java/dereferenced-value-may-be-null`, and `java/useless-null-check`. +* The default sanitizer in taint tracking has been made more precise. The + sanitizer works by looking for guards that inspect tainted strings, and it + used to work at the level of individual variables. This has been changed to + use the `Guards` library, such that only guarded variable accesses are + sanitized. This may give additional results in the security queries. * Spring framework support is enhanced by taking into account additional annotations that indicate remote user input. This affects all security queries, which may yield additional results. diff --git a/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll b/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll index 3df7bee84014..72ceb91b16c9 100644 --- a/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll +++ b/java/ql/src/semmle/code/java/dataflow/TaintTracking.qll @@ -59,7 +59,7 @@ module TaintTracking { isSanitizer(node) or // Ignore paths through test code. node.getEnclosingCallable().getDeclaringType() instanceof NonSecurityTestClass or - exists(ValidatedVariable var | node.asExpr() = var.getAnAccess()) + node.asExpr() instanceof ValidatedVariableAccess } /** Holds if the edge from `node1` to `node2` is a taint sanitizer. */ @@ -131,7 +131,7 @@ module TaintTracking { isSanitizer(node) or // Ignore paths through test code. node.getEnclosingCallable().getDeclaringType() instanceof NonSecurityTestClass or - exists(ValidatedVariable var | node.asExpr() = var.getAnAccess()) + node.asExpr() instanceof ValidatedVariableAccess } /** Holds if the edge from `node1` to `node2` is a taint sanitizer. */ diff --git a/java/ql/src/semmle/code/java/security/ControlledString.qll b/java/ql/src/semmle/code/java/security/ControlledString.qll index f1f5fd34abfc..c9f036b0ea69 100644 --- a/java/ql/src/semmle/code/java/security/ControlledString.qll +++ b/java/ql/src/semmle/code/java/security/ControlledString.qll @@ -1,4 +1,4 @@ -/* +/** * Controlled strings are the opposite of tainted strings. * There is positive evidence that they are fully controlled by * the program source code. @@ -90,7 +90,7 @@ predicate controlledString(Expr expr) { boxedToString(method) ) or - exists(ValidatedVariable var | var.getAnAccess() = expr) + expr instanceof ValidatedVariableAccess or forex(Expr other | controlledStringLimitedProp(other, expr) | controlledString(other)) ) and diff --git a/java/ql/src/semmle/code/java/security/Validation.qll b/java/ql/src/semmle/code/java/security/Validation.qll index 893a956a43f2..9d215041e4cd 100644 --- a/java/ql/src/semmle/code/java/security/Validation.qll +++ b/java/ql/src/semmle/code/java/security/Validation.qll @@ -1,4 +1,6 @@ import semmle.code.java.Expr +import semmle.code.java.dataflow.SSA +import semmle.code.java.controlflow.Guards bindingset[result, i] private int unbindInt(int i) { i <= result and i >= result } @@ -25,8 +27,49 @@ predicate validationMethod(Method method, int arg) { ) } -/** A variable that is ever passed to a string verification method. */ -class ValidatedVariable extends Variable { +private predicate validationCall(MethodAccess ma, VarAccess va) { + exists(int arg | validationMethod(ma.getMethod(), arg) and ma.getArgument(arg) = va) +} + +private predicate validatedAccess(VarAccess va) { + exists(SsaVariable v, MethodAccess guardcall | + va = v.getAUse() and + validationCall(guardcall, v.getAUse()) + | + guardcall.(Guard).controls(va.getBasicBlock(), _) + or + exists(ControlFlowNode node | + guardcall.getMethod().getReturnType() instanceof VoidType and + guardcall.getControlFlowNode() = node + | + exists(BasicBlock succ | + succ = node.getANormalSuccessor() and + dominatingEdge(node.getBasicBlock(), succ) and + succ.bbDominates(va.getBasicBlock()) + ) + or + exists(BasicBlock bb, int i | + bb.getNode(i) = node and + bb.getNode(i + 1) = node.getANormalSuccessor() + | + bb.bbStrictlyDominates(va.getBasicBlock()) or + bb.getNode(any(int j | j > i)) = va + ) + ) + ) +} + +/** A variable access that is guarded by a string verification method. */ +class ValidatedVariableAccess extends VarAccess { + ValidatedVariableAccess() { validatedAccess(this) } +} + +/** + * DEPRECATED: Use ValidatedVariableAccess instead. + * + * A variable that is ever passed to a string verification method. + */ +deprecated class ValidatedVariable extends Variable { ValidatedVariable() { exists(MethodAccess call, int arg, VarAccess access | validationMethod(call.getMethod(), arg) and diff --git a/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.expected b/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.expected index f11e1825ef8b..b839d98d3876 100644 --- a/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.expected +++ b/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.expected @@ -1 +1 @@ -| Test.java:60:4:60:29 | Test:60 | Test.java:60:4:60:29 | String category | +| Test.java:64:8:64:15 | category | diff --git a/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.ql b/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.ql index 02be4f462aa6..0542b879033c 100644 --- a/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.ql +++ b/java/ql/test/query-tests/security/CWE-089/semmle/examples/ValidatedVariable.ql @@ -1,4 +1,4 @@ import semmle.code.java.security.Validation -from ValidatedVariable var -select var.getLocation(), var +from ValidatedVariableAccess va +select va diff --git a/java/ql/test/query-tests/security/CWE-089/semmle/examples/controlledString.expected b/java/ql/test/query-tests/security/CWE-089/semmle/examples/controlledString.expected index 7f75f3e527d7..b5e717e5734a 100644 --- a/java/ql/test/query-tests/security/CWE-089/semmle/examples/controlledString.expected +++ b/java/ql/test/query-tests/security/CWE-089/semmle/examples/controlledString.expected @@ -5,20 +5,16 @@ | checkIdentifier | 1 | Validation.java:7:16:7:16 | 0 | | checkIdentifier | 1 | Validation.java:7:19:7:19 | i | | checkIdentifier | 1 | Validation.java:7:19:7:33 | ... < ... | -| checkIdentifier | 1 | Validation.java:7:23:7:24 | id | | checkIdentifier | 1 | Validation.java:7:23:7:33 | length(...) | | checkIdentifier | 1 | Validation.java:7:36:7:36 | i | | checkIdentifier | 1 | Validation.java:7:36:7:38 | ...++ | | checkIdentifier | 2 | Validation.java:8:9:8:24 | c | -| checkIdentifier | 2 | Validation.java:8:13:8:14 | id | | checkIdentifier | 2 | Validation.java:8:13:8:24 | charAt(...) | | checkIdentifier | 2 | Validation.java:8:23:8:23 | i | | checkIdentifier | 3 | Validation.java:9:8:9:29 | !... | | checkIdentifier | 3 | Validation.java:9:9:9:29 | isLetter(...) | | checkIdentifier | 3 | Validation.java:9:28:9:28 | c | | checkIdentifier | 4 | Validation.java:10:32:10:53 | "Invalid identifier: " | -| checkIdentifier | 4 | Validation.java:10:32:10:58 | ... + ... | -| checkIdentifier | 4 | Validation.java:10:57:10:58 | id | | controlledStrings | 4 | Test.java:114:26:114:79 | "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" | | controlledStrings | 4 | Test.java:114:26:115:17 | ... + ... | | controlledStrings | 4 | Test.java:114:26:115:38 | ... + ... | @@ -78,7 +74,6 @@ | tainted | 22 | Test.java:51:19:51:72 | "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" | | tainted | 24 | Test.java:53:19:53:36 | "' ORDER BY PRICE" | | tainted | 31 | Test.java:60:27:60:27 | 1 | -| tainted | 32 | Test.java:61:31:61:38 | category | | tainted | 34 | Test.java:63:20:63:73 | "SELECT ITEM,PRICE FROM PRODUCT WHERE ITEM_CATEGORY='" | | tainted | 34 | Test.java:63:20:64:15 | ... + ... | | tainted | 34 | Test.java:63:20:64:36 | ... + ... | diff --git a/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.expected b/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.expected index 5fdcf230dbad..8cd4a3016375 100644 --- a/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.expected +++ b/java/ql/test/query-tests/security/CWE-089/semmle/examples/taintedString.expected @@ -18,6 +18,7 @@ | Test.java:29:22:29:28 | tainted | 27 | Test.java:56:47:56:61 | querySbToString | | Test.java:29:22:29:28 | tainted | 31 | Test.java:60:22:60:25 | args | | Test.java:29:22:29:28 | tainted | 31 | Test.java:60:22:60:28 | ...[...] | +| Test.java:29:22:29:28 | tainted | 32 | Test.java:61:31:61:38 | category | | Test.java:99:22:99:25 | good | 3 | Test.java:102:22:102:25 | args | | Test.java:99:22:99:25 | good | 3 | Test.java:102:22:102:28 | ...[...] | | Test.java:99:22:99:25 | good | 6 | Test.java:105:27:105:34 | category | @@ -30,3 +31,7 @@ | Test.java:190:21:190:24 | main | 1 | Test.java:191:11:191:14 | args | | Test.java:190:21:190:24 | main | 3 | Test.java:193:8:193:11 | args | | Test.java:190:21:190:24 | main | 5 | Test.java:195:14:195:17 | args | +| Validation.java:6:21:6:35 | checkIdentifier | 1 | Validation.java:7:23:7:24 | id | +| Validation.java:6:21:6:35 | checkIdentifier | 2 | Validation.java:8:13:8:14 | id | +| Validation.java:6:21:6:35 | checkIdentifier | 4 | Validation.java:10:32:10:58 | ... + ... | +| Validation.java:6:21:6:35 | checkIdentifier | 4 | Validation.java:10:57:10:58 | id |