Merge pull request #1344 from aschackmull/java/validatedvariable

Java: Replace ValidatedVariable with guarded accesses.
2026-04-30 11:15:13 +02:00 · 2019-05-24 11:23:18 -04:00
parent 15e2248440 1a9b1d5865
commit 87cbc7e199
8 changed files with 62 additions and 14 deletions
--- 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. */
--- 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
--- 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
--- 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 |
--- 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
--- 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 | ... + ... |
--- 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 |