Merge remote-tracking branch 'upstream/master' into dataflow/precise-field-types

Author: hvitved

change-notes/1.25/analysis-cpp.md

@@ -0,0 +1,41 @@
+# Improvements to C/C++ analysis
+
+The following changes in version 1.25 affect C/C++ analysis in all applications.
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+
+## Changes to libraries
+
+* The data-flow library has been improved, which affects most security queries by potentially
+  adding more results. Flow through functions now takes nested field reads/writes into account.
+  For example, the library is able to track flow from `taint()` to `sink()` via the method
+  `getf2f1()` in
+  ```c
+  struct C {
+      int f1;
+  };
+
+  struct C2
+  {
+      C f2;
+
+      int getf2f1() {
+          return f2.f1; // Nested field read
+      }
+
+      void m() {
+          f2.f1 = taint();
+          sink(getf2f1()); // NEW: taint() reaches here
+      }
+  };
+  ```

change-notes/1.25/analysis-csharp.md

@@ -24,5 +24,28 @@ The following changes in version 1.25 affect C# analysis in all applications.
   have type parameters. This means that non-generic nested types inside construced types,
   such as `A<int>.B`, no longer are considered unbound generics. (Such nested types do,
   however, still have relevant `.getSourceDeclaration()`s, for example `A<>.B`.)
+* The data-flow library has been improved, which affects most security queries by potentially
+  adding more results. Flow through methods now takes nested field reads/writes into account.
+  For example, the library is able to track flow from `"taint"` to `Sink()` via the method
+  `GetF2F1()` in
+  ```csharp
+  class C1
+  {
+      string F1;
+  }
+
+  class C2
+  {
+      C1 F2;
+  
+      string GetF2F1() => F2.F1; // Nested field read
+
+      void M()
+      {
+          F2 = new C1() { F1 = "taint" };
+          Sink(GetF2F1()); // NEW: "taint" reaches here
+      }
+  }
+  ```
 
 ## Changes to autobuilder

change-notes/1.25/analysis-java.md

@@ -0,0 +1,41 @@
+# Improvements to Java analysis
+
+The following changes in version 1.25 affect Java analysis in all applications.
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **Query**                    | **Expected impact**    | **Change**                        |
+|------------------------------|------------------------|-----------------------------------|
+
+
+## Changes to libraries
+
+* The data-flow library has been improved, which affects most security queries by potentially
+  adding more results. Flow through methods now takes nested field reads/writes into account.
+  For example, the library is able to track flow from `"taint"` to `sink()` via the method
+  `getF2F1()` in
+  ```java
+  class C1 {
+    String f1;
+    C1(String f1) { this.f1 = f1; }
+  }
+
+  class C2 {
+    C1 f2;
+    String getF2F1() {
+        return this.f2.f1; // Nested field read
+    }
+    void m() {
+        this.f2 = new C1("taint");
+        sink(this.getF2F1()); // NEW: "taint" reaches here
+    }
+  }
+  ```

change-notes/1.25/analysis-javascript.md

