Merge branch 'master' into rdmarsh/cpp/malloc-alias-locations

Author: Robert Marsh

.codeqlmanifest.json

@@ -2,5 +2,4 @@
                "*/ql/test/qlpack.yml",
                "*/upgrades/qlpack.yml",
                "misc/legacy-support/*/qlpack.yml",
-               "misc/suite-helpers/qlpack.yml",
-               "codeql/.codeqlmanifest.json" ] }
+               "misc/suite-helpers/qlpack.yml" ] }

.gitignore

@@ -1,6 +1,7 @@
 # editor and OS artifacts
 *~
 .DS_STORE
+*.swp
 
 # query compilation caches
 .cache

change-notes/1.24/analysis-cpp.md

@@ -19,17 +19,18 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Memory is never freed (`cpp/memory-never-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Missing return statement (`cpp/missing-return`) | Fewer false positive results | Functions containing `asm` statements are no longer highlighted by this query. |
+| No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
 | 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. |
 | 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. |
 
 ## Changes to libraries
 
-* The data-flow library has been improved when flow through functions needs to be
-  combined with both taint tracking and flow through fields allowing more flow
-  to be tracked. This affects and improves some security queries, which may
-  report additional results.
+* The data-flow library has been improved, which affects and improves some security queries. The improvements are:
+  - Track flow through functions that combine taint tracking with flow through fields.
+  - Track flow through clone-like functions, that is, functions that read contents of a field from a
+    parameter and stores the value in the field of a returned object.
 * Created the `semmle.code.cpp.models.interfaces.Allocation` library to model allocation such as `new` expressions and calls to `malloc`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
 * Created the `semmle.code.cpp.models.interfaces.Deallocation` library to model deallocation such as `delete` expressions and calls to `free`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
 * The new class `StackVariable` should be used in place of `LocalScopeVariable`

change-notes/1.24/analysis-csharp.md

@@ -27,16 +27,19 @@ The following changes in version 1.24 affect C# analysis in all applications.
 ## Changes to code extraction
 
 * Tuple expressions, for example `(int,bool)` in `default((int,bool))` are now extracted correctly.
-* Expression nullability flow state is extracted. 
+* Expression nullability flow state is extracted.
+* Implicitly typed `stackalloc` expressions are now extracted correctly.
+* The difference between `stackalloc` array creations and normal array creations is extracted.
 
 ## Changes to libraries
 
-* The data-flow library has been improved when flow through methods needs to be
-  combined with both taint tracking and flow through fields allowing more flow
-  to be tracked. This affects and improves most security queries, which may
-  report additional results.
+* The data-flow library has been improved, which affects and improves most security queries. The improvements are:
+  - Track flow through methods that combine taint tracking with flow through fields.
+  - Track flow through clone-like methods, that is, methods that read contents of a field from a
+    parameter and stores the value in the field of a returned object.
 * The taint tracking library now tracks flow through (implicit or explicit) conversion operator calls.
 * [Code contracts](https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts) are now recognized, and are treated like any other assertion methods.
 * Expression nullability flow state is given by the predicates `Expr.hasNotNullFlowState()` and `Expr.hasMaybeNullFlowState()`.
+* `stackalloc` array creations are now represented by the QL class `Stackalloc`. Previously they were represented by the class `ArrayCreation`.
 
 ## Changes to autobuilder

change-notes/1.24/analysis-java.md

@@ -26,10 +26,10 @@ The following changes in version 1.24 affect Java analysis in all applications.
 
 ## Changes to libraries
 
-* The data-flow library has been improved when flow through methods needs to be
-  combined with both taint tracking and flow through fields allowing more flow
-  to be tracked. This affects and improves most security queries, which may
-  report additional results.
+* The data-flow library has been improved, which affects and improves most security queries. The improvements are:
+  - Track flow through methods that combine taint tracking with flow through fields.
+  - Track flow through clone-like methods, that is, methods that read contents of a field from a
+    parameter and stores the value in the field of a returned object.
 * Identification of test classes has been improved. Previously, one of the
   match conditions would classify any class with a name containing the string
   "Test" as a test class, but now this matching has been replaced with one that

change-notes/1.24/analysis-javascript.md

@@ -9,21 +9,26 @@
 
 * Imports that rely on path-mappings from a `tsconfig.json` file can now be resolved.
 
+* Export declarations of the form `export * as ns from "x"` are now analyzed more precisely.
+
 * The analysis of sanitizer guards has improved, leading to fewer false-positive results from the security queries.
 
 * Support for the following frameworks and libraries has been improved:
-  - [react](https://www.npmjs.com/package/react)
-  - [typeahead.js](https://www.npmjs.com/package/typeahead.js)
-  - [Handlebars](https://www.npmjs.com/package/handlebars)
   - [Electron](https://electronjs.org/)
+  - [Handlebars](https://www.npmjs.com/package/handlebars)
+  - [Koa](https://www.npmjs.com/package/koa)
   - [Node.js](https://nodejs.org/)
   - [Socket.IO](https://socket.io/)
-  - [ws](https://github.com/websockets/ws)
   - [WebSocket](https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API)
-  - [Koa](https://www.npmjs.com/package/koa)
-  - [lazy-cache](https://www.npmjs.com/package/lazy-cache)
+  - [chrome-remote-interface](https://www.npmjs.com/package/chrome-remote-interface)
   - [for-in](https://www.npmjs.com/package/for-in)
   - [for-own](https://www.npmjs.com/package/for-own)
+  - [http2](https://nodejs.org/api/http2.html)
+  - [lazy-cache](https://www.npmjs.com/package/lazy-cache)
+  - [react](https://www.npmjs.com/package/react)
+  - [send](https://www.npmjs.com/package/send)
+  - [typeahead.js](https://www.npmjs.com/package/typeahead.js)
+  - [ws](https://github.com/websockets/ws)
 
 ## New queries
 
@@ -33,6 +38,7 @@
 | Regular expression always matches (`js/regex/always-matches`) | correctness, regular-expressions | Highlights regular expression checks that trivially succeed by matching an empty substring. Results are shown on LGTM by default. |
 | Missing await (`js/missing-await`) | correctness | Highlights expressions that operate directly on a promise object in a nonsensical way, instead of awaiting its result. Results are shown on LGTM by default. |
 | Prototype pollution in utility function (`js/prototype-pollution-utility`) | security, external/cwe/cwe-400, external/cwe/cwe-471 | Highlights recursive copying operations that are susceptible to prototype pollution. Results are shown on LGTM by default. |
+| Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | Highlights potential XSS vulnerabilities in unsafely designed jQuery plugins. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
 
@@ -46,6 +52,7 @@
 | Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. |
 | Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. |
 | Uncontrolled data used in path expression (`js/path-injection`) | More results | This query now recognizes additional ways dangerous paths can be constructed. |
+| Uncontrolled command line (`js/command-line-injection`) | More results | This query now recognizes additional ways of constructing arguments to `cmd.exe` and `/bin/sh`. |
 
 ## Changes to libraries
 

change-notes/1.24/analysis-python.md

@@ -0,0 +1,37 @@
+# Improvements to Python analysis
+
+The following changes in version 1.24 affect Python analysis in all applications.
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+
+### Web framework support
+
+The QL-library support for the web frameworks Bottle, CherryPy, Falcon, Pyramid, TurboGears, Tornado, and Twisted have
+been fixed so they provide a proper HttpRequestTaintSource, instead of a TaintSource. This will enable results for the following queries:
+
+- py/path-injection
+- py/command-line-injection
+- py/reflective-xss
+- py/sql-injection
+- py/code-injection
+- py/unsafe-deserialization
+- py/url-redirection
+
+The QL-library support for the web framework Twisted have been fixed so they provide a proper
+HttpResponseTaintSink, instead of a TaintSink. This will enable results for the following
+queries:
+
+- py/reflective-xss
+- py/stack-trace-exposure
+
+## Changes to libraries

config/identical-files.json

@@ -222,10 +222,12 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/PrintSSA.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/PrintSSA.qll"
   ],
-  "C++ IR ValueNumberInternal": [
+  "IR ValueNumberInternal": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingInternal.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingInternal.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingInternal.qll"
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/internal/ValueNumberingInternal.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/internal/ValueNumberingInternal.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingInternal.qll"
   ],
   "C++ IR ValueNumber": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll",

cpp/ql/src/Security/CWE/CWE-131/NoSpaceForZeroTerminator.ql

@@ -22,16 +22,25 @@ import semmle.code.cpp.models.interfaces.Allocation
 predicate terminationProblem(AllocationExpr malloc, string msg) {
   // malloc(strlen(...))
   exists(StrlenCall strlen | DataFlow::localExprFlow(strlen, malloc.getSizeExpr())) and
-  // flows into a null-terminated string function
+  // flows to a call that implies this is a null-terminated string
   exists(ArrayFunction af, FunctionCall fc, int arg |
     DataFlow::localExprFlow(malloc, fc.getArgument(arg)) and
     fc.getTarget() = af and
     (
-      // null terminated string
+      // flows into null terminated string argument
       af.hasArrayWithNullTerminator(arg)
       or
-      // likely a null terminated string (such as `strcpy`, `strcat`)
+      // flows into likely null terminated string argument (such as `strcpy`, `strcat`)
       af.hasArrayWithUnknownSize(arg)
+      or
+      // flows into string argument to a formatting function (such as `printf`)
+      exists(int n, FormatLiteral fl |
+        fc.getArgument(arg) = fc.(FormattingFunctionCall).getConversionArgument(n) and
+        fl = fc.(FormattingFunctionCall).getFormat() and
+        fl.getConversionType(n) instanceof PointerType and // `%s`, `%ws` etc
+        not fl.getConversionType(n) instanceof VoidPointerType and // exclude: `%p`
+        not fl.hasPrecision(n) // exclude: `%.*s`
+      )
     )
   ) and
   msg = "This allocation does not include space to null-terminate the string."

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

@@ -139,6 +139,13 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
   /** Gets a parameter of this function. */
   Parameter getAParameter() { params(unresolveElement(result), underlyingElement(this), _, _) }
 
+  /**
+   * Gets an access of this function.
+   *
+   * To get calls to this function, use `getACallToThisFunction` instead.
+   */
+  FunctionAccess getAnAccess() { result.getTarget() = this }
+
   /**
    * Gets the number of parameters of this function, _not_ including any
    * implicit `this` parameter or any `...` varargs pseudo-parameter.
@@ -174,6 +181,7 @@ class Function extends Declaration, ControlFlowNode, AccessHolder, @function {
     result = getParameter(index).getTypedName() + ", " + getParameterStringFrom(index + 1)
   }
 
+  /** Gets a call to this function. */
   FunctionCall getACallToThisFunction() { result.getTarget() = this }
 
   /**