Merge rc/1.19 into master

.lgtm.yml

@@ -10,6 +10,8 @@ path_classifiers:
   - javascript/extractor/tests
   - javascript/ql/src
   - javascript/ql/test
+  - python/ql/src
+  - python/ql/test
 
 queries:
   - include: "*"

change-notes/1.19/analysis-cpp.md

@@ -6,8 +6,8 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Enabled by default. |
-| Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | external/cwe/cwe-732 | This query finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Enabled by default. |
+| Cast between `HRESULT` and a Boolean type (`cpp/hresult-boolean-conversion`) | security, external/cwe/cwe-253 | Finds logic errors caused by mistakenly treating the Windows `HRESULT` type as a Boolean instead of testing it with the appropriate macros. Enabled by default. |
+| Setting a DACL to `NULL` in a `SECURITY_DESCRIPTOR` (`cpp/unsafe-dacl-security-descriptor`) | security, external/cwe/cwe-732 | This query finds code that creates world-writable objects on Windows by setting their DACL to `NULL`. Enabled by default. |
 | Cast from `char*` to `wchar_t*` | security, external/cwe/cwe-704 | Detects potentially dangerous casts from `char*` to `wchar_t*`.  Enabled by default on LGTM. |
 | Dead code due to `goto` or `break` statement (`cpp/dead-code-goto`) | maintainability, external/cwe/cwe-561 | Detects dead code following a `goto` or `break` statement. Enabled by default on LGTM. |
 | Inconsistent direction of for loop | correctness, external/cwe/cwe-835 | This query detects `for` loops where the increment and guard condition don't appear to correspond.  Enabled by default on LGTM. |

change-notes/1.19/analysis-csharp.md

@@ -11,7 +11,7 @@
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
 | Using a package with a known vulnerability (cs/use-of-vulnerable-package) | security, external/cwe/cwe-937 | Finds project build files that import packages with known vulnerabilities. This is included by default. |
-
+| Uncontrolled format string (cs/uncontrolled-format-string) | security, external/cwe/cwe-134 | Finds data flow from remote inputs to the format string in `String.Format`. This is included by default. |
 
 ## Changes to existing queries
 

change-notes/1.19/analysis-javascript.md