@@ -6,13 +6,16 @@
   - [fstream](https://www.npmjs.com/package/fstream)
   - [jGrowl](https://github.com/stanlemon/jGrowl)
   - [jQuery](https://jquery.com/)
+  - [marsdb](https://www.npmjs.com/package/marsdb)
+  - [minimongo](https://www.npmjs.com/package/minimongo/)
 
 ## New queries
 
 | **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
 |---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
 | Cross-site scripting through DOM (`js/xss-through-dom`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities where existing text from the DOM is used as HTML. Results are not shown on LGTM by default. |
 | Incomplete HTML attribute sanitization (`js/incomplete-html-attribute-sanitization`) | security, external/cwe/cwe-20, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities due to incomplete sanitization of HTML meta-characters. Results are shown on LGTM by default. |
+| Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
 
 ## Changes to existing queries
 
@@ -23,7 +26,10 @@
 | Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional command execution calls. |
 | Expression has no effect (`js/useless-expression`) | Less results | This query no longer flags an expression when that expression is the only content of the containing file. |
 | Unknown directive (`js/unknown-directive`) | Less results | This query no longer flags directives generated by the Babel compiler. |
+| Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving NoSQL code operators are now recognized. |
+| Zip Slip (`js/zipslip`) | More results | This query now recognizes zip-slip vulnerabilities involving links. |
 
 ## Changes to libraries
 
+* A library `semmle.javascript.explore.CallGraph` has been added to help write queries for exploring the call graph.
 * Added data flow for `Map` and `Set`, and added matching type-tracking steps that can accessed using the `CollectionsTypeTracking` module.

cpp/ql/src/experimental/semmle/code/cpp/rangeanalysis/InBoundsPointerDeref.qll

@@ -0,0 +1,105 @@
+/**
+ * This library proves that a subset of pointer dereferences in a program are
+ * safe, i.e. in-bounds.
+ * It does so by first defining what a pointer dereference is (on the IR
+ * `Instruction` level), and then using the array length analysis and the range
+ * analysis together to prove that some of these pointer dereferences are safe.
+ *
+ * The analysis is soundy, i.e. it is sound if no undefined behaviour is present
+ * in the program.
+ * Furthermore, it crucially depends on the soundiness of the range analysis and
+ * the array length analysis.
+ */
+
+import cpp
+private import experimental.semmle.code.cpp.rangeanalysis.ArrayLengthAnalysis
+private import semmle.code.cpp.rangeanalysis.RangeAnalysis
+
+/**
+ * Gets the instruction that computes the address of memory that `i` accesses.
+ * Only holds if `i` dereferences a pointer, not when the computation of the
+ * memory address is constant, or if the address of a local variable is loaded/stored to.
+ */
+private Instruction getMemoryAddressInstruction(Instruction i) {
+  (
+    result = i.(FieldAddressInstruction).getObjectAddress() or
+    result = i.(LoadInstruction).getSourceAddress() or
+    result = i.(StoreInstruction).getDestinationAddress()
+  ) and
+  not result instanceof FieldAddressInstruction and
+  not result instanceof VariableAddressInstruction and
+  not result instanceof ConstantValueInstruction
+}
+
+/**
+ * All instructions that dereference a pointer.
+ */
+class PointerDereferenceInstruction extends Instruction {
+  PointerDereferenceInstruction() { exists(getMemoryAddressInstruction(this)) }
+
+  Instruction getAddress() { result = getMemoryAddressInstruction(this) }
+}
+
+/**
+ * Holds if `ptrDeref` can be proven to always access allocated memory.
+ */
+predicate inBounds(PointerDereferenceInstruction ptrDeref) {
+  exists(Length length, int lengthDelta, Offset offset, int offsetDelta |
+    knownArrayLength(ptrDeref.getAddress(), length, lengthDelta, offset, offsetDelta) and
+    // lower bound - note that we treat a pointer that accesses an array of
+    // length 0 as on upper-bound violation, but not as a lower-bound violation
+    (
+      offset instanceof ZeroOffset and
+      offsetDelta >= 0
+      or
+      offset instanceof OpOffset and
+      exists(int lowerBoundDelta |
+        boundedOperand(offset.(OpOffset).getOperand(), any(ZeroBound b), lowerBoundDelta,
+          /*upper*/ false, _) and
+        lowerBoundDelta + offsetDelta >= 0
+      )
+    ) and
+    // upper bound
+    (
+      // both offset and length are only integers
+      length instanceof ZeroLength and
+      offset instanceof ZeroOffset and
+      offsetDelta < lengthDelta
+      or
+      exists(int lengthBound |
+        // array length is variable+integer, and there's a fixed (integer-only)
+        // lower bound on the variable, so we can guarantee this access is always in-bounds
+        length instanceof VNLength and
+        offset instanceof ZeroOffset and
+        boundedInstruction(length.(VNLength).getInstruction(), any(ZeroBound b), lengthBound,
+          /* upper*/ false, _) and
+        offsetDelta < lengthBound + lengthDelta
+      )
+      or
+      exists(int offsetBoundDelta |
+        length instanceof ZeroLength and
+        offset instanceof OpOffset and
+        boundedOperand(offset.(OpOffset).getOperand(), any(ZeroBound b), offsetBoundDelta,
+          /* upper */ true, _) and
+        // offset <= offsetBoundDelta, so offset + offsetDelta <= offsetDelta + offsetBoundDelta
+        // Thus, in-bounds if offsetDelta + offsetBoundDelta < lengthDelta
+        // as we have length instanceof ZeroLength
+        offsetDelta + offsetBoundDelta < lengthDelta
+      )
+      or
+      exists(ValueNumberBound b, int offsetBoundDelta |
+        length instanceof VNLength and
+        offset instanceof OpOffset and
+        b.getValueNumber() = length.(VNLength).getValueNumber() and
+        // It holds that offset <= length + offsetBoundDelta
+        boundedOperand(offset.(OpOffset).getOperand(), b, offsetBoundDelta, /*upper*/ true, _) and
+        // it also holds that
+        offsetDelta < lengthDelta - offsetBoundDelta
+        // taking both inequalities together we get
+        //    offset <= length + offsetBoundDelta
+        // => offset + offsetDelta <= length + offsetBoundDelta + offsetDelta < length + offsetBoundDelta + lengthDelta - offsetBoundDelta
+        // as required
+      )
+    )
+  )
+}

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

@@ -184,17 +184,8 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
    * For example: for a function `int Foo(int p1, int p2)` this would
    * return `int p1, int p2`.
    */
-  string getParameterString() { result = getParameterStringFrom(0) }
-
-  private string getParameterStringFrom(int index) {
-    index = getNumberOfParameters() and
-    result = ""
-    or
-    index = getNumberOfParameters() - 1 and
-    result = getParameter(index).getTypedName()
-    or
-    index < getNumberOfParameters() - 1 and
-    result = getParameter(index).getTypedName() + ", " + getParameterStringFrom(index + 1)
+  string getParameterString() {
+    result = concat(int i | | min(getParameter(i).getTypedName()), ", " order by i)
   }
 
   /** Gets a call to this function. */
@@ -616,18 +607,8 @@ class FunctionDeclarationEntry extends DeclarationEntry, @fun_decl {
    * For example: for a function 'int Foo(int p1, int p2)' this would
    * return 'int p1, int p2'.
    */
-  string getParameterString() { result = getParameterStringFrom(0) }
-
-  private string getParameterStringFrom(int index) {
-    index = getNumberOfParameters() and
-    result = ""
-    or
-    index = getNumberOfParameters() - 1 and
-    result = getParameterDeclarationEntry(index).getTypedName()
-    or
-    index < getNumberOfParameters() - 1 and
-    result =
-      getParameterDeclarationEntry(index).getTypedName() + ", " + getParameterStringFrom(index + 1)
+  string getParameterString() {
+    result = concat(int i | | min(getParameterDeclarationEntry(i).getTypedName()), ", " order by i)
   }
 
   /**

cpp/ql/src/semmle/code/cpp/exprs/Access.qll

@@ -6,9 +6,13 @@ private import semmle.code.cpp.dataflow.EscapesTree
 /**
  * A C/C++ access expression. This refers to a function, variable, or enum constant.
  */
-abstract class Access extends Expr, NameQualifiableElement {
+class Access extends Expr, NameQualifiableElement, @access {
+  // As `@access` is a union type containing `@routineexpr` (which describes function accesses
+  // that are called), we need to exclude function calls.
+  Access() { this instanceof @routineexpr implies not iscall(underlyingElement(this), _) }
+
   /** Gets the accessed function, variable, or enum constant. */
-  abstract Declaration getTarget();
+  Declaration getTarget() { none() } // overridden in subclasses
 
   override predicate mayBeImpure() { none() }
 

cpp/ql/src/semmle/code/cpp/exprs/Cast.qll

@@ -7,7 +7,7 @@ private import semmle.code.cpp.internal.ResolveClass
  * Instances of this class are not present in the main AST which is navigated by parent/child links. Instead,
  * instances of this class are attached to nodes in the main AST via special conversion links.
  */
-abstract class Conversion extends Expr {
+class Conversion extends Expr, @conversion {
   /** Gets the expression being converted. */
   Expr getExpr() { result.getConversion() = this }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/Opcode.qll

@@ -61,7 +61,6 @@ private newtype TOpcode =
   TReThrow() or
   TUnwind() or
   TUnmodeledDefinition() or
-  TUnmodeledUse() or
   TAliasedDefinition() or
   TInitializeNonLocal() or
   TAliasedUse() or
@@ -587,14 +586,6 @@ module Opcode {
     }
   }
 
-  class UnmodeledUse extends Opcode, TUnmodeledUse {
-    final override string toString() { result = "UnmodeledUse" }
-
-    final override predicate hasOperandInternal(OperandTag tag) {
-      tag instanceof UnmodeledUseOperandTag
-    }
-  }
-
   class AliasedDefinition extends Opcode, TAliasedDefinition {
     final override string toString() { result = "AliasedDefinition" }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll

@@ -31,10 +31,14 @@ class IRBlockBase extends TIRBlock {
       config.shouldEvaluateDebugStringsForFunction(this.getEnclosingFunction())
     ) and
     this =
-      rank[result + 1](IRBlock funcBlock |
-        funcBlock.getEnclosingFunction() = getEnclosingFunction()
+      rank[result + 1](IRBlock funcBlock, int sortOverride |
+        funcBlock.getEnclosingFunction() = getEnclosingFunction() and
+        // Ensure that the block containing `EnterFunction` always comes first.
+        if funcBlock.getFirstInstruction() instanceof EnterFunctionInstruction
+        then sortOverride = 0
+        else sortOverride = 1
       |
-        funcBlock order by funcBlock.getUniqueId()
+        funcBlock order by sortOverride, funcBlock.getUniqueId()
       )
   }