Merge remote-tracking branch 'upstream/master' into Maps

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

@@ -0,0 +1,22 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+
+## New queries
+
+| **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
+|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **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/Class.qll

@@ -458,6 +458,15 @@ class Class extends UserType {
     exists(ClassDerivation d | d.getDerivedClass() = this and d = result)
   }
 
+  /**
+   * Gets class derivation number `index` of this class/struct, for example the
+   * `public B` is derivation 1 in the following code:
+   * ```
+   * class D : public A, public B, public C {
+   *   ...
+   * };
+   * ```
+   */
   ClassDerivation getDerivation(int index) {
     exists(ClassDerivation d | d.getDerivedClass() = this and d.getIndex() = index and d = result)
   }
@@ -900,6 +909,22 @@ class AbstractClass extends Class {
 class TemplateClass extends Class {
   TemplateClass() { usertypes(underlyingElement(this), _, 6) }
 
+  /**
+   * Gets a class instantiated from this template.
+   *
+   * For example for `MyTemplateClass<T>` in the following code, the results are
+   * `MyTemplateClass<int>` and `MyTemplateClass<long>`:
+   * ```
+   * template<class T>
+   * class MyTemplateClass {
+   *   ...
+   * };
+   *
+   * MyTemplateClass<int> instance;
+   *
+   * MyTemplateClass<long> instance;
+   * ```
+   */
   Class getAnInstantiation() {
     result.isConstructedFrom(this) and
     exists(result.getATemplateArgument())

cpp/ql/src/semmle/code/cpp/Comments.qll

@@ -13,8 +13,20 @@ class Comment extends Locatable, @comment {
 
   override Location getLocation() { comments(underlyingElement(this), _, result) }
 
+  /**
+   * Gets the text of this comment, including the opening `//` or `/*`, and the closing `*``/` if
+   * present.
+   */
   string getContents() { comments(underlyingElement(this), result, _) }
 
+  /**
+   * Gets the AST element this comment is associated with. For example, the comment in the
+   * following code is associated with the declaration of `j`.
+   * ```
+   * int i;
+   * int j; // Comment on j
+   * ```
+   */
   Element getCommentedElement() {
     commentbinding(underlyingElement(this), unresolveElement(result))
   }

cpp/ql/src/semmle/code/cpp/Compilation.qll

@@ -40,6 +40,7 @@ class Compilation extends @compilation {
   /** Gets a file compiled during this invocation. */
   File getAFileCompiled() { result = getFileCompiled(_) }
 
+  /** Gets the `i`th file compiled during this invocation */
   File getFileCompiled(int i) { compilation_compiling_files(this, i, unresolveElement(result)) }
 
   /**

cpp/ql/src/semmle/code/cpp/Diagnostics.qll

@@ -11,6 +11,7 @@ class Diagnostic extends Locatable, @diagnostic {
   /** Gets the error code for this compiler message. */
   string getTag() { diagnostics(underlyingElement(this), _, result, _, _, _) }
 
+  /** Holds if `s` is the error code for this compiler message. */
   predicate hasTag(string s) { this.getTag() = s }
 
   /**

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)
 }