Merge remote-tracking branch 'upstream/master' into dataflow-indirect-args

The conflicts came from how `this` is now a parameter but not a
`Parameter` on `master`.

Conflicts:
	cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll
	cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/defaulttainttracking.cpp
	cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/tainted.expected
	cpp/ql/test/library-tests/dataflow/DefaultTaintTracking/test_diff.expected
	cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-ir-consistency.expected
	cpp/ql/test/library-tests/dataflow/fields/ir-flow.expected
	cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected

Author: jbj

.devcontainer/devcontainer.json

@@ -0,0 +1,9 @@
+{
+  "extensions": [
+    "github.vscode-codeql",
+    "slevesque.vscode-zipexplorer"
+  ],
+  "settings": {
+    "codeQL.experimentalBqrsParsing": true
+  }
+}

README.md

@@ -1,6 +1,6 @@
 # CodeQL
 
-This open source repository contains the standard CodeQL libraries and queries that power [LGTM](https://lgtm.com) and the other CodeQL products that [GitHub](https://github.com) makes available to its customers worldwide.
+This open source repository contains the standard CodeQL libraries and queries that power [LGTM](https://lgtm.com) and the other CodeQL products that [GitHub](https://github.com) makes available to its customers worldwide. For the queries, libraries, and extractor that power Go analysis, visit the [CodeQL for Go repository](https://github.com/github/codeql-go).
 
 ## How do I learn CodeQL and run queries?
 

change-notes/1.25/analysis-cpp.md

@@ -0,0 +1,44 @@
+# 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 library `VCS.qll` and all queries that imported it have been removed.
+* 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
+      }
+  };
+  ```
+* The security pack taint tracking library (`semmle.code.cpp.security.TaintTracking`) now considers that equality checks may block the flow of taint.  This results in fewer false positive results from queries that use this library.
+* The length of a tainted string (such as the return value of a call to `strlen` or `strftime` with tainted parameters) is no longer itself considered tainted by the `models` library.  This leads to fewer false positive results in queries that use any of our taint libraries.

change-notes/1.25/analysis-csharp.md

@@ -18,11 +18,37 @@ The following changes in version 1.25 affect C# analysis in all applications.
 
 ## Changes to code extraction
 
+* Index initializers, of the form `{ [1] = "one" }`, are extracted correctly. Previously, the kind of the
+  expression was incorrect, and the index was not extracted.
+
 ## Changes to libraries
 
 * The class `UnboundGeneric` has been refined to only be those declarations that actually
-  have type parameters. This means that non-generic nested types inside construced types,
+  have type parameters. This means that non-generic nested types inside constructed 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,27 +3,84 @@
 ## General improvements
 
 * Support for the following frameworks and libraries has been improved:
+  - [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise)
+  - [bluebird](http://bluebirdjs.com/)
+  - [express](https://www.npmjs.com/package/express)
+  - [fastify](https://www.npmjs.com/package/fastify)
   - [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/)
+  - [mssql](https://www.npmjs.com/package/mssql)
+  - [mysql](https://www.npmjs.com/package/mysql)
+  - [pg](https://www.npmjs.com/package/pg)
+  - [sequelize](https://www.npmjs.com/package/sequelize)
+  - [spanner](https://www.npmjs.com/package/spanner)
+  - [sqlite](https://www.npmjs.com/package/sqlite)
+  - [ssh2-streams](https://www.npmjs.com/package/ssh2-streams)
+  - [ssh2](https://www.npmjs.com/package/ssh2)
+
+* TypeScript 3.9 is now supported.
+
+* The analysis of sanitizers has improved, leading to more accurate
+  results from the security queries.
 
 ## 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. |
+| Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Client-side cross-site scripting (`js/xss`) | Fewer results | This query now recognizes additional safe patterns of constructing HTML. |
+| 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. |
+| 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. |
-| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
+| Prototype pollution in utility function (`js/prototype-pollution-utility`) | More results | This query now recognizes additional utility functions as vulnerable to prototype polution. |
 | 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. |
+| Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional file system calls. |
+| Unknown directive (`js/unknown-directive`) | Fewer results | This query no longer flags directives generated by the Babel compiler. |
+| Unused property (`js/unused-property`) | Fewer results | This query no longer flags properties of objects that are operands of `yield` expressions. |
+| Zip Slip (`js/zipslip`) | More results | This query now recognizes additional vulnerabilities. |
+
+The following low-precision queries are no longer run by default on LGTM (their results already were not displayed):
+
+  - `js/angular/dead-event-listener`
+  - `js/angular/unused-dependency`
+  - `js/bitwise-sign-check`
+  - `js/comparison-of-identical-expressions`
+  - `js/conflicting-html-attribute`
+  - `js/ignored-setter-parameter`
+  - `js/jsdoc/malformed-param-tag`
+  - `js/jsdoc/missing-parameter`
+  - `js/jsdoc/unknown-parameter`
+  - `js/json-in-javascript-file`
+  - `js/misspelled-identifier`
+  - `js/nested-loops-with-same-variable`
+  - `js/node/cyclic-import`
+  - `js/node/unused-npm-dependency`
+  - `js/omitted-array-element`
+  - `js/return-outside-function`
+  - `js/single-run-loop`
+  - `js/too-many-parameters`
+  - `js/unused-property`
+  - `js/useless-assignment-to-global`
 
 ## 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.
+* The data-flow node representing a parameter or destructuring pattern is now always the `ValueNode` corresponding to that AST node. This has a few consequences:
+  - `Parameter.flow()` now gets the correct data flow node for a parameter. Previously this had a result, but the node was disconnected from the data flow graph.
+  - `ParameterNode.asExpr()` and `.getAstNode()` now gets the parameter's AST node, whereas previously it had no result.
+  - `Expr.flow()` now has a more meaningful result for destructuring patterns. Previously this node was disconnected from the data flow graph. Now it represents the values being destructured by the pattern.
+* The global data-flow and taint-tracking libraries now model indirect parameter accesses through the `arguments` object in some cases, which may lead to additional results from some of the security queries, particularly "Prototype pollution in utility function".

cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.qll

@@ -6,29 +6,50 @@
 
 import cpp
 
-// True if function was ()-declared, but not (void)-declared or K&R-defined
+/**
+ * Holds if `fde` has a parameter declaration that's clear on the minimum
+ * number of parameters. This is essentially true for everything except
+ * `()`-declarations.
+ */
+private predicate hasDefiniteNumberOfParameters(FunctionDeclarationEntry fde) {
+  fde.hasVoidParamList()
+  or
+  fde.getNumberOfParameters() > 0
+  or
+  fde.isDefinition()
+}
+
+/* Holds if function was ()-declared, but not (void)-declared or K&R-defined. */
 private predicate hasZeroParamDecl(Function f) {
   exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
-    not fde.hasVoidParamList() and fde.getNumberOfParameters() = 0 and not fde.isDefinition()
+    not hasDefiniteNumberOfParameters(fde)
   )
 }
 
-// True if this file (or header) was compiled as a C file
+/* Holds if this file (or header) was compiled as a C file. */
 private predicate isCompiledAsC(File f) {
   f.compiledAsC()
   or
   exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
 }
 
+/** Holds if `fc` is a call to `f` with too few arguments. */
 predicate tooFewArguments(FunctionCall fc, Function f) {
   f = fc.getTarget() and
   not f.isVarargs() and
   not f instanceof BuiltInFunction and
+  // This query should only have results on C (not C++) functions that have a
+  // `()` parameter list somewhere. If it has results on other functions, then
+  // it's probably because the extractor only saw a partial compilation.
   hasZeroParamDecl(f) and
   isCompiledAsC(f.getFile()) and
-  // There is an explicit declaration of the function whose parameter count is larger
-  // than the number of call arguments
-  exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |
+  // Produce an alert when all declarations that are authoritative on the
+  // parameter count specify a parameter count larger than the number of call
+  // arguments.
+  forex(FunctionDeclarationEntry fde |
+    fde = f.getADeclarationEntry() and
+    hasDefiniteNumberOfParameters(fde)
+  |
     fde.getNumberOfParameters() > fc.getNumberOfArguments()
   )
 }