C++: Let data flow past definition by reference

cpp/ql/src/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.ql

@@ -14,6 +14,7 @@
  */
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.dataflow.DataFlow2
 
 /**
  * A function call to SetSecurityDescriptorDacl to set the ACL, specified by (2nd argument) bDaclPresent = TRUE
@@ -28,9 +29,9 @@ class SetSecurityDescriptorDaclFunctionCall extends FunctionCall {
 /**
  * Dataflow that detects a call to SetSecurityDescriptorDacl with a NULL DACL as the pDacl argument
  */
-class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configuration {
-  SetSecurityDescriptorDaclFunctionConfiguration() {
-    this = "SetSecurityDescriptorDaclFunctionConfiguration"
+class NullDaclConfig extends DataFlow::Configuration {
+  NullDaclConfig() {
+    this = "NullDaclConfig"
   }
 
   override predicate isSource(DataFlow::Node source) {
@@ -49,6 +50,43 @@ class SetSecurityDescriptorDaclFunctionConfiguration extends DataFlow::Configura
   }
 }
 
+/**
+ * Dataflow that detects a call to SetSecurityDescriptorDacl with a pDacl
+ * argument that's _not_ likely to be NULL.
+ */
+class NonNullDaclConfig extends DataFlow2::Configuration {
+  NonNullDaclConfig() {
+    this = "NonNullDaclConfig"
+  }
+
+  override predicate isSource(DataFlow::Node source) {
+    source.getType().getUnspecifiedType().(PointerType).getBaseType() =
+      any(Type t | t.getName() = "ACL").getUnspecifiedType() and
+    (
+      // If the value comes from a function whose body we can't see, assume
+      // it's not null.
+      exists(Call call |
+        not exists(call.getTarget().getBlock()) and
+        source.asExpr() = call
+      )
+      or
+      // If the value is assigned by reference, assume it's not null. The data
+      // flow library cannot currently follow flow from the body of a function to
+      // an assignment by reference, so this rule applies whether we see the
+      // body or not.
+      exists(Call call |
+        call.getAnArgument() = source.asDefiningArgument()
+      )
+    )
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(SetSecurityDescriptorDaclFunctionCall call |
+      sink.asExpr() = call.getArgument(2)
+    )
+  }
+}
+
 from SetSecurityDescriptorDaclFunctionCall call, string message
 where exists
   ( 
@@ -59,9 +97,10 @@ where exists
   ) or exists
   ( 
     Expr constassign, VariableAccess var, 
-    SetSecurityDescriptorDaclFunctionConfiguration config |
+    NullDaclConfig nullDaclConfig, NonNullDaclConfig nonNullDaclConfig |
     message = "Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable " + var + " that is set to NULL will result in an unprotected object." |
     var = call.getArgument(2)
-    and config.hasFlow(DataFlow::exprNode(constassign), DataFlow::exprNode(var))
+    and nullDaclConfig.hasFlow(DataFlow::exprNode(constassign), DataFlow::exprNode(var))
+    and not nonNullDaclConfig.hasFlow(_, DataFlow::exprNode(var))
   )
 select call, message

cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll

@@ -93,9 +93,17 @@ module FlowVar_internal {
    * - Supporting fields, globals and statics like the Java SSA library does.
    * - Supporting all local variables, even if their address is taken by
    *   address-of, reference assignments, or lambdas.
+   * - Understanding that assignment to a field of a local struct is a
+   *   definition of the struct but not a complete overwrite. This is what the
+   *   IR library uses chi nodes for.
    */
   predicate fullySupportedSsaVariable(Variable v) {
     v = any(SsaDefinition def).getAVariable() and
+    // After `foo(&x.field)` we need for there to be two definitions of `x`:
+    // the one that existed before the call to `foo` and the def-by-ref from
+    // the call. It's fundamental in SSA that each use is only associated with
+    // one def, so we can't easily get this effect with SSA.
+    not definitionByReference(v.getAnAccess(), _) and
     // SSA variables do not exist before their first assignment, but one
     // feature of this data flow library is to track where uninitialized data
     // ends up.
@@ -183,8 +191,7 @@ module FlowVar_internal {
     }
 
     override predicate definedByReference(Expr arg) {
-      definitionByReference(v.getAnAccess(), arg) and
-      arg = def.getDefinition()
+      none() // Not supported for SSA. See `fullySupportedSsaVariable`.
     }
 
     override predicate definedByInitialValue(LocalScopeVariable param) {
@@ -204,8 +211,6 @@ module FlowVar_internal {
       this.definedByExpr(_, _)
       or
       this.definedByInitialValue(_)
-      or
-      this.definedByReference(_)
     }
 
     /**
@@ -236,10 +241,7 @@ module FlowVar_internal {
     BlockVar() { this = TBlockVar(sbb, v) }
 
     override VariableAccess getAnAccess() {
-      exists(SubBasicBlock reached |
-        reached = getAReachedBlockVarSBB(this) and
-        variableAccessInSBB(v, reached, result)
-      )
+      variableAccessInSBB(v, getAReachedBlockVarSBB(this), result)
     }
 
     override predicate definedByInitialValue(LocalScopeVariable lsv) {
@@ -401,8 +403,7 @@ module FlowVar_internal {
       mid = getAReachedBlockVarSBB(start) and
       result = mid.getASuccessor() and
       not skipLoop(mid, result, sbbDef, v) and
-      not assignmentLikeOperation(result, v, _) and
-      not blockVarDefinedByReference(result, v, _)
+      not assignmentLikeOperation(result, v, _)
     )
   }
 
@@ -413,12 +414,6 @@ module FlowVar_internal {
     va.getTarget() = v and
     va = sbb.getANode() and
     not overwrite(va, _)
-    or
-    // Allow flow into a `VariableAccess` that is used as definition by
-    // reference. This flow is blocked by `getAReachedBlockVarSBB` because
-    // flow should not propagate past that.
-    va = sbb.getASuccessor().(VariableAccess) and
-    blockVarDefinedByReference(va, v, _)
   }
 
   /**
@@ -516,9 +511,6 @@ module FlowVar_internal {
    */
   predicate overwrite(VariableAccess va, ControlFlowNode node) {
     va = node.(AssignExpr).getLValue()
-    or
-    va = node and
-    definitionByReference(node, _)
   }
 
   /**

cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow.expected

@@ -1,11 +1,16 @@
 | example.c:15:37:15:37 | b | example.c:19:6:19:6 | b |
 | example.c:15:44:15:46 | pos | example.c:24:24:24:26 | pos |
 | example.c:15:44:15:46 | pos | example.c:28:23:28:25 | pos |
+| example.c:17:19:17:22 | {...} | example.c:24:2:24:7 | coords |
 | example.c:17:19:17:22 | {...} | example.c:24:13:24:18 | coords |
+| example.c:17:19:17:22 | {...} | example.c:26:2:26:7 | coords |
+| example.c:17:19:17:22 | {...} | example.c:26:19:26:24 | coords |
 | example.c:24:13:24:30 | ... = ... | example.c:24:2:24:30 | ... = ... |
 | example.c:24:24:24:30 | ... + ... | example.c:24:13:24:30 | ... = ... |
 | example.c:26:13:26:16 | call to getX | example.c:26:2:26:25 | ... = ... |
 | example.c:26:18:26:24 | ref arg & ... | example.c:26:2:26:7 | coords |
+| example.c:26:18:26:24 | ref arg & ... | example.c:26:19:26:24 | coords |
+| example.c:28:22:28:25 | ref arg & ... | example.c:28:23:28:25 | pos |
 | test.cpp:6:12:6:17 | call to source | test.cpp:7:8:7:9 | t1 |
 | test.cpp:6:12:6:17 | call to source | test.cpp:8:8:8:9 | t1 |
 | test.cpp:6:12:6:17 | call to source | test.cpp:9:8:9:9 | t1 |
@@ -31,14 +36,22 @@
 | test.cpp:24:10:24:11 | t2 | test.cpp:26:8:26:9 | t1 |
 | test.cpp:430:48:430:54 | source1 | test.cpp:432:17:432:23 | source1 |
 | test.cpp:431:12:431:13 | 0 | test.cpp:432:11:432:13 | tmp |
+| test.cpp:431:12:431:13 | 0 | test.cpp:432:33:432:35 | tmp |
+| test.cpp:431:12:431:13 | 0 | test.cpp:433:8:433:10 | tmp |
 | test.cpp:432:10:432:13 | & ... | test.cpp:432:3:432:8 | call to memcpy |
+| test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:11:432:13 | tmp |
+| test.cpp:432:10:432:13 | ref arg & ... | test.cpp:432:33:432:35 | tmp |
 | test.cpp:432:10:432:13 | ref arg & ... | test.cpp:433:8:433:10 | tmp |
 | test.cpp:432:17:432:23 | source1 | test.cpp:432:10:432:13 | ref arg & ... |
 | test.cpp:436:53:436:59 | source1 | test.cpp:439:17:439:23 | source1 |
 | test.cpp:436:66:436:66 | b | test.cpp:441:7:441:7 | b |
 | test.cpp:437:12:437:13 | 0 | test.cpp:438:19:438:21 | tmp |
 | test.cpp:437:12:437:13 | 0 | test.cpp:439:11:439:13 | tmp |
+| test.cpp:437:12:437:13 | 0 | test.cpp:439:33:439:35 | tmp |
+| test.cpp:437:12:437:13 | 0 | test.cpp:440:8:440:10 | tmp |
+| test.cpp:437:12:437:13 | 0 | test.cpp:442:10:442:12 | tmp |
 | test.cpp:439:10:439:13 | & ... | test.cpp:439:3:439:8 | call to memcpy |
+| test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:11:439:13 | tmp |
 | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:439:33:439:35 | tmp |
 | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:440:8:440:10 | tmp |
 | test.cpp:439:10:439:13 | ref arg & ... | test.cpp:442:10:442:12 | tmp |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp

@@ -443,17 +443,17 @@ void flowThroughMemcpy_blockvar_with_local_flow(int source1, int b) {
   }
 }
 
-void cleanedByMemcpy_ssa(int clean1) {
+void cleanedByMemcpy_ssa(int clean1) { // currently modeled with BlockVar, not SSA
   int tmp;
   memcpy(&tmp, &clean1, sizeof tmp);
-  sink(tmp); // clean
+  sink(tmp); // clean [FALSE POSITIVE]
 }
 
 void cleanedByMemcpy_blockvar(int clean1) {
   int tmp;
   int *capture = &tmp;
   memcpy(&tmp, &clean1, sizeof tmp);
-  sink(tmp); // clean
+  sink(tmp); // clean [FALSE POSITIVE]
 }
 
 void intRefSource(int &ref_source);

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.expected

@@ -30,10 +30,17 @@
 | test.cpp:433:8:433:10 | tmp | test.cpp:430:48:430:54 | source1 |
 | test.cpp:440:8:440:10 | tmp | test.cpp:436:53:436:59 | source1 |
 | test.cpp:442:10:442:12 | tmp | test.cpp:436:53:436:59 | source1 |
+| test.cpp:449:8:449:10 | tmp | test.cpp:447:7:447:9 | tmp |
+| test.cpp:456:8:456:10 | tmp | test.cpp:453:7:453:9 | tmp |
+| test.cpp:466:8:466:12 | local | test.cpp:464:7:464:11 | local |
 | test.cpp:466:8:466:12 | local | test.cpp:465:16:465:20 | ref arg local |
+| test.cpp:472:8:472:12 | local | test.cpp:470:7:470:11 | local |
 | test.cpp:472:8:472:12 | local | test.cpp:471:20:471:25 | ref arg & ... |
+| test.cpp:478:8:478:12 | local | test.cpp:476:7:476:11 | local |
 | test.cpp:478:8:478:12 | local | test.cpp:477:20:477:24 | ref arg local |
+| test.cpp:485:8:485:12 | local | test.cpp:483:7:483:11 | local |
 | test.cpp:485:8:485:12 | local | test.cpp:484:18:484:23 | ref arg & ... |
+| test.cpp:491:8:491:12 | local | test.cpp:489:7:489:11 | local |
 | test.cpp:491:8:491:12 | local | test.cpp:490:18:490:22 | ref arg local |
 | true_upon_entry.cpp:21:8:21:8 | x | true_upon_entry.cpp:17:11:17:16 | call to source |
 | true_upon_entry.cpp:29:8:29:8 | x | true_upon_entry.cpp:27:9:27:14 | call to source |

cpp/ql/test/library-tests/dataflow/dataflow-tests/test_diff.expected

@@ -11,10 +11,17 @@
 | test.cpp:430:48:430:54 | test.cpp:433:8:433:10 | AST only |
 | test.cpp:436:53:436:59 | test.cpp:440:8:440:10 | AST only |
 | test.cpp:436:53:436:59 | test.cpp:442:10:442:12 | AST only |
+| test.cpp:447:7:447:9 | test.cpp:449:8:449:10 | AST only |
+| test.cpp:453:7:453:9 | test.cpp:456:8:456:10 | AST only |
+| test.cpp:464:7:464:11 | test.cpp:466:8:466:12 | AST only |
 | test.cpp:465:16:465:20 | test.cpp:466:8:466:12 | AST only |
+| test.cpp:470:7:470:11 | test.cpp:472:8:472:12 | AST only |
 | test.cpp:471:20:471:25 | test.cpp:472:8:472:12 | AST only |
+| test.cpp:476:7:476:11 | test.cpp:478:8:478:12 | AST only |
 | test.cpp:477:20:477:24 | test.cpp:478:8:478:12 | AST only |
+| test.cpp:483:7:483:11 | test.cpp:485:8:485:12 | AST only |
 | test.cpp:484:18:484:23 | test.cpp:485:8:485:12 | AST only |
+| test.cpp:489:7:489:11 | test.cpp:491:8:491:12 | AST only |
 | test.cpp:490:18:490:22 | test.cpp:491:8:491:12 | AST only |
 | true_upon_entry.cpp:9:11:9:16 | true_upon_entry.cpp:13:8:13:8 | IR only |
 | true_upon_entry.cpp:62:11:62:16 | true_upon_entry.cpp:66:8:66:8 | IR only |

cpp/ql/test/library-tests/dataflow/dataflow-tests/uninitialized.expected

@@ -1,4 +1,20 @@
 | test.cpp:75:7:75:8 | u1 | test.cpp:76:8:76:9 | u1 |
 | test.cpp:83:7:83:8 | u2 | test.cpp:84:13:84:14 | u2 |
 | test.cpp:83:7:83:8 | u2 | test.cpp:85:8:85:9 | u2 |
+| test.cpp:447:7:447:9 | tmp | test.cpp:448:11:448:13 | tmp |
+| test.cpp:447:7:447:9 | tmp | test.cpp:449:8:449:10 | tmp |
 | test.cpp:453:7:453:9 | tmp | test.cpp:454:19:454:21 | tmp |
+| test.cpp:453:7:453:9 | tmp | test.cpp:455:11:455:13 | tmp |
+| test.cpp:453:7:453:9 | tmp | test.cpp:456:8:456:10 | tmp |
+| test.cpp:464:7:464:11 | local | test.cpp:465:16:465:20 | local |
+| test.cpp:464:7:464:11 | local | test.cpp:466:8:466:12 | local |
+| test.cpp:470:7:470:11 | local | test.cpp:471:21:471:25 | local |
+| test.cpp:470:7:470:11 | local | test.cpp:472:8:472:12 | local |
+| test.cpp:476:7:476:11 | local | test.cpp:477:20:477:24 | local |
+| test.cpp:476:7:476:11 | local | test.cpp:478:8:478:12 | local |
+| test.cpp:476:7:476:11 | local | test.cpp:479:9:479:13 | local |
+| test.cpp:483:7:483:11 | local | test.cpp:484:19:484:23 | local |
+| test.cpp:483:7:483:11 | local | test.cpp:485:8:485:12 | local |
+| test.cpp:489:7:489:11 | local | test.cpp:490:18:490:22 | local |
+| test.cpp:489:7:489:11 | local | test.cpp:491:8:491:12 | local |
+| test.cpp:489:7:489:11 | local | test.cpp:492:9:492:13 | local |

cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

@@ -128,13 +128,24 @@
 | taint.cpp:164:19:164:24 | call to source | taint.cpp:168:8:168:14 | tainted |  |
 | taint.cpp:164:19:164:24 | call to source | taint.cpp:172:18:172:24 | tainted |  |
 | taint.cpp:165:22:165:25 | {...} | taint.cpp:170:10:170:15 | buffer |  |
+| taint.cpp:165:22:165:25 | {...} | taint.cpp:171:8:171:13 | buffer |  |
+| taint.cpp:165:22:165:25 | {...} | taint.cpp:172:10:172:15 | buffer |  |
+| taint.cpp:165:22:165:25 | {...} | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:165:24:165:24 | 0 | taint.cpp:165:22:165:25 | {...} | TAINT |
 | taint.cpp:170:10:170:15 | buffer | taint.cpp:170:3:170:8 | call to strcpy |  |
+| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:170:10:170:15 | buffer |  |
 | taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:171:8:171:13 | buffer |  |
+| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer |  |
+| taint.cpp:170:10:170:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer |  |
+| taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:171:8:171:13 | buffer |  |
 | taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:172:10:172:15 | buffer |  |
+| taint.cpp:171:8:171:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:172:10:172:15 | buffer | taint.cpp:172:3:172:8 | call to strcat |  |
+| taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:172:10:172:15 | buffer |  |
 | taint.cpp:172:10:172:15 | ref arg buffer | taint.cpp:173:8:173:13 | buffer |  |
+| taint.cpp:173:8:173:13 | ref arg buffer | taint.cpp:173:8:173:13 | buffer |  |
 | taint.cpp:180:19:180:19 | p | taint.cpp:181:9:181:9 | p |  |
 | taint.cpp:181:9:181:9 | p | taint.cpp:181:8:181:9 | * ... | TAINT |
 | taint.cpp:185:11:185:16 | call to source | taint.cpp:186:11:186:11 | x |  |
+| taint.cpp:186:10:186:11 | ref arg & ... | taint.cpp:186:11:186:11 | x |  |
 | taint.cpp:186:11:186:11 | x | taint.cpp:186:10:186:11 | & ... | TAINT |

cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.cpp

@@ -89,4 +89,44 @@ void Test()
         NULL,       // DACL is going to be removed from security descriptor. Default/inherited access ==> should not be flagged
         FALSE);
 
-}
+}
+
+PACL returnUnknownAcl();
+
+PACL returnNull() {
+  return NULL;
+}
+
+PACL returnMaybeAcl(bool b) {
+  PACL pDacl = NULL;
+  if (b) {
+    SetEntriesInAcl(0, NULL, NULL, &pDacl);
+  }
+  return pDacl;
+}
+
+void Test2()
+{
+    PSECURITY_DESCRIPTOR pSecurityDescriptor;
+
+    PACL pDacl1 = returnUnknownAcl();
+    SetSecurityDescriptorDacl(
+        pSecurityDescriptor,
+        TRUE,       // Dacl Present
+        pDacl1,     // Give `returnUnknownAcl` the benefit of the doubt ==> should not be flagged
+        FALSE);
+
+    PACL pDacl2 = returnNull();
+    SetSecurityDescriptorDacl(
+        pSecurityDescriptor,
+        TRUE,       // Dacl Present
+        pDacl2,     // NULL pointer to DACL  == BUG
+        FALSE);
+
+    PACL pDacl3 = returnMaybeAcl(true);
+    SetSecurityDescriptorDacl(
+        pSecurityDescriptor,
+        TRUE,       // Dacl Present
+        pDacl3,     // should not be flagged
+        FALSE);
+}

cpp/ql/test/query-tests/Security/CWE/CWE-732/UnsafeDaclSecurityDescriptor.expected

@@ -1,2 +1,3 @@
 | UnsafeDaclSecurityDescriptor.cpp:70:9:70:33 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR will result in an unprotected object. |
 | UnsafeDaclSecurityDescriptor.cpp:76:9:76:33 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable pDacl that is set to NULL will result in an unprotected object. |
+| UnsafeDaclSecurityDescriptor.cpp:120:5:120:29 | call to SetSecurityDescriptorDacl | Setting a DACL to NULL in a SECURITY_DESCRIPTOR using variable pDacl2 that is set to NULL will result in an unprotected object. |