Merge branch 'master' into strftime

Author: geoffw0

change-notes/1.25/analysis-javascript.md

@@ -36,8 +36,9 @@
 
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Client-side cross-site scripting (`js/xss`) | Fewer results | This query no longer flags optionally sanitized values. |
 | Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Fewer results | This query now recognizes additional safe patterns of doing URL redirects. |
-| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe strings based on URLs. |
+| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe patterns of constructing HTML. |
 | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
 | Expression has no effect (`js/useless-expression`) | Fewer results | This query no longer flags an expression when that expression is the only content of the containing file. |
 | Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |

cpp/ql/src/semmle/code/cpp/controlflow/BasicBlocks.qll

@@ -1,3 +1,8 @@
+/**
+ * Provides a library for reasoning about control flow at the granularity of basic blocks.
+ * This is usually much more efficient than reasoning directly at the level of `ControlFlowNode`s.
+ */
+
 import cpp
 private import internal.PrimitiveBasicBlocks
 private import internal.ConstantExprs
@@ -148,22 +153,37 @@ predicate bb_successor = bb_successor_cached/2;
 class BasicBlock extends ControlFlowNodeBase {
   BasicBlock() { basic_block_entry_node(this) }
 
+  /** Holds if this basic block contains `node`. */
   predicate contains(ControlFlowNode node) { basic_block_member(node, this, _) }
 
+  /** Gets the `ControlFlowNode` at position `pos` in this basic block. */
   ControlFlowNode getNode(int pos) { basic_block_member(result, this, pos) }
 
+  /** Gets a `ControlFlowNode` in this basic block. */
   ControlFlowNode getANode() { basic_block_member(result, this, _) }
 
+  /** Gets a `BasicBlock` that is a direct successor of this basic block. */
   BasicBlock getASuccessor() { bb_successor(this, result) }
 
+  /** Gets a `BasicBlock` that is a direct predecessor of this basic block. */
   BasicBlock getAPredecessor() { bb_successor(result, this) }
 
+  /**
+   * Gets a `BasicBlock` such that the control-flow edge `(this, result)` may be taken
+   * when the outgoing edge of this basic block is an expression that is true.
+   */
   BasicBlock getATrueSuccessor() { result.getStart() = this.getEnd().getATrueSuccessor() }
 
+  /**
+   * Gets a `BasicBlock` such that the control-flow edge `(this, result)` may be taken
+   * when the outgoing edge of this basic block is an expression that is false.
+   */
   BasicBlock getAFalseSuccessor() { result.getStart() = this.getEnd().getAFalseSuccessor() }
 
+  /** Gets the final `ControlFlowNode` of this basic block. */
   ControlFlowNode getEnd() { basic_block_member(result, this, bb_length(this) - 1) }
 
+  /** Gets the first `ControlFlowNode` of this basic block. */
   ControlFlowNode getStart() { result = this }
 
   /** Gets the number of `ControlFlowNode`s in this basic block. */
@@ -192,6 +212,7 @@ class BasicBlock extends ControlFlowNodeBase {
     this.getEnd().getLocation().hasLocationInfo(endf, _, _, endl, endc)
   }
 
+  /** Gets the function containing this basic block. */
   Function getEnclosingFunction() { result = this.getStart().getControlFlowScope() }
 
   /**

cpp/ql/src/semmle/code/cpp/controlflow/ControlFlowGraph.qll

@@ -1,3 +1,8 @@
+/**
+ * Provides a library for reasoning about control flow at the granularity of
+ * individual nodes in the control-flow graph.
+ */
+
 import cpp
 import BasicBlocks
 private import semmle.code.cpp.controlflow.internal.ConstantExprs
@@ -29,8 +34,10 @@ private import semmle.code.cpp.controlflow.internal.CFG
  * `Handler`. There are no edges from function calls to `Handler`s.
  */
 class ControlFlowNode extends Locatable, ControlFlowNodeBase {
+  /** Gets a direct successor of this control-flow node, if any. */
   ControlFlowNode getASuccessor() { successors_adapted(this, result) }
 
+  /** Gets a direct predecessor of this control-flow node, if any. */
   ControlFlowNode getAPredecessor() { this = result.getASuccessor() }
 
   /** Gets the function containing this control-flow node. */
@@ -71,6 +78,7 @@ class ControlFlowNode extends Locatable, ControlFlowNodeBase {
     result = getASuccessor()
   }
 
+  /** Gets the `BasicBlock` containing this control-flow node. */
   BasicBlock getBasicBlock() { result.getANode() = this }
 }
 
@@ -86,10 +94,18 @@ import ControlFlowGraphPublic
  */
 class ControlFlowNodeBase extends ElementBase, @cfgnode { }
 
+/**
+ * Holds when `n2` is a control-flow node such that the control-flow
+ * edge `(n1, n2)` may be taken when `n1` is an expression that is true.
+ */
 predicate truecond_base(ControlFlowNodeBase n1, ControlFlowNodeBase n2) {
   qlCFGTrueSuccessor(n1, n2)
 }
 
+/**
+ * Holds when `n2` is a control-flow node such that the control-flow
+ * edge `(n1, n2)` may be taken when `n1` is an expression that is false.
+ */
 predicate falsecond_base(ControlFlowNodeBase n1, ControlFlowNodeBase n2) {
   qlCFGFalseSuccessor(n1, n2)
 }

cpp/ql/src/semmle/code/cpp/controlflow/Dataflow.qll

@@ -15,14 +15,25 @@ import Dereferenced
 abstract class DataflowAnnotation extends string {
   DataflowAnnotation() { this = "pointer-null" or this = "pointer-valid" }
 
+  /** Holds if this annotation is the default annotation. */
   abstract predicate isDefault();
 
+  /** Holds if this annotation is generated when analyzing expression `e`. */
   abstract predicate generatedOn(Expr e);
 
+  /**
+   * Holds if this annotation is generated for the variable `v` when
+   * the control-flow edge `(src, dest)` is taken.
+   */
   abstract predicate generatedBy(LocalScopeVariable v, ControlFlowNode src, ControlFlowNode dest);
 
+  /**
+   * Holds if this annotation is removed for the variable `v` when
+   * the control-flow edge `(src, dest)` is taken.
+   */
   abstract predicate killedBy(LocalScopeVariable v, ControlFlowNode src, ControlFlowNode dest);
 
+  /** Holds if expression `e` is given this annotation. */
   predicate marks(Expr e) {
     this.generatedOn(e) and reachable(e)
     or
@@ -31,6 +42,7 @@ abstract class DataflowAnnotation extends string {
     exists(LocalScopeVariable v | this.marks(v, e) and e = v.getAnAccess())
   }
 
+  /** Holds if the variable `v` accessed in control-flow node `n` is given this annotation. */
   predicate marks(LocalScopeVariable v, ControlFlowNode n) {
     v.getAnAccess().getEnclosingFunction().getBlock() = n and
     this.isDefault()
@@ -57,13 +69,21 @@ abstract class DataflowAnnotation extends string {
     )
   }
 
+  /**
+   * Holds if the variable `v` preserves this annotation when the control-flow
+   * edge `(src, dest)` is taken.
+   */
   predicate preservedBy(LocalScopeVariable v, ControlFlowNode src, ControlFlowNode dest) {
     this.marks(v, src) and
     src.getASuccessor() = dest and
     not v.getInitializer() = dest and
     not v.getAnAssignment() = src
   }
 
+  /**
+   * Holds if the variable `v` is assigned this annotation when `src` is an assignment
+   * expression that assigns to `v` and the control-flow edge `(src, dest)` is taken.
+   */
   predicate assignedBy(LocalScopeVariable v, ControlFlowNode src, ControlFlowNode dest) {
     this.marks(src.(AssignExpr).getRValue()) and
     src = v.getAnAssignment() and

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

@@ -1,6 +1,7 @@
 import cpp
 import semmle.code.cpp.security.Security
 private import semmle.code.cpp.ir.dataflow.DataFlow
+private import semmle.code.cpp.ir.dataflow.internal.DataFlowUtil
 private import semmle.code.cpp.ir.dataflow.DataFlow2
 private import semmle.code.cpp.ir.dataflow.DataFlow3
 private import semmle.code.cpp.ir.IR

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

@@ -71,9 +71,7 @@ class Node extends TIRDataFlowNode {
    * `x.set(taint())` is a partial definition of `x`, and `transfer(&x, taint())` is
    * a partial definition of `&x`).
    */
-  Expr asPartialDefinition() {
-    result = this.(PartialDefinitionNode).getInstruction().getUnconvertedResultExpression()
-  }
+  Expr asPartialDefinition() { result = this.(PartialDefinitionNode).getDefinedExpr() }
 
   /**
    * DEPRECATED: See UninitializedNode.
@@ -251,14 +249,17 @@ abstract class PostUpdateNode extends InstructionNode {
  * setY(&x); // a partial definition of the object `x`.
  * ```
  */
-abstract private class PartialDefinitionNode extends PostUpdateNode, TInstructionNode { }
+abstract private class PartialDefinitionNode extends PostUpdateNode, TInstructionNode {
+  abstract Expr getDefinedExpr();
+}
 
 private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
   override ChiInstruction instr;
+  FieldAddressInstruction field;
 
   ExplicitFieldStoreQualifierNode() {
     not instr.isResultConflated() and
-    exists(StoreInstruction store, FieldInstruction field |
+    exists(StoreInstruction store |
       instr.getPartial() = store and field = store.getDestinationAddress()
     )
   }
@@ -268,6 +269,10 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
   // DataFlowImplConsistency::Consistency. However, it's not clear what (if any) implications
   // this consistency failure has.
   override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
+
+  override Expr getDefinedExpr() {
+    result = field.getObjectAddress().getUnconvertedResultExpression()
+  }
 }
 
 /**
@@ -278,15 +283,18 @@ private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
  */
 private class ExplicitSingleFieldStoreQualifierNode extends PartialDefinitionNode {
   override StoreInstruction instr;
+  FieldAddressInstruction field;
 
   ExplicitSingleFieldStoreQualifierNode() {
-    exists(FieldAddressInstruction field |
-      field = instr.getDestinationAddress() and
-      not exists(ChiInstruction chi | chi.getPartial() = instr)
-    )
+    field = instr.getDestinationAddress() and
+    not exists(ChiInstruction chi | chi.getPartial() = instr)
   }
 
   override Node getPreUpdateNode() { none() }
+
+  override Expr getDefinedExpr() {
+    result = field.getObjectAddress().getUnconvertedResultExpression()
+  }
 }
 
 /**
@@ -458,9 +466,9 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // for now.
   iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
   or
-  // The next two rules allow flow from partial definitions in setters to succeeding loads in the caller.
-  // First, we add flow from write side-effects to non-conflated chi instructions through their
-  // partial operands. Consider the following example:
+  // Add flow from write side-effects to non-conflated chi instructions through their
+  // partial operands. From there, a `readStep` will find subsequent reads of that field.
+  // Consider the following example:
   // ```
   // void setX(Point* p, int new_x) {
   //   p->x = new_x;
@@ -470,14 +478,9 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // ```
   // Here, a `WriteSideEffectInstruction` will provide a new definition for `p->x` after the call to
   // `setX`, which will be melded into `p` through a chi instruction.
-  iTo.getAnOperand().(ChiPartialOperand).getDef() = iFrom.(WriteSideEffectInstruction) and
-  not iTo.isResultConflated()
-  or
-  // Next, we add flow from non-conflated chi instructions to loads (even when they are not precise).
-  // This ensures that loads of `p->x` gets data flow from the `WriteSideEffectInstruction` above.
-  exists(ChiInstruction chi | iFrom = chi |
-    not chi.isResultConflated() and
-    iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = chi
+  exists(ChiInstruction chi | chi = iTo |
+    chi.getPartialOperand().getDef() = iFrom.(WriteSideEffectInstruction) and
+    not chi.isResultConflated()
   )
   or
   // Flow from stores to structs with a single field to a load of that field.

cpp/ql/src/semmle/code/cpp/ir/internal/CppType.qll

@@ -362,7 +362,7 @@ CppType getTypeForPRValueOrUnknown(Type type) {
 /**
  * Gets the `CppType` that represents a glvalue of type `type`.
  */
-CppType getTypeForGLValue(Type type) { result.hasType(type, true) }
+CppGLValueAddressType getTypeForGLValue(Type type) { result.hasType(type, true) }
 
 /**
  * Gets the `CppType` that represents a prvalue of type `int`.

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

@@ -115,5 +115,5 @@ void test_conflated_fields3() {
   XY xy;
   xy.x = 0;
   taint_y(&xy);
-  sink(xy.x); // not tainted [FALSE POSITIVE]
+  sink(xy.x); // not tainted
 }

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

@@ -103,8 +103,6 @@
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:110:17:110:32 | (int)... |
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:110:17:110:32 | access to array |
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:111:12:111:18 | tainted |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:118:11:118:11 | x |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | shared.h:6:15:6:23 | sinkparam |
 | dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:24:28:27 | call to atoi |
 | dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:29:28:34 | call to getenv |
 | dispatch.cpp:28:29:28:34 | call to getenv | dispatch.cpp:28:29:28:45 | (const char *)... |

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

@@ -21,8 +21,6 @@
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | defaulttainttracking.cpp:98:10:98:11 | p2 | IR only |
 | defaulttainttracking.cpp:97:27:97:32 | call to getenv | shared.h:5:23:5:31 | sinkparam | IR only |
 | defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:111:8:111:8 | y | AST only |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | defaulttainttracking.cpp:118:11:118:11 | x | IR only |
-| defaulttainttracking.cpp:110:17:110:22 | call to getenv | shared.h:6:15:6:23 | sinkparam | IR only |
 | globals.cpp:13:15:13:20 | call to getenv | globals.cpp:13:5:13:11 | global1 | AST only |
 | globals.cpp:23:15:23:20 | call to getenv | globals.cpp:23:5:23:11 | global2 | AST only |
 | test_diff.cpp:104:12:104:15 | argv | test_diff.cpp:104:11:104:20 | (...) | IR only |