Merge branch 'master' of git.semmle.com:Semmle/ql into Mispelled

Author: erik-krogh

change-notes/1.24/analysis-cpp.md

@@ -25,6 +25,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
 | Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | Cases where the tainted allocation size is range checked are now more reliably excluded. |
+| Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | Fewer false positive results | The query now produces fewer, more accurate results. |
 | Overloaded assignment does not return 'this' (`cpp/assignment-does-not-return-this`) | Fewer false positive results | This query no longer reports incorrect results in template classes. |
 | Unsafe array for days of the year (`cpp/leap-year/unsafe-array-for-days-of-the-year`) |  | This query is no longer run on LGTM. |
 | Unsigned comparison to zero (`cpp/unsigned-comparison-zero`) | More correct results | This query now also looks for comparisons of the form `0 <= x`. |

change-notes/1.24/analysis-javascript.md

@@ -86,7 +86,7 @@
 | Useless regular-expression character escape (`js/useless-regexp-character-escape`) | Fewer false positive results | This query now distinguishes escapes in strings and regular expression literals. |
 | Identical operands (`js/redundant-operation`) | Fewer results | This query now recognizes cases where the operands change a value using ++/-- expressions. |
 | Superfluous trailing arguments (`js/superfluous-trailing-arguments`) | Fewer results | This query now recognizes cases where a function uses the `Function.arguments` value to process a variable number of parameters. |
-| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes more variations of URL scheme checks. |
+| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional variations of URL scheme checks. |
 
 ## Changes to libraries
 

change-notes/1.25/analysis-javascript.md

@@ -14,6 +14,8 @@
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
+| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
+| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
 
 ## Changes to libraries
 

cpp/ql/src/Security/CWE/CWE-190/TaintedAllocationSize.ql

@@ -15,31 +15,31 @@ import cpp
 import semmle.code.cpp.security.TaintTracking
 import TaintedWithPath
 
-predicate taintedChild(Expr e, Expr tainted) {
-  (
-    isAllocationExpr(e)
-    or
-    any(MulExpr me | me.getAChild() instanceof SizeofOperator) = e
-  ) and
-  tainted = e.getAChild() and
+/**
+ * Holds if `alloc` is an allocation, and `tainted` is a child of it that is a
+ * taint sink.
+ */
+predicate allocSink(Expr alloc, Expr tainted) {
+  isAllocationExpr(alloc) and
+  tainted = alloc.getAChild() and
   tainted.getUnspecifiedType() instanceof IntegralType
 }
 
 class TaintedAllocationSizeConfiguration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) { taintedChild(_, tainted) }
+  override predicate isSink(Element tainted) { allocSink(_, tainted) }
 }
 
 predicate taintedAllocSize(
-  Expr e, Expr source, PathNode sourceNode, PathNode sinkNode, string taintCause
+  Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
 ) {
   isUserInput(source, taintCause) and
   exists(Expr tainted |
-    taintedChild(e, tainted) and
+    allocSink(alloc, tainted) and
     taintedWithPath(source, tainted, sourceNode, sinkNode)
   )
 }
 
-from Expr e, Expr source, PathNode sourceNode, PathNode sinkNode, string taintCause
-where taintedAllocSize(e, source, sourceNode, sinkNode, taintCause)
-select e, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow",
+from Expr source, Expr alloc, PathNode sourceNode, PathNode sinkNode, string taintCause
+where taintedAllocSize(source, alloc, sourceNode, sinkNode, taintCause)
+select alloc, sourceNode, sinkNode, "This allocation size is derived from $@ and might overflow",
   source, "user input (" + taintCause + ")"

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

@@ -186,7 +186,12 @@ private class ArrayContent extends Content, TArrayContent {
  * value of `node1`.
  */
 predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
-  none() // stub implementation
+  exists(FieldAddressInstruction fa, StoreInstruction store |
+    node1.asInstruction() = store and
+    store.getDestinationAddress() = fa and
+    node2.asInstruction().(ChiInstruction).getPartial() = store and
+    f.(FieldContent).getField() = fa.getField()
+  )
 }
 
 /**
@@ -195,7 +200,12 @@ predicate storeStep(Node node1, Content f, PostUpdateNode node2) {
  * `node2`.
  */
 predicate readStep(Node node1, Content f, Node node2) {
-  none() // stub implementation
+  exists(FieldAddressInstruction fa, LoadInstruction load |
+    load.getSourceAddress() = fa and
+    node1.asInstruction() = load.getSourceValueOperand().getAnyDef() and
+    fa.getField() = f.(FieldContent).getField() and
+    load = node2.asInstruction()
+  )
 }
 
 /**

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

@@ -55,14 +55,26 @@ class Node extends TIRDataFlowNode {
   Expr asDefiningArgument() { result = this.(DefinitionByReferenceNode).getArgument() }
 
   /** Gets the parameter corresponding to this node, if any. */