@@ -28,6 +28,8 @@
 | Stored cross-site scripting (`js/stored-xss`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights uncontrolled stored values flowing into HTML content, indicating a violation of [CWE-079](https://cwe.mitre.org/data/definitions/79.html). Results shown on LGTM by default. |
 | Unclear precedence of nested operators (`js/unclear-operator-precedence`) | maintainability, correctness, external/cwe/cwe-783 | Highlights nested binary operators whose relative precedence is easy to misunderstand. Results shown on LGTM by default. |
 | Unneeded defensive code | correctness, external/cwe/cwe-570, external/cwe/cwe-571 | Highlights locations where defensive code is not needed. Results are shown on LGTM by default. |
+| Unsafe dynamic method access (`js/unsafe-dynamic-method-access` ) | security, external/cwe/cwe-094 | Highlights code that invokes a user-controlled method on an object with unsafe methods. Results are shown on LGTM by default. |
+| Unvalidated dynamic method access (`js/unvalidated-dynamic-method-call` ) | security, external/cwe/cwe-754 | Highlights code that invokes a user-controlled method without guarding against exceptional circumstances. Results are shown on LGTM by default. |
 | Useless assignment to property | maintainability | Highlights property assignments whose value is always overwritten. Results are shown on LGTM by default. |
 | User-controlled data in file | security, external/cwe/cwe-912 | Highlights locations where user-controlled data is written to a file. Results are not shown on LGTM by default. |
 
@@ -43,11 +45,11 @@
 | Duplicate 'if' condition | Lower severity | The severity of this rule has been revised to "warning". |
 | Duplicate switch case | Lower severity | The severity of this rule has been revised to "warning". |
 | Information exposure through a stack trace | More results | This rule now also flags cases where the entire exception object (including the stack trace) may be exposed. |
-| Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
 | Missing 'this' qualifier | Fewer false-positive results | This rule now recognizes additional intentional calls to global functions. | 
+| Missing CSRF middleware | Fewer false-positive results | This rule now recognizes additional CSRF protection middlewares. |
 | Missing variable declaration | Lower severity | The severity of this rule has been revised to "warning". |
 | Regular expression injection | Fewer false-positive results | This rule now identifies calls to `String.prototype.search` with more precision. |
-| Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Results are no longer shown on LGTM by default. |
+| Remote property injection | Fewer results | The precision of this rule has been revised to "medium". Furthermore, it no longer flags dynamic method calls, which are now handled by two new queries. Results are no longer shown on LGTM by default. |
 | Self assignment | Fewer false-positive results | This rule now ignores self-assignments preceded by a JSDoc comment with a `@type` tag. |
 | Server-side URL redirect | Fewer false-positive results | This rule now recognizes safe redirects in more cases. |
 | Server-side URL redirect | More results | This rule now recognizes redirection calls in more cases. |
@@ -58,6 +60,7 @@
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that may be used by `eval` calls. |
 | Unused variable, import, function or class | Fewer results | This rule now flags import statements with multiple unused imports once. |
 | Useless assignment to local variable | Fewer false-positive results | This rule now recognizes additional ways default values can be set. |
+| Useless conditional | More results, fewer false-positive results | This rule now recognizes conditionals in more cases, but no longer flags certain defensive coding patterns. |
 | Whitespace contradicts operator precedence | Fewer false-positive results | This rule no longer flags operators with asymmetric whitespace. |
 | Wrong use of 'this' for static method | More results, fewer false-positive results | This rule now recognizes inherited methods. |
 

change-notes/1.19/analysis-python.md

@@ -0,0 +1,97 @@
+# Improvements to Python analysis
+
+
+## General improvements
+
+> Changes that affect alerts in many files or from many queries
+> For example, changes to file classification
+
+### Representation of the control flow graph
+
+The representation of the control flow graph (CFG) has been modified to better reflect the semantics of Python.
+
+The following statement types no longer have a CFG node for the statement itself, as their sub-expressions already contain all the
+semantically significant information:
+
+* `ExprStmt`
+* `If`
+* `Assign`
+* `Import`
+
+For example, the CFG for `if cond: foo else bar` now starts with the CFG node for `cond`.
+
+For the following statement types, the CFG node for the statement now follows the CFG nodes of its sub-expressions to better reflect the semantics:
+
+* `Print`
+* `TemplateWrite`
+* `ImportStar`
+
+For example the CFG for `print foo` (in Python 2) has changed from `print -> foo` to `foo -> print`, better reflecting the runtime behavior.
+
+
+The CFG for the `with` statement has been re-ordered to more closely reflect the semantics.
+For the `with` statement:
+```python
+with cm as var:
+    body
+```
+The order of the CFG changes from:
+
+    <with>
+    cm
+    var
+    body
+
+to:
+
+    cm
+    <with>
+    var
+    body
+
+A new predicate `Stmt.getAnEntryNode()` has been added to make it easier to write reachability queries involving statements.
+
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+| Flask app is run in debug mode (`py/flask-debug`) | security, external/cwe/cwe-215, external/cwe/cwe-489 | Finds instances where a Flask application is run in debug mode. Enabled on LGTM by default. |
+| Information exposure through an exception (`py/stack-trace-exposure`) | security, external/cwe/cwe-209, external/cwe/cwe-497 | Finds instances where information about an exception may be leaked to an external user. Enabled on LGTM by default. |
+| Request without certificate validation (`py/request-without-cert-validation`) | security, external/cwe/cwe-295 | Finds requests where certificate verification has been explicitly turned off, possibly allowing man-in-the-middle attacks. Not enabled on LGTM by default. |
+
+## Changes to existing queries
+
+All taint-tracking queries now support visualization of paths in QL for Eclipse.
+Most security alerts are now visible on LGTM by default.
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+| Assert statement tests the truth value of a literal constant (`py/assert-literal-constant`) | reliability, correctness     | Checks whether an assert statement is testing the truth of a literal constant value. Not shown by default. |
+| Code injection (`py/code-injection`) | Supports path visualization and is now visible on LGTM by default | No change to expected results |
+| Command injection (`py/command-line-injection`) | Additional sinks in the `os`, and `popen` modules | Possibility of new results |
+| Deserializing untrusted input (`py/unsafe-deserialization`) | Supports path visualization | No change to expected results |
+| Encoding error (`py/encoding-error`) | Better alert location | Alert is now shown at the position of the first offending character, rather than at the top of the file. |
+| Missing call to \_\_init\_\_ during object initialization (`py/missing-call-to-init`) | Fewer false positive results | Results where it is likely that the full call chain has not been analyzed are no longer reported. |
+| Reflected server-side cross-site scripting (`py/reflective-xss`) | Supports path visualization and is now visible on LGTM by default | No change to expected results |
+| SQL query built from user-controlled sources (`py/sql-injection`) | Supports path visualization and is now visible on LGTM by default | No change to expected results |
+| Uncontrolled data used in path expression (`py/path-injection`) | Supports path visualization and is now visible on LGTM by default | No change to expected results |
+| Uncontrolled command line (`py/command-line-injection`) | Supports path visualization and is now visible on LGTM by default | No change to expected results |
+| URL redirection from remote source (`py/url-redirection`) | Fewer false positive results and now supports path visualization | Taint is no longer tracked from the right hand side of binary expressions. In other words `SAFE + TAINTED` is now treated as safe. |
+
+
+## Changes to code extraction
+
+* Improved scalability: Scaling is near linear to at least 20 CPU cores.
+* Five levels of logging can be selected: `ERROR`, `WARN`, `INFO`, `DEBUG` and `TRACE`. `WARN` is the stand-alone default, but `INFO` will be used when run by LGTM.
+* The `-v` flag can be specified multiple times to increase logging level by one per `-v`.
+* The `-q` flag has been added and can be specified multiple times to reduce the logging level by one per `-q`.
+* Log lines are now in the `[SEVERITY] message` style and never overlap.
+* Extractor now outputs the location of the first offending character when an EncodingError is encountered.
+
+## Changes to QL libraries
+
+* Taint tracking analysis now understands HTTP requests in the `twisted` library.
+
+* The analysis now handles `isinstance` and `issubclass` tests involving the basic abstract base classes better. For example, the test `issubclass(list, collections.Sequence)` is now understood to be `True`
+* Taint tracking automatically tracks tainted mappings and collections, without you having to add additional taint kinds. This means that custom taints are tracked from `x` to `y` in the following flow: `l = [x]; y =l[0]`.

change-notes/1.19/extractor-javascript.md

@@ -32,3 +32,8 @@ extraction:
 * The TypeScript compiler is now bundled with the distribution, and no longer needs to be installed manually.
   Should the compiler version need to be overridden, set the `SEMMLE_TYPESCRIPT_HOME` environment variable to
   point to an installation of the `typescript` NPM package.
+
+* The extractor now supports [Optional Chaining](https://github.com/tc39/proposal-optional-chaining) expressions.
+
+* The extractor now supports additional [Flow](https://flow.org/) syntax.
+

cpp/ql/src/Critical/NotInitialised.ql

@@ -9,9 +9,7 @@
  */
 import cpp
 
-// This query is the JSF version
-//
-// (see also InitialisationNotRun.ql and GlobalUseBeforeInit.ql)
+// See also InitialisationNotRun.ql and GlobalUseBeforeInit.ql
 
 // Holds if s defines variable v (conservative)
 predicate defines(ControlFlowNode s, Variable lv) {

cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql

@@ -33,14 +33,7 @@ abstract class SystemData extends Element {
     result = getAnExpr() or
 
     // flow via global or member variable (conservative approximation)
-    exists(Variable var |
-      (
-        var.getAnAssignedValue() = getAnExprIndirect() or
-        var.getAnAccess() = getAnExprIndirect()
-      ) and
-      result = var.getAnAccess() and
-      not var instanceof LocalScopeVariable
-    ) or
+    result = getAnAffectedVar().getAnAccess() or
 
     // flow via stack variable
     definitionUsePair(_, getAnExprIndirect(), result) or
@@ -50,6 +43,17 @@ abstract class SystemData extends Element {
     // flow from assigned value to assignment expression
     result.(AssignExpr).getRValue() = getAnExprIndirect()
   }
+
+  /** Gets a global or member variable that may be affected by this system
+   * data (conservative approximation).
+   */
+  private Variable getAnAffectedVar() {
+    (
+      result.getAnAssignedValue() = this.getAnExprIndirect() or
+      result.getAnAccess() = this.getAnExprIndirect()
+    ) and
+    not result instanceof LocalScopeVariable
+  }
 }
 
 /**

cpp/ql/src/jsf/4.05 Libraries/AV Rule 24.qhelp

@@ -5,8 +5,12 @@
 
 
 <overview>
+
+<!-- Mention that this rule may not be applicable in projects that don't follow the JSF standard. -->
+<include src="cpp/jsfNote.qhelp" />
+
 <p>
-This rule finds calls to the standard library functions <code>abort, exit, getenv</code> and <code>system</code>.
+This query highlights calls to the standard library functions <code>abort, exit, getenv</code> and <code>system</code>.
 The functions <code>abort</code> and <code>exit</code> should not be called as they immediately terminate the program
 and will bypass all the normal error and exception handling routines in the software. This is especially important in
 software which is run on systems without an interactive OS, as restarting the software may require a complete reboot

cpp/ql/src/jsf/4.10 Classes/AV Rule 85.qhelp

@@ -5,8 +5,12 @@
 
 
 <overview>
+
+<!-- Mention that this rule may not be applicable in projects that don't follow the JSF standard. -->
+<include src="cpp/jsfNote.qhelp" />
+
 <p>
-This rule ensures that all operators with opposites (e.g. == and !=) are both defined, and
+This query ensures that all operators with opposites (e.g. == and !=) are both defined, and
 that one of them is defined in terms of the other. This just enforces the consistency of meaning
 of the operators.
 </p>

cpp/ql/src/jsf/4.10 Classes/AV Rule 85.ql

@@ -21,23 +21,50 @@ predicate oppositeOperators(string op1, string op2) {
 /* this match is very syntactic: we simply check that op1 is defined as
    !op2(_, _) */
 predicate implementedAsNegationOf(Operator op1, Operator op2) {
-  exists(Block b, ReturnStmt r, NotExpr n, FunctionCall c |
+  exists(Block b, ReturnStmt r, NotExpr n, Expr o |
     b = op1.getBlock() and
     b.getNumStmt() = 1 and
     r = b.getStmt(0) and
     n = r.getExpr() and
-    c = n.getOperand() and
-    c.getTarget() = op2)
+    o = n.getOperand() and
+    (
+      o instanceof LTExpr and op2.hasName("operator<") or
+      o instanceof LEExpr and op2.hasName("operator<=") or
+      o instanceof GTExpr and op2.hasName("operator>") or
+      o instanceof GEExpr and op2.hasName("operator>=") or
+      o instanceof EQExpr and op2.hasName("operator==") or
+      o instanceof NEExpr and op2.hasName("operator!=") or
+      o.(FunctionCall).getTarget() = op2
+    )
+  )
+}
+
+predicate classIsCheckableFor(Class c, string op) {
+  oppositeOperators(op, _) and
+  // We check the template, not its instantiations
+  not c instanceof ClassTemplateInstantiation and
+  // Member functions of templates are not necessarily instantiated, so
+  // if the function we want to check exists, then make sure that its
+  // body also exists
+  ((c instanceof TemplateClass)
+   implies
+   forall(Function f | f = c.getAMember() and f.hasName(op)
+                     | exists(f.getEntryPoint())))
 }
 
 from Class c, string op, string opp, Operator rator
 where c.fromSource() and
       oppositeOperators(op, opp) and
+      classIsCheckableFor(c, op) and
+      classIsCheckableFor(c, opp) and
       rator = c.getAMember() and
       rator.hasName(op) and
-      not exists(Operator oprator | oprator = c.getAMember() and
-                                    oprator.hasName(opp) and
-                                    (   implementedAsNegationOf(rator, oprator)
-                                     or implementedAsNegationOf(oprator, rator)))
+      forex(Operator aRator |
+            aRator = c.getAMember() and aRator.hasName(op) |
+            not exists(Operator oprator |
+                       oprator = c.getAMember() and
+                       oprator.hasName(opp) and
+                       (   implementedAsNegationOf(aRator, oprator)
+                        or implementedAsNegationOf(oprator, aRator))))
 select c, "When two operators are opposites, both should be defined and one should be defined in terms of the other. Operator " + op +
           " is declared on line " + rator.getLocation().getStartLine().toString() + ", but it is not defined in terms of its opposite operator " + opp + "."

cpp/ql/src/jsf/4.13 Functions/AV Rule 111.qhelp

@@ -5,8 +5,12 @@
 
 
 <overview>
+
+<!-- Mention that this rule may not be applicable in projects that don't follow the JSF standard. -->
+<include src="cpp/jsfNote.qhelp" />
+
 <p>
-This rule finds return statements that return pointers to an object allocated on the stack. The lifetime
+This query highlights return statements that return pointers to an object allocated on the stack. The lifetime
 of a stack allocated memory location only lasts until the function returns, , and 
 the contents of that memory become undefined after that. Clearly, using a pointer to stack 
 memory after the function has already returned will have undefined results.