Merge remote-tracking branch 'github/master' into github/codeql-c-analysis-team/69_union

Author: Dave Bartolomeo

CONTRIBUTING.md

@@ -2,7 +2,7 @@
 
 We welcome contributions to our CodeQL libraries and queries. Got an idea for a new check, or how to improve an existing query? Then please go ahead and open a pull request! Contributions to this project are [released](https://help.github.com/articles/github-terms-of-service/#6-contributions-under-repository-license) to the public under the [project's open source license](LICENSE).
 
-There is lots of useful documentation to help you write queries, ranging from information about query file structure to tutorials for specific target languages. For more information on the documentation available, see [Writing CodeQL queries](https://help.semmle.com/QL/learn-ql/writing-queries/writing-queries.html) on [help.semmle.com](https://help.semmle.com).
+There is lots of useful documentation to help you write queries, ranging from information about query file structure to tutorials for specific target languages. For more information on the documentation available, see [CodeQL queries](https://help.semmle.com/QL/learn-ql/writing-queries/writing-queries.html) on [help.semmle.com](https://help.semmle.com).
 
 
 ## Submitting a new experimental query
@@ -32,7 +32,7 @@ If you have an idea for a query that you would like to share with other CodeQL u
 
     For details, see the [guide on query metadata](docs/query-metadata-style-guide.md).
 
-    Make sure the `select` statement is compatible with the query `@kind`. See [Introduction to query files](https://help.semmle.com/QL/learn-ql/writing-queries/introduction-to-queries.html#select-clause) on help.semmle.com.
+    Make sure the `select` statement is compatible with the query `@kind`. See [About CodeQL queries](https://help.semmle.com/QL/learn-ql/writing-queries/introduction-to-queries.html#select-clause) on help.semmle.com.
 
 3. **Formatting**
 

change-notes/1.25/analysis-javascript.md

@@ -43,6 +43,7 @@
 | Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Fewer results | This query now recognizes additional safe patterns of doing URL redirects. |
 | 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. |
+| Hard-coded credentials (`js/hardcoded-credentials`) | More results | This query now recognizes hard-coded credentials sent via HTTP authorization headers. |
 | Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
 | Misspelled variable name (`js/misspelled-variable-name`) | Message changed | The message for this query now correctly identifies the misspelled variable in additional cases. |
 | Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |

change-notes/1.25/analysis-python.md

@@ -0,0 +1,22 @@
+# Improvements to Python analysis
+
+The following changes in version 1.25 affect Python analysis in all applications.
+
+## General improvements
+
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+
+
+## Changes to libraries
+
+* Importing `semmle.python.web.HttpRequest` will no longer import `UntrustedStringKind` transitively. `UntrustedStringKind` is the most commonly used non-abstract subclass of `ExternalStringKind`. If not imported (by one mean or another), taint-tracking queries that concern `ExternalStringKind` will not produce any results. Please ensure such queries contain an explicit import (`import semmle.python.security.strings.Untrusted`).

config/sync-files.py

@@ -59,29 +59,41 @@ def file_checksum(filename):
         return hashlib.sha1(file_handle.read()).hexdigest()
 
 def check_group(group_name, files, master_file_picker, emit_error):
-    checksums = {file_checksum(f) for f in files}
+    extant_files = [f for f in files if path.isfile(f)]
+    if len(extant_files) == 0:
+        emit_error(__file__, 0, "No files found from group '" + group_name + "'.")
+        emit_error(__file__, 0,
+                "Create one of the following files, and then run this script with "
+                "the --latest switch to sync it to the other file locations.")
+        for filename in files:
+            emit_error(__file__, 0, "    " + filename)
+        return
+
+    checksums = {file_checksum(f) for f in extant_files}
 
-    if len(checksums) == 1:
+    if len(checksums) == 1 and len(extant_files) == len(files):
+        # All files are present and identical.
         return
 
-    master_file = master_file_picker(files)
+    master_file = master_file_picker(extant_files)
     if master_file is None:
         emit_error(__file__, 0,
                 "Files from group '"+ group_name +"' not in sync.")
         emit_error(__file__, 0,
                 "Run this script with a file-name argument among the "
                 "following to overwrite the remaining files with the contents "
-                "of that file or run with the --latest switch to update each "
+                "of that file, or run with the --latest switch to update each "
                 "group of files from the most recently modified file in the group.")
-        for filename in files:
+        for filename in extant_files:
             emit_error(__file__, 0, "    " + filename)
     else:
         print("  Syncing others from", master_file)
         for filename in files:
             if filename == master_file:
                 continue
             print("    " + filename)
-            os.replace(filename, filename + '~')
+            if path.isfile(filename):
+                os.replace(filename, filename + '~')
             shutil.copy(master_file, filename)
         print("  Backups written with '~' appended to file names")
 

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

@@ -67,6 +67,9 @@ private DataFlow::Node getNodeForSource(Expr source) {
     // to `gets`. It's impossible here to tell which is which, but the "access
     // to argv" source is definitely not intended to match an output argument,
     // and it causes false positives if we let it.
+    //
+    // This case goes together with the similar (but not identical) rule in
+    // `nodeIsBarrierIn`.
     result = DataFlow::definitionByReferenceNode(source) and
     not argv(source.(VariableAccess).getTarget())
   )
@@ -202,7 +205,13 @@ private predicate nodeIsBarrier(DataFlow::Node node) {
 
 private predicate nodeIsBarrierIn(DataFlow::Node node) {
   // don't use dataflow into taint sources, as this leads to duplicate results.
-  node = getNodeForSource(any(Expr e))
+  exists(Expr source | isUserInput(source, _) |
+    node = DataFlow::exprNode(source)
+    or
+    // This case goes together with the similar (but not identical) rule in
+    // `getNodeForSource`.
+    node = DataFlow::definitionByReferenceNode(source)
+  )
 }
 
 cached

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

@@ -82,47 +82,45 @@ private module VirtualDispatch {
         exists(LoadInstruction load, GlobalOrNamespaceVariable var |
           var = src.asVariable() and
           other.asInstruction() = load and
+          addressOfGlobal(load.getSourceAddress(), var) and
           // The `allowFromArg` concept doesn't play a role when `src` is a
           // global variable, so we just set it to a single arbitrary value for
           // performance.
           allowFromArg = true
-        |
-          // Load directly from the global variable
-          load.getSourceAddress().(VariableAddressInstruction).getASTVariable() = var
-          or
-          // Load from a field on a global union
-          exists(FieldAddressInstruction fa |
-            fa = load.getSourceAddress() and
-            fa.getObjectAddress().(VariableAddressInstruction).getASTVariable() = var and
-            fa.getField().getDeclaringType() instanceof Union
-          )
         )
         or
-        // Flow from store to global variable. These cases are similar to the
-        // above but have `StoreInstruction` instead of `LoadInstruction` and
-        // have the roles swapped between `other` and `src`.
+        // Flow from store to global variable.
         exists(StoreInstruction store, GlobalOrNamespaceVariable var |
           var = other.asVariable() and
           store = src.asInstruction() and
+          storeIntoGlobal(store, var) and
           // Setting `allowFromArg` to `true` like in the base case means we
           // treat a store to a global variable like the dispatch itself: flow
           // may come from anywhere.
           allowFromArg = true
-        |
-          // Store directly to the global variable
-          store.getDestinationAddress().(VariableAddressInstruction).getASTVariable() = var
-          or
-          // Store to a field on a global union
-          exists(FieldAddressInstruction fa |
-            fa = store.getDestinationAddress() and
-            fa.getObjectAddress().(VariableAddressInstruction).getASTVariable() = var and
-            fa.getField().getDeclaringType() instanceof Union
-          )
         )
       )
     }
   }
 
+  pragma[noinline]
+  private predicate storeIntoGlobal(StoreInstruction store, GlobalOrNamespaceVariable var) {
+    addressOfGlobal(store.getDestinationAddress(), var)
+  }
+
+  /** Holds if `addressInstr` is an instruction that produces the address of `var`. */
+  private predicate addressOfGlobal(Instruction addressInstr, GlobalOrNamespaceVariable var) {
+    // Access directly to the global variable
+    addressInstr.(VariableAddressInstruction).getASTVariable() = var
+    or
+    // Access to a field on a global union
+    exists(FieldAddressInstruction fa |
+      fa = addressInstr and
+      fa.getObjectAddress().(VariableAddressInstruction).getASTVariable() = var and
+      fa.getField().getDeclaringType() instanceof Union
+    )
+  }
+
   /**
    * A ReturnNode with its ReturnKind and its enclosing callable.
    *

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

@@ -8,16 +8,28 @@ private import DataFlowDispatch
  * to the callable. Instance arguments (`this` pointer) are also included.
  */
 class ArgumentNode extends InstructionNode {
-  ArgumentNode() { exists(CallInstruction call | this.getInstruction() = call.getAnArgument()) }
+  ArgumentNode() {
+    exists(CallInstruction call |
+      instr = call.getAnArgument()
+      or
+      instr.(ReadSideEffectInstruction).getPrimaryInstruction() = call
+    )
+  }
 
   /**
    * 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.getInstruction() = call.getPositionalArgument(pos)
+    instr = call.getPositionalArgument(pos)
     or
-    this.getInstruction() = call.getThisArgument() and pos = -1
+    instr = call.getThisArgument() and pos = -1
+    or
+    exists(ReadSideEffectInstruction read |
+      read = instr and
+      read.getPrimaryInstruction() = call and
+      pos = getArgumentPosOfSideEffect(read.getIndex())
+    )
   }
 
   /** Gets the call in which this node is an argument. */

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

@@ -54,7 +54,7 @@ class Node extends TIRDataFlowNode {
   /** Gets the argument that defines this `DefinitionByReferenceNode`, if any. */
   Expr asDefiningArgument() { result = this.(DefinitionByReferenceNode).getArgument() }
 
-  /** Gets the parameter corresponding to this node, if any. */
+  /** Gets the positional parameter corresponding to this node, if any. */
   Parameter asParameter() { result = this.(ExplicitParameterNode).getParameter() }
 
   /**
@@ -156,44 +156,90 @@ class ExprNode extends InstructionNode {
 }
 
 /**
- * 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()`
+ * INTERNAL: do not use. Translates a parameter/argument index into a negative
+ * number that denotes the index of its side effect (pointer indirection).
+ */
+bindingset[index]
+int getArgumentPosOfSideEffect(int index) {
+  // -1 -> -2
+  //  0 -> -3
+  //  1 -> -4
+  // ...
+  result = -3 - index
+}
+
+/**
+ * The value of a parameter at function entry, viewed as a node in a data
+ * flow graph. This includes both explicit parameters such as `x` in `f(x)`
+ * and implicit parameters such as `this` in `x.f()`.
+ *
+ * To match a specific kind of parameter, consider using one of the subclasses
+ * `ExplicitParameterNode`, `ThisParameterNode`, or
+ * `ParameterIndirectionNode`.
  */
 class ParameterNode extends InstructionNode {
-  override InitializeParameterInstruction instr;
+  ParameterNode() {
+    // To avoid making this class abstract, we enumerate its values here
+    instr instanceof InitializeParameterInstruction
+    or
+    instr instanceof InitializeIndirectionInstruction
+  }
 
   /**
-   * 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`.
+   * Holds if this node is the parameter of `f` at the specified position. The
+   * implicit `this` parameter is considered to have position `-1`, and
+   * pointer-indirection parameters are at further negative positions.
    */
-  predicate isParameterOf(Function f, int i) { none() } // overriden by subclasses
+  predicate isParameterOf(Function f, int pos) { none() } // overridden by subclasses
 }
 
-/**
- * The value of a parameter at function entry, viewed as a node in a data
- * flow graph.
- */
+/** An explicit positional parameter, not including `this` or `...`. */
 private class ExplicitParameterNode extends ParameterNode {
+  override InitializeParameterInstruction instr;
+
   ExplicitParameterNode() { exists(instr.getParameter()) }
 
-  override predicate isParameterOf(Function f, int i) { f.getParameter(i) = instr.getParameter() }
+  override predicate isParameterOf(Function f, int pos) {
+    f.getParameter(pos) = instr.getParameter()
+  }
 
-  /** Gets the parameter corresponding to this node. */
+  /** Gets the `Parameter` associated with this node. */
   Parameter getParameter() { result = instr.getParameter() }
 
   override string toString() { result = instr.getParameter().toString() }
 }
 
-private class ThisParameterNode extends ParameterNode {
+/** An implicit `this` parameter. */
+class ThisParameterNode extends ParameterNode {
+  override InitializeParameterInstruction instr;
+
   ThisParameterNode() { instr.getIRVariable() instanceof IRThisVariable }
 
-  override predicate isParameterOf(Function f, int i) {
-    i = -1 and instr.getEnclosingFunction() = f
+  override predicate isParameterOf(Function f, int pos) {
+    pos = -1 and instr.getEnclosingFunction() = f
   }
 
   override string toString() { result = "this" }
 }
 
+/** A synthetic parameter to model the pointed-to object of a pointer parameter. */
+class ParameterIndirectionNode extends ParameterNode {
+  override InitializeIndirectionInstruction instr;
+
+  override predicate isParameterOf(Function f, int pos) {
+    exists(int index |
+      f.getParameter(index) = instr.getParameter()
+      or
+      index = -1 and
+      instr.getIRVariable().(IRThisVariable).getEnclosingFunction() = f
+    |
+      pos = getArgumentPosOfSideEffect(index)
+    )
+  }
+
+  override string toString() { result = "*" + instr.getIRVariable().toString() }
+}
+
 /**
  * DEPRECATED: Data flow was never an accurate way to determine what
  * expressions might be uninitialized. It errs on the side of saying that
@@ -341,6 +387,18 @@ class DefinitionByReferenceNode extends InstructionNode {
   }
 }
 
+/**
+ * A node representing the memory pointed to by a function argument.
+ *
+ * This class exists only in order to override `toString`, which would
+ * otherwise be the default implementation inherited from `InstructionNode`.
+ */
+private class ArgumentIndirectionNode extends InstructionNode {
+  override ReadSideEffectInstruction instr;
+
+  override string toString() { result = "Argument " + instr.getIndex() + " indirection" }
+}
+
 /**
  * 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
@@ -442,6 +500,31 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   or
   iTo.(PhiInstruction).getAnOperand().getDef() = iFrom
   or
+  // A read side effect is almost never exact since we don't know exactly how
+  // much memory the callee will read.
+  iTo.(ReadSideEffectInstruction).getSideEffectOperand().getAnyDef() = iFrom and
+  not iFrom.isResultConflated()
+  or
+  // Loading a single `int` from an `int *` parameter is not an exact load since
+  // the parameter may point to an entire array rather than a single `int`. The
+  // following rule ensures that any flow going into the
+  // `InitializeIndirectionInstruction`, even if it's for a different array
+  // element, will propagate to a load of the first element.
+  //
+  // Since we're linking `InitializeIndirectionInstruction` and
+  // `LoadInstruction` together directly, this rule will break if there's any
+  // reassignment of the parameter indirection, including a conditional one that
+  // leads to a phi node.
+  exists(InitializeIndirectionInstruction init |
+    iFrom = init and
+    iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = init and
+    // Check that the types match. Otherwise we can get flow from an object to
+    // its fields, which leads to field conflation when there's flow from other
+    // fields to the object elsewhere.
+    init.getParameter().getType().getUnspecifiedType().(DerivedType).getBaseType() =
+      iTo.getResultType().getUnspecifiedType()
+  )
+  or
   // Treat all conversions as flow, even conversions between different numeric types.
   iTo.(ConvertInstruction).getUnary() = iFrom
   or

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll

@@ -56,7 +56,7 @@ class ValueNumber extends TValueNumber {
     or
     this instanceof TInitializeParameterValueNumber and result = "InitializeParameter"
     or
-    this instanceof TInitializeThisValueNumber and result = "InitializeThis"
+    this instanceof TConstantValueNumber and result = "Constant"
     or
     this instanceof TStringConstantValueNumber and result = "StringConstant"
     or