Merge remote-tracking branch 'upstream/rc/1.20' into mergeback-20190408

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, _)
   }
 
   /**