Merge branch 'master' into python-keyword-only-args

Author: RasmusWL

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

@@ -3,15 +3,20 @@
 ## General improvements
 
 * Support for the following frameworks and libraries has been improved:
+  - [express](https://www.npmjs.com/package/express)
+  - [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
 
@@ -20,8 +25,16 @@
 | 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. |
+| Client-side URL redirect (`js/client-side-unvalidated-url-redirection`) | Less results | This query now recognizes additional safe patterns of doing URL redirects. |
+| Client-side cross-site scripting (`js/xss`) | Less results | This query now recognizes additional safe strings based on URLs. |
+| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes additional url scheme checks. |
+| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
 | 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 additional vulnerabilities. |
 
 ## 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.

config/identical-files.json

@@ -111,12 +111,12 @@
     "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IR.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IR.qll"
   ],
-  "IR IRSanity": [
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRSanity.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRSanity.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRSanity.qll",
-    "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRSanity.qll",
-    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRSanity.qll"
+  "IR IRConsistency": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRConsistency.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRConsistency.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRConsistency.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/IRConsistency.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/IRConsistency.qll"
   ],
   "IR PrintIR": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/PrintIR.qll",
@@ -157,10 +157,10 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/Opcode.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/Opcode.qll"
   ],
-  "IR SSASanity": [
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSASanity.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSASanity.qll",
-    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSASanity.qll"
+  "IR SSAConsistency": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConsistency.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConsistency.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SSAConsistency.qll"
   ],
   "C++ IR InstructionImports": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/InstructionImports.qll",

cpp/ql/src/Documentation/CaptionedComments.qll

@@ -4,6 +4,10 @@
 
 import cpp
 
+/**
+ * Gets a string representation of the comment `c` containing the caption 'TODO' or 'FIXME'.
+ * If `c` spans multiple lines, all lines after the first are abbreviated as [...].
+ */
 string getCommentTextCaptioned(Comment c, string caption) {
   (caption = "TODO" or caption = "FIXME") and
   exists(

cpp/ql/src/Documentation/CommentedOutCode.qll

@@ -1,3 +1,7 @@
+/**
+ * Provides classes and predicates for identifying C/C++ comments that look like code.
+ */
+
 import cpp
 
 /**
@@ -137,30 +141,52 @@ class CommentBlock extends Comment {
     )
   }
 
+  /**
+   * Gets the last comment associated with this comment block.
+   */
   Comment lastComment() { result = this.getComment(max(int i | exists(this.getComment(i)))) }
 
+  /**
+   * Gets the contents of the `i`'th comment associated with this comment block.
+   */
   string getLine(int i) {
     this instanceof CStyleComment and
     result = this.getContents().regexpCapture("(?s)/\\*+(.*)\\*+/", 1).splitAt("\n", i)
     or
     this instanceof CppStyleComment and result = this.getComment(i).getContents().suffix(2)
   }
 
+  /**
+   * Gets the number of lines in the comments associated with this comment block.
+   */
   int numLines() {
     result = strictcount(int i, string line | line = this.getLine(i) and line.trim() != "")
   }
 
+  /**
+   * Gets the number of lines that look like code in the comments associated with this comment block.
+   */
   int numCodeLines() {
     result = strictcount(int i, string line | line = this.getLine(i) and looksLikeCode(line))
   }
 
+  /**
+   * Holds if the comment block is a C-style comment, and each
+   * comment line starts with a *.
+   */
   predicate isDocumentation() {
     // If a C-style comment starts each line with a *, then it's
     // probably documentation rather than code.
     this instanceof CStyleComment and
     forex(int i | i in [1 .. this.numLines() - 1] | this.getLine(i).trim().matches("*%"))
   }
 
+  /**
+   * Holds if this comment block looks like code that has been commented out. Specifically:
+   * 1. It does not look like documentation (see `isDocumentation`).
+   * 2. It is not in a header file without any declaration entries or top level declarations.
+   * 3. More than half of the lines in the comment block look like code.
+   */
   predicate isCommentedOutCode() {
     not this.isDocumentation() and
     not this.getFile().(HeaderFile).noTopLevelCode() and

cpp/ql/src/codeql-suites/cpp-lgtm-full.qls

@@ -12,3 +12,8 @@
       - Critical/FileNeverClosed.ql
       - Critical/MemoryMayNotBeFreed.ql
       - Critical/MemoryNeverFreed.ql
+# These are only for IDE use.
+- exclude:
+    tags contain:
+      - ide-contextual-queries/local-definitions
+      - ide-contextual-queries/local-references

cpp/ql/src/definitions.qll

@@ -132,6 +132,7 @@ private predicate constructorCallTypeMention(ConstructorCall cc, TypeMention tm)
  *  - `"X"` for macro accesses
  *  - `"I"` for import / include directives
  */
+cached
 Top definitionOf(Top e, string kind) {
   (
     // call -> function called
@@ -213,3 +214,11 @@ Top definitionOf(Top e, string kind) {
   // later on.
   strictcount(result.getLocation()) < 10
 }
+
+/**
+ * Returns an appropriately encoded version of a filename `name`
+ * passed by the VS Code extension in order to coincide with the
+ * output of `.getFile()` on locatable entities.
+ */
+cached
+File getEncodedFile(string name) { result.getAbsolutePath().replaceAll(":", "_") = name }

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