Merge pull request #2763 from jbj/ir-VariableNode

C++: DefaultTaintTracking perf fix for globals

Author: Robert Marsh

cpp/ql/src/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -69,7 +69,7 @@ private class DefaultTaintTrackingCfg extends DataFlow::Configuration {
 
   override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
-  override predicate isSink(DataFlow::Node sink) { any() }
+  override predicate isSink(DataFlow::Node sink) { exists(adjustedSink(sink)) }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     instructionTaintStep(n1.asInstruction(), n2.asInstruction())
@@ -84,18 +84,15 @@ private class ToGlobalVarTaintTrackingCfg extends DataFlow::Configuration {
   override predicate isSource(DataFlow::Node source) { source = getNodeForSource(_) }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(GlobalOrNamespaceVariable gv | writesVariable(sink.asInstruction(), gv))
+    sink.asVariable() instanceof GlobalOrNamespaceVariable
   }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     instructionTaintStep(n1.asInstruction(), n2.asInstruction())
     or
-    exists(StoreInstruction i1, LoadInstruction i2, GlobalOrNamespaceVariable gv |
-      writesVariable(i1, gv) and
-      readsVariable(i2, gv) and
-      i1 = n1.asInstruction() and
-      i2 = n2.asInstruction()
-    )
+    writesVariable(n1.asInstruction(), n2.asVariable().(GlobalOrNamespaceVariable))
+    or
+    readsVariable(n2.asInstruction(), n1.asVariable().(GlobalOrNamespaceVariable))
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
@@ -105,19 +102,20 @@ private class FromGlobalVarTaintTrackingCfg extends DataFlow2::Configuration {
   FromGlobalVarTaintTrackingCfg() { this = "FromGlobalVarTaintTrackingCfg" }
 
   override predicate isSource(DataFlow::Node source) {
-    exists(
-      ToGlobalVarTaintTrackingCfg other, DataFlow::Node prevSink, GlobalOrNamespaceVariable gv
-    |
-      other.hasFlowTo(prevSink) and
-      writesVariable(prevSink.asInstruction(), gv) and
-      readsVariable(source.asInstruction(), gv)
-    )
+    // This set of sources should be reasonably small, which is good for
+    // performance since the set of sinks is very large.
+    exists(ToGlobalVarTaintTrackingCfg otherCfg | otherCfg.hasFlowTo(source))
   }
 
-  override predicate isSink(DataFlow::Node sink) { any() }
+  override predicate isSink(DataFlow::Node sink) { exists(adjustedSink(sink)) }
 
   override predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     instructionTaintStep(n1.asInstruction(), n2.asInstruction())
+    or
+    // Additional step for flow out of variables. There is no flow _into_
+    // variables in this configuration, so this step only serves to take flow
+    // out of a variable that's a source.
+    readsVariable(n2.asInstruction(), n1.asVariable())
   }
 
   override predicate isBarrier(DataFlow::Node node) { nodeIsBarrier(node) }
@@ -351,23 +349,12 @@ predicate taintedIncludingGlobalVars(Expr source, Element tainted, string global
   globalVar = ""
   or
   exists(
-    ToGlobalVarTaintTrackingCfg toCfg, FromGlobalVarTaintTrackingCfg fromCfg, DataFlow::Node store,
-    GlobalOrNamespaceVariable global, DataFlow::Node load, DataFlow::Node sink
+    ToGlobalVarTaintTrackingCfg toCfg, FromGlobalVarTaintTrackingCfg fromCfg,
+    DataFlow::VariableNode variableNode, GlobalOrNamespaceVariable global, DataFlow::Node sink
   |
-    toCfg.hasFlow(getNodeForSource(source), store) and
-    store
-        .asInstruction()
-        .(StoreInstruction)
-        .getDestinationAddress()
-        .(VariableAddressInstruction)
-        .getASTVariable() = global and
-    load
-        .asInstruction()
-        .(LoadInstruction)
-        .getSourceAddress()
-        .(VariableAddressInstruction)
-        .getASTVariable() = global and
-    fromCfg.hasFlow(load, sink) and
+    global = variableNode.getVariable() and
+    toCfg.hasFlow(getNodeForSource(source), variableNode) and
+    fromCfg.hasFlow(variableNode, sink) and
     tainted = adjustedSink(sink) and
     global = globalVarFromId(globalVar)
   )

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowPrivate.qll

@@ -7,17 +7,17 @@ private import DataFlowDispatch
  * A data flow node that occurs as the argument of a call and is passed as-is
  * to the callable. Instance arguments (`this` pointer) are also included.
  */
-class ArgumentNode extends Node {
-  ArgumentNode() { exists(CallInstruction call | this.asInstruction() = call.getAnArgument()) }
+class ArgumentNode extends InstructionNode {
+  ArgumentNode() { exists(CallInstruction call | this.getInstruction() = call.getAnArgument()) }
 
   /**
    * Holds if this argument occurs at the given position in the given call.
    * The instance argument is considered to have index `-1`.
    */
   predicate argumentOf(DataFlowCall call, int pos) {
-    this.asInstruction() = call.getPositionalArgument(pos)
+    this.getInstruction() = call.getPositionalArgument(pos)
     or
-    this.asInstruction() = call.getThisArgument() and pos = -1
+    this.getInstruction() = call.getThisArgument() and pos = -1
   }
 
   /** Gets the call in which this node is an argument. */
@@ -36,15 +36,15 @@ class ReturnKind extends TReturnKind {
 }
 
 /** A data flow node that occurs as the result of a `ReturnStmt`. */
-class ReturnNode extends Node {
-  ReturnNode() { exists(ReturnValueInstruction ret | this.asInstruction() = ret.getReturnValue()) }
+class ReturnNode extends InstructionNode {
+  ReturnNode() { exists(ReturnValueInstruction ret | this.getInstruction() = ret.getReturnValue()) }
 
   /** Gets the kind of this returned value. */
   ReturnKind getKind() { result = TNormalReturnKind() }
 }
 
 /** A data flow node that represents the output of a call. */
-class OutNode extends Node {
+class OutNode extends InstructionNode {
   override CallInstruction instr;
 
   /** Gets the underlying call. */
@@ -181,11 +181,17 @@ private predicate suppressUnusedType(Type t) { any() }
 // Java QL library compatibility wrappers
 //////////////////////////////////////////////////////////////////////////////
 /** A node that performs a type cast. */
-class CastNode extends Node {
+class CastNode extends InstructionNode {
   CastNode() { none() } // stub implementation
 }
 
-class DataFlowCallable = Function;
+/**
+ * A function that may contain code or a variable that may contain itself. When
+ * flow crosses from one _enclosing callable_ to another, the interprocedural
+ * data-flow library discards call contexts and inserts a node in the big-step
+ * relation used for human-readable path explanations.
+ */
+class DataFlowCallable = Declaration;
 
 class DataFlowExpr = Expr;
 

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -8,12 +8,9 @@ private import semmle.code.cpp.controlflow.IRGuards
 private import semmle.code.cpp.ir.ValueNumbering
 private import semmle.code.cpp.models.interfaces.DataFlow
 
-/**
- * A newtype wrapper to prevent accidental casts between `Node` and
- * `Instruction`. This ensures we can add `Node`s that are not `Instruction`s
- * in the future.
- */
-private newtype TIRDataFlowNode = MkIRDataFlowNode(Instruction i)
+private newtype TIRDataFlowNode =
+  TInstructionNode(Instruction i) or
+  TVariableNode(Variable var)
 
 /**
  * A node in a data flow graph.
@@ -23,44 +20,45 @@ private newtype TIRDataFlowNode = MkIRDataFlowNode(Instruction i)
  * `DataFlow::parameterNode`, and `DataFlow::uninitializedNode` respectively.
  */
 class Node extends TIRDataFlowNode {
-  Instruction instr;
-
-  Node() { this = MkIRDataFlowNode(instr) }
-
   /**
-   * INTERNAL: Do not use. Alternative name for `getFunction`.
+   * INTERNAL: Do not use.
    */
-  Function getEnclosingCallable() { result = this.getFunction() }
+  Declaration getEnclosingCallable() { none() } // overridden in subclasses
 
-  Function getFunction() { result = instr.getEnclosingFunction() }
+  /** Gets the function to which this node belongs, if any. */
+  Function getFunction() { none() } // overridden in subclasses
 
   /** Gets the type of this node. */
-  Type getType() { result = instr.getResultType() }
+  Type getType() { none() } // overridden in subclasses
 
-  Instruction asInstruction() { this = MkIRDataFlowNode(result) }
+  /** Gets the instruction corresponding to this node, if any. */
+  Instruction asInstruction() { result = this.(InstructionNode).getInstruction() }
 
   /**
    * Gets the non-conversion expression corresponding to this node, if any. If
    * this node strictly (in the sense of `asConvertedExpr`) corresponds to a
    * `Conversion`, then the result is that `Conversion`'s non-`Conversion` base
    * expression.
    */
-  Expr asExpr() {
-    result.getConversion*() = instr.getConvertedResultExpression() and
-    not result instanceof Conversion
-  }
+  Expr asExpr() { result = this.(ExprNode).getExpr() }
 
   /**
    * Gets the expression corresponding to this node, if any. The returned
    * expression may be a `Conversion`.
    */
-  Expr asConvertedExpr() { result = instr.getConvertedResultExpression() }
+  Expr asConvertedExpr() { result = this.(ExprNode).getConvertedExpr() }
 
   /** Gets the argument that defines this `DefinitionByReferenceNode`, if any. */
   Expr asDefiningArgument() { result = this.(DefinitionByReferenceNode).getArgument() }
 
   /** Gets the parameter corresponding to this node, if any. */
-  Parameter asParameter() { result = instr.(InitializeParameterInstruction).getParameter() }
+  Parameter asParameter() { result = this.(ParameterNode).getParameter() }
+
+  /**
+   * Gets the variable corresponding to this node, if any. This can be used for
+   * modelling flow in and out of global variables.
+   */
+  Variable asVariable() { result = this.(VariableNode).getVariable() }
 
   /**
    * DEPRECATED: See UninitializedNode.
@@ -76,7 +74,7 @@ class Node extends TIRDataFlowNode {
   Type getTypeBound() { result = getType() }
 
   /** Gets the location of this element. */
-  Location getLocation() { result = instr.getLocation() }
+  Location getLocation() { none() } // overridden by subclasses
 
   /**
    * Holds if this element is at the specified location.
@@ -91,32 +89,55 @@ class Node extends TIRDataFlowNode {
     this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
 
-  string toString() {
+  /** Gets a textual representation of this element. */
+  string toString() { none() } // overridden by subclasses
+}
+
+class InstructionNode extends Node, TInstructionNode {
+  Instruction instr;
+
+  InstructionNode() { this = TInstructionNode(instr) }
+
+  /** Gets the instruction corresponding to this node. */
+  Instruction getInstruction() { result = instr }
+
+  override Declaration getEnclosingCallable() { result = this.getFunction() }
+
+  override Function getFunction() { result = instr.getEnclosingFunction() }
+
+  override Type getType() { result = instr.getResultType() }
+
+  override Location getLocation() { result = instr.getLocation() }
+
+  override string toString() {
     // This predicate is overridden in subclasses. This default implementation
     // does not use `Instruction.toString` because that's expensive to compute.
-    result = this.asInstruction().getOpcode().toString()
+    result = this.getInstruction().getOpcode().toString()
   }
 }
 
 /**
  * An expression, viewed as a node in a data flow graph.
  */
-class ExprNode extends Node {
-  ExprNode() { exists(this.asExpr()) }
+class ExprNode extends InstructionNode {
+  ExprNode() { exists(instr.getConvertedResultExpression()) }
 
   /**
    * Gets the non-conversion expression corresponding to this node, if any. If
    * this node strictly (in the sense of `getConvertedExpr`) corresponds to a
    * `Conversion`, then the result is that `Conversion`'s non-`Conversion` base
    * expression.
    */
-  Expr getExpr() { result = this.asExpr() }
+  Expr getExpr() {
+    result.getConversion*() = instr.getConvertedResultExpression() and
+    not result instanceof Conversion
+  }
 
   /**
    * Gets the expression corresponding to this node, if any. The returned
    * expression may be a `Conversion`.
    */
-  Expr getConvertedExpr() { result = this.asConvertedExpr() }
+  Expr getConvertedExpr() { result = instr.getConvertedResultExpression() }
 
   override string toString() { result = this.asConvertedExpr().toString() }
 }
@@ -125,7 +146,7 @@ class ExprNode extends Node {
  * The value of a parameter at function entry, viewed as a node in a data
  * flow graph.
  */
-class ParameterNode extends Node {
+class ParameterNode extends InstructionNode {
   override InitializeParameterInstruction instr;
 
   /**
@@ -139,7 +160,7 @@ class ParameterNode extends Node {
   override string toString() { result = instr.getParameter().toString() }
 }
 
-private class ThisParameterNode extends Node {
+private class ThisParameterNode extends InstructionNode {
   override InitializeThisInstruction instr;
 
   override string toString() { result = "this" }
@@ -176,7 +197,7 @@ deprecated class UninitializedNode extends Node {
  * This class exists to match the interface used by Java. There are currently no non-abstract
  * classes that extend it. When we implement field flow, we can revisit this.
  */
-abstract class PostUpdateNode extends Node {
+abstract class PostUpdateNode extends InstructionNode {
   /**
    * Gets the node before the state update.
    */
@@ -193,7 +214,7 @@ abstract class PostUpdateNode extends Node {
  * returned. This node will have its `getArgument()` equal to `&x` and its
  * `getVariableAccess()` equal to `x`.
  */
-class DefinitionByReferenceNode extends Node {
+class DefinitionByReferenceNode extends InstructionNode {
   override WriteSideEffectInstruction instr;
 
   /** Gets the argument corresponding to this node. */
@@ -220,10 +241,41 @@ class DefinitionByReferenceNode extends Node {
   }
 }
 
+/**
+ * A `Node` corresponding to a variable in the program, as opposed to the
+ * value of that variable at some particular point. This can be used for
+ * modelling flow in and out of global variables.
+ */
+class VariableNode extends Node, TVariableNode {
+  Variable v;
+
+  VariableNode() { this = TVariableNode(v) }
+
+  /** Gets the variable corresponding to this node. */
+  Variable getVariable() { result = v }
+
+  override Function getFunction() { none() }
+
+  override Declaration getEnclosingCallable() {
+    // When flow crosses from one _enclosing callable_ to another, the
+    // interprocedural data-flow library discards call contexts and inserts a
+    // node in the big-step relation used for human-readable path explanations.
+    // Therefore we want a distinct enclosing callable for each `VariableNode`,
+    // and that can be the `Variable` itself.
+    result = v
+  }
+
+  override Type getType() { result = v.getType() }
+
+  override Location getLocation() { result = v.getLocation() }
+
+  override string toString() { result = v.toString() }
+}
+
 /**
  * Gets the node corresponding to `instr`.
  */
-Node instructionNode(Instruction instr) { result.asInstruction() = instr }
+InstructionNode instructionNode(Instruction instr) { result.getInstruction() = instr }
 
 DefinitionByReferenceNode definitionByReferenceNode(Expr e) { result.getArgument() = e }
 
@@ -244,6 +296,9 @@ ExprNode convertedExprNode(Expr e) { result.getExpr() = e }
  */
 ParameterNode parameterNode(Parameter p) { result.getParameter() = p }
 
+/** Gets the `VariableNode` corresponding to the variable `v`. */
+VariableNode variableNode(Variable v) { result.getVariable() = v }
+
 /**
  * Gets the `Node` corresponding to the value of an uninitialized local
  * variable `v`.

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

@@ -0,0 +1,4 @@
+| globals.cpp:13:15:13:20 | call to getenv | globals.cpp:2:17:2:25 | sinkParam | global1 |
+| globals.cpp:13:15:13:20 | call to getenv | globals.cpp:12:10:12:16 | global1 | global1 |
+| globals.cpp:23:15:23:20 | call to getenv | globals.cpp:2:17:2:25 | sinkParam | global2 |
+| globals.cpp:23:15:23:20 | call to getenv | globals.cpp:19:10:19:16 | global2 | global2 |