-  Parameter asParameter() { result = this.(ParameterNode).getParameter() }
+  Parameter asParameter() { result = this.(ExplicitParameterNode).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() }
 
+  /**
+   * Gets the expression that is partially defined by this node, if any.
+   *
+   * Partial definitions are created for field stores (`x.y = taint();` is a partial
+   * definition of `x`), and for calls that may change the value of an object (so
+   * `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()
+  }
+
   /**
    * DEPRECATED: See UninitializedNode.
    *
@@ -96,6 +108,9 @@ class Node extends TIRDataFlowNode {
   string toString() { none() } // overridden by subclasses
 }
 
+/**
+ * An instruction, viewed as a node in a data flow graph.
+ */
 class InstructionNode extends Node, TInstructionNode {
   Instruction instr;
 
@@ -143,26 +158,45 @@ class ExprNode extends InstructionNode {
 }
 
 /**
- * The value of a parameter at function entry, viewed as a node in a data
- * flow graph.
+ * A node representing a `Parameter`. This includes both explicit parameters such
+ * as `x` in `f(x)` and implicit parameters such as `this` in `x.f()`
  */
 class ParameterNode extends InstructionNode {
-  override InitializeParameterInstruction instr;
+  ParameterNode() {
+    instr instanceof InitializeParameterInstruction
+    or
+    instr instanceof InitializeThisInstruction
+  }
 
   /**
    * Holds if this node is the parameter of `c` at the specified (zero-based)
    * position. The implicit `this` parameter is considered to have index `-1`.
    */
-  predicate isParameterOf(Function f, int i) { f.getParameter(i) = instr.getParameter() }
+  predicate isParameterOf(Function f, int i) { none() } // overriden by subclasses
+}
+
+/**
+ * The value of a parameter at function entry, viewed as a node in a data
+ * flow graph.
+ */
+private class ExplicitParameterNode extends ParameterNode {
+  override InitializeParameterInstruction instr;
 
+  override predicate isParameterOf(Function f, int i) { f.getParameter(i) = instr.getParameter() }
+
+  /** Gets the parameter corresponding to this node. */
   Parameter getParameter() { result = instr.getParameter() }
 
   override string toString() { result = instr.getParameter().toString() }
 }
 
-private class ThisParameterNode extends InstructionNode {
+private class ThisParameterNode extends ParameterNode {
   override InitializeThisInstruction instr;
 
+  override predicate isParameterOf(Function f, int i) {
+    i = -1 and instr.getEnclosingFunction() = f
+  }
+
   override string toString() { result = "this" }
 }
 
@@ -204,6 +238,38 @@ abstract class PostUpdateNode extends InstructionNode {
   abstract Node getPreUpdateNode();
 }
 
+/**
+ * The base class for nodes that perform "partial definitions".
+ *
+ * In contrast to a normal "definition", which provides a new value for
+ * something, a partial definition is an expression that may affect a
+ * value, but does not necessarily replace it entirely. For example:
+ * ```
+ * x.y = 1; // a partial definition of the object `x`.
+ * x.y.z = 1; // a partial definition of the object `x.y`.
+ * x.setY(1); // a partial definition of the object `x`.
+ * setY(&x); // a partial definition of the object `x`.
+ * ```
+ */
+abstract private class PartialDefinitionNode extends PostUpdateNode, TInstructionNode { }
+
+private class ExplicitFieldStoreQualifierNode extends PartialDefinitionNode {
+  override ChiInstruction instr;
+
+  ExplicitFieldStoreQualifierNode() {
+    not instr.isResultConflated() and
+    exists(StoreInstruction store, FieldInstruction field |
+      instr.getPartial() = store and field = store.getDestinationAddress()
+    )
+  }
+
+  // There might be multiple `ChiInstructions` that has a particular instruction as
+  // the total operand - so this definition gives consistency errors in
+  // DataFlowImplConsistency::Consistency. However, it's not clear what (if any) implications
+  // this consistency failure has.
+  override Node getPreUpdateNode() { result.asInstruction() = instr.getTotal() }
+}
+
 /**
  * A node that represents the value of a variable after a function call that
  * may have changed the variable because it's passed by reference.
@@ -288,6 +354,10 @@ class VariableNode extends Node, TVariableNode {
  */
 InstructionNode instructionNode(Instruction instr) { result.getInstruction() = instr }
 
+/**
+ * Gets the `Node` corresponding to a definition by reference of the variable
+ * that is passed as `argument` of a call.
+ */
 DefinitionByReferenceNode definitionByReferenceNode(Expr e) { result.getArgument() = e }
 
 /**
@@ -305,7 +375,7 @@ ExprNode convertedExprNode(Expr e) { result.getConvertedExpr() = e }
 /**
  * Gets the `Node` corresponding to the value of `p` at function entry.
  */
-ParameterNode parameterNode(Parameter p) { result.getParameter() = p }
+ExplicitParameterNode parameterNode(Parameter p) { result.getParameter() = p }
 
 /** Gets the `VariableNode` corresponding to the variable `v`. */
 VariableNode variableNode(Variable v) { result.getVariable() = v }
@@ -360,6 +430,28 @@ 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:
+  // ```
+  // void setX(Point* p, int new_x) {
+  //   p->x = new_x;
+  // }
+  // ...
+  // setX(&p, taint());
+  // ```
+  // 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
+  )
+  or
   // Flow through modeled functions
   modelFlow(iFrom, iTo)
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintSSA.qll

@@ -6,11 +6,12 @@ private import DebugSSA
 
 bindingset[offset]
 private string getKeySuffixForOffset(int offset) {
+  offset >= 0 and
   if offset % 2 = 0 then result = "" else result = "_Chi"
 }
 
 bindingset[offset]
-private int getIndexForOffset(int offset) { result = offset / 2 }
+private int getIndexForOffset(int offset) { offset >= 0 and result = offset / 2 }
 
 /**
  * Property provide that dumps the memory access of each result. Useful for debugging SSA

cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/PrintSSA.qll

@@ -6,11 +6,12 @@ private import DebugSSA
 
 bindingset[offset]
 private string getKeySuffixForOffset(int offset) {
+  offset >= 0 and
   if offset % 2 = 0 then result = "" else result = "_Chi"
 }
 
 bindingset[offset]
-private int getIndexForOffset(int offset) { result = offset / 2 }
+private int getIndexForOffset(int offset) { offset >= 0 and result = offset / 2 }
 
 /**
  * Property provide that dumps the memory access of each result. Useful for debugging SSA

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

@@ -23,17 +23,20 @@ Type getVariableType(Variable v) {
     then
       result = getDecayedType(declaredType)
       or
-      not exists(getDecayedType(declaredType)) and result = declaredType
+      not exists(getDecayedType(declaredType)) and result = v.getType()
     else
       if declaredType instanceof ArrayType and not declaredType.(ArrayType).hasArraySize()
       then
-        result = v.getInitializer().getExpr().getUnspecifiedType()
+        result = v.getInitializer().getExpr().getType()
         or
-        not exists(v.getInitializer()) and result = declaredType
-      else result = declaredType
+        not exists(v.getInitializer()) and result = v.getType()
+      else result = v.getType()
   )
 }
 
+/**
+ * Holds if the database contains a `case` label with the specified minimum and maximum value.
+ */
 predicate hasCaseEdge(SwitchCase switchCase, string minValue, string maxValue) {
   minValue = switchCase.getExpr().getFullyConverted().getValue() and
   if exists(switchCase.getEndExpr())

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

@@ -30,6 +30,8 @@ localCallNodes
 postIsNotPre
 postHasUniquePre
 uniquePostUpdate
+| ref.cpp:83:5:83:17 | Chi | Node has multiple PostUpdateNodes. |
+| ref.cpp:109:5:109:22 | Chi | Node has multiple PostUpdateNodes. |
 postIsInSameCallable
 reverseRead
 storeIsPostUpdate