Merge master into rc/1.20

change-notes/1.20/analysis-cpp.md

@@ -39,3 +39,4 @@
 * There is a new `Namespace.isInline()` predicate, which holds if the namespace was declared as `inline namespace`.
 * The `Expr.isConstant()` predicate now also holds for _address constant expressions_, which are addresses that will be constant after the program has been linked. These address constants do not have a result for `Expr.getValue()`.
 * There are new `Function.isDeclaredConstexpr()` and `Function.isConstexpr()` predicates. They can be used to tell whether a function was declared as `constexpr`, and whether it actually is `constexpr`.
+* There is a new `Variable.isConstexpr()` predicate. It can be used to tell whether a variable is `constexpr`.

change-notes/1.20/analysis-javascript.md

@@ -5,6 +5,7 @@
 * Support for many frameworks and libraries has been improved, in particular including the following:
   - [a-sync-waterfall](https://www.npmjs.com/package/a-sync-waterfall)
   - [Electron](https://electronjs.org)
+  - [Express](https://npmjs.org/express)
   - [hapi](https://hapijs.com/)
   - [js-cookie](https://github.com/js-cookie/js-cookie)
   - [React](https://reactjs.org/)
@@ -30,7 +31,7 @@
 | Incomplete URL substring sanitization | correctness, security, external/cwe/cwe-020 | Highlights URL sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results shown on LGTM by default. |
 | Incorrect suffix check (`js/incorrect-suffix-check`) | correctness, security, external/cwe/cwe-020 | Highlights error-prone suffix checks based on `indexOf`, indicating a potential violation of [CWE-20](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default. |
 | Loop iteration skipped due to shifting (`js/loop-iteration-skipped-due-to-shifting`) | correctness | Highlights code that removes an element from an array while iterating over it, causing the loop to skip over some elements. Results are shown on LGTM by default. |
-| Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | Additional ways that class methods can be bound are recognized. |
+| Unused property (`js/unused-property`) | maintainability | Highlights properties that are unused. Results are shown on LGTM by default. |
 | Useless comparison test (`js/useless-comparison-test`) | correctness | Highlights code that is unreachable due to a numeric comparison that is always true or always false. Results are shown on LGTM by default. |
 
 ## Changes to existing queries
@@ -43,9 +44,10 @@
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
 | Reflected cross-site scripting             | Fewer false-positive results. | This rule now recognizes custom sanitizers. |
 | Stored cross-site scripting                | Fewer false-positive results. | This rule now recognizes custom sanitizers. |
+| Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | Additional ways that class methods can be bound are recognized. |
 | Uncontrolled data used in network request  | More results                 | This rule now recognizes host values that are vulnerable to injection. |
 | Unused parameter                           | Fewer false-positive results | This rule no longer flags parameters with leading underscore. |
-| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
+| Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, no longer flags variables with leading underscore, and no longer flags variables in dead code. |
 | Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. |
 | Unneeded defensive code | More true-positive results, fewer false-positive results. | This rule now recognizes additional defensive code patterns. |
 | Useless conditional | Fewer results | Additional defensive coding patterns are now ignored. |

change-notes/1.20/analysis-python.md

@@ -28,6 +28,7 @@ The API has been improved to declutter the global namespace and improve discover
  | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Comparison using is when operands support \_\_eq\_\_ (`py/comparison-using-is`) | Fewer false positive results | Results where one of the objects being compared is an enum member are no longer reported. |
+| Modification of parameter with default (`py/modification-of-default-value`) | More true positive results | Instances where the mutable default value is mutated inside other functions are now also reported. |
 | Mutation of descriptor in \_\_get\_\_ or \_\_set\_\_ method (`py/mutable-descriptor`) | Fewer false positive results | Results where the mutation does not occur when calling one of the  `__get__`, `__set__` or `__delete__` methods are no longer reported. |
 | Unused import (`py/unused-import`) | Fewer false positive results | Results where the imported module is used in a `doctest` string are no longer reported. |
 | Unused import (`py/unused-import`) | Fewer false positive results | Results where the imported module is used in a type-hint comment are no longer reported. |
@@ -40,6 +41,8 @@ The API has been improved to declutter the global namespace and improve discover
 
  * Added support for the `dill` pickle library.
  * Added support for the `bottle` web framework.
+ * Added support for the `CherryPy` web framework.
+ * Added support for the `falcon` web API framework.
  * Added support for the `turbogears` web framework.
 
 

change-notes/1.20/support/python-frameworks.csv

@@ -1,6 +1,8 @@
 Name, Category
 Bottle, Web framework
+CherryPy, Web framework
 Django, Web application framework
+Falcon, Web API framework
 Flask, Microframework
 Pyramid, Web application framework
 Tornado, Web application framework and asynchronous networking library

cpp/config/suites/c/experimental

@@ -0,0 +1 @@
++ semmlecode-cpp-queries/Likely Bugs/RedundantNullCheckSimple.ql: /Correctness/Common Errors

cpp/config/suites/cpp/experimental

@@ -0,0 +1 @@
++ semmlecode-cpp-queries/Likely Bugs/RedundantNullCheckSimple.ql: /Correctness/Common Errors

cpp/ql/src/Likely Bugs/RedundantNullCheckSimple.ql

@@ -0,0 +1,72 @@
+/**
+ * @name Redundant null check due to previous dereference
+ * @description Checking a pointer for nullness after dereferencing it is
+ *              likely to be a sign that either the check can be removed, or
+ *              it should be moved before the dereference.
+ * @kind problem
+ * @problem.severity error
+ * @id cpp/redundant-null-check-simple
+ * @tags reliability
+ *       correctness
+ *       external/cwe/cwe-476
+ */
+
+/*
+ * Note: this query is not assigned a precision yet because we don't want it on
+ * LGTM until its performance is well understood. It's also lacking qhelp.
+ */
+
+import semmle.code.cpp.ir.IR
+
+class NullInstruction extends ConstantValueInstruction {
+  NullInstruction() {
+    this.getValue() = "0" and
+    this.getResultType().getUnspecifiedType() instanceof PointerType
+  }
+}
+
+/**
+ * An instruction that will never have slicing on its result.
+ */
+class SingleValuedInstruction extends Instruction {
+  SingleValuedInstruction() {
+    this.getResultMemoryAccess() instanceof IndirectMemoryAccess
+    or
+    not this.hasMemoryResult()
+  }
+}
+
+predicate explicitNullTestOfInstruction(Instruction checked, Instruction bool) {
+  bool = any(CompareInstruction cmp |
+      exists(NullInstruction null |
+        cmp.getLeft() = null and cmp.getRight() = checked
+        or
+        cmp.getLeft() = checked and cmp.getRight() = null
+      |
+        cmp instanceof CompareEQInstruction
+        or
+        cmp instanceof CompareNEInstruction
+      )
+    )
+  or
+  bool = any(ConvertInstruction convert |
+      checked = convert.getUnary() and
+      convert.getResultType() instanceof BoolType and
+      checked.getResultType() instanceof PointerType
+    )
+}
+
+from LoadInstruction checked, LoadInstruction deref, SingleValuedInstruction sourceValue
+where
+  explicitNullTestOfInstruction(checked, _) and
+  sourceValue = deref.getSourceAddress().(LoadInstruction).getSourceValue() and
+  sourceValue = checked.getSourceValue() and
+  // This also holds if the blocks are equal, meaning that the check could come
+  // before the deref. That's still not okay because when they're in the same
+  // basic block then the deref is unavoidable even if the check concluded that
+  // the pointer was null. To follow this idea to its full generality, we
+  // should also give an alert when `check` post-dominates `deref`.
+  deref.getBlock().dominates(checked.getBlock()) and
+  not checked.getAST().isInMacroExpansion()
+select checked, "This null check is redundant because the value is $@ in any case", deref,
+  "dereferenced here"

cpp/ql/src/Security/CWE/CWE-078/ExecTainted.ql

@@ -5,7 +5,7 @@
  *              to command injection.
  * @kind problem
  * @problem.severity error
- * @precision high
+ * @precision low
  * @id cpp/command-line-injection
  * @tags security
  *       external/cwe/cwe-078

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

@@ -121,6 +121,13 @@ class Variable extends Declaration, @variable {
     result.getLValue() = this.getAnAccess()
   }
 
+  /**
+   * Holds if this variable is `constexpr`.
+   */
+  predicate isConstexpr() {
+    this.hasSpecifier("is_constexpr")
+  }
+
   /**
    * Holds if this variable is constructed from `v` as a result
    * of template instantiation. If so, it originates either from a template

cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowUtil.qll

@@ -4,6 +4,7 @@
 import cpp
 private import semmle.code.cpp.dataflow.internal.FlowVar
 
+cached
 private newtype TNode =
   TExprNode(Expr e) or
   TParameterNode(Parameter p) { exists(p.getFunction().getBlock()) } or
@@ -161,6 +162,7 @@ private Variable asVariable(Node node) {
  * Holds if data flows from `nodeFrom` to `nodeTo` in exactly one local
  * (intra-procedural) step.
  */
+cached
 predicate localFlowStep(Node nodeFrom, Node nodeTo) {
   // Expr -> Expr
   exprToExprStep_nocfg(nodeFrom.asExpr(), nodeTo.asExpr())

cpp/ql/test/library-tests/variables/constexpr/constexpr.cpp

@@ -0,0 +1,6 @@
+
+constexpr int var_constexpr = 5;
+int var_not_constexpr_initialised = 6;
+const int var_not_constexpr_const = 7;
+int var_not_constexpr;
+

cpp/ql/test/library-tests/variables/constexpr/constexpr.expected

@@ -0,0 +1,10 @@
+| constexpr.cpp:2:15:2:27 | var_constexpr | true |
+| constexpr.cpp:3:5:3:33 | var_not_constexpr_initialised | false |
+| constexpr.cpp:4:11:4:33 | var_not_constexpr_const | false |
+| constexpr.cpp:5:5:5:21 | var_not_constexpr | false |
+| file://:0:0:0:0 | fp_offset | false |
+| file://:0:0:0:0 | gp_offset | false |
+| file://:0:0:0:0 | overflow_arg_area | false |
+| file://:0:0:0:0 | p#0 | false |
+| file://:0:0:0:0 | p#0 | false |
+| file://:0:0:0:0 | reg_save_area | false |

cpp/ql/test/library-tests/variables/constexpr/constexpr.ql

@@ -0,0 +1,5 @@
+import cpp
+
+from Variable v
+select v,
+       any(boolean b | if v.isConstexpr() then b = true else b = false)

cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.cpp

@@ -0,0 +1,71 @@
+void test_simple_bad(int *p) {
+  int x;
+  x = *p;
+  if (p == nullptr) { // BAD
+    return;
+  }
+}
+
+void test_not_same_basic_block(int *p) {
+  int x = *p;
+  if (x > 100)
+    return;
+  if (!p) // BAD
+    return;
+}
+
+void test_indirect(int **p) {
+  int x;
+  x = **p;
+  if (*p == nullptr) { // BAD [NOT DETECTED]
+    return;
+  }
+}
+
+struct ContainsIntPtr {
+  int **intPtr;
+};
+
+bool check_curslist(ContainsIntPtr *cip) {
+  // both the deref and the null check come from the same instruction, but it's
+  // an AliasedDefinition instruction.
+  return *cip->intPtr != nullptr; // GOOD
+}
+
+void test_no_single_dominator(int *p, bool b) {
+  int x;
+  if (b) {
+    x = *p;
+  } else {
+    x = *p;
+  }
+  if (p == nullptr) { // BAD [NOT DETECTED]
+    return;
+  }
+}
+
+int test_postdominator_same_bb(int *p) {
+  int b = (p == nullptr); // BAD
+  // This dereference is a postdominator of the null check, meaning that all
+  // paths from the check to the function exit will pass through it.
+  return *p + b;
+}
+
+int test_postdominator(int *p) {
+  int b = (p == nullptr); // BAD [NOT DETECTED]
+
+  if (b) b++; // This line breaks up the basic block
+
+  // This dereference is a postdominator of the null check, meaning that all
+  // paths from the check to the function exit will pass through it.
+  return *p + b;
+}
+
+int test_inverted_logic(int *p) {
+  if (p == nullptr) { // BAD [NOT DETECTED]
+    // The check above should probably have been `!=` instead of `==`.
+    return *p;
+  } else {
+    return 0;
+  }
+}

cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.expected

@@ -0,0 +1,3 @@
+| RedundantNullCheckSimple.cpp:4:7:4:7 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:3:7:3:8 | Load: * ... | dereferenced here |
+| RedundantNullCheckSimple.cpp:13:8:13:8 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:10:11:10:12 | Load: * ... | dereferenced here |
+| RedundantNullCheckSimple.cpp:48:12:48:12 | Load: p | This null check is redundant because the value is $@ in any case | RedundantNullCheckSimple.cpp:51:10:51:11 | Load: * ... | dereferenced here |

cpp/ql/test/query-tests/Likely Bugs/RedundantNullCheckSimple/RedundantNullCheckSimple.qlref

@@ -0,0 +1 @@
+Likely Bugs/RedundantNullCheckSimple.ql

csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformBad.cs

@@ -0,0 +1,38 @@
+internal class TokenCacheThreadUnsafeICryptoTransformDemo
+{
+    private static SHA256 _sha = SHA256.Create();
+
+    public string ComputeHash(string data)
+    {
+        byte[] passwordBytes = UTF8Encoding.UTF8.GetBytes(data);
+        return Convert.ToBase64String(_sha.ComputeHash(passwordBytes));
+    }
+}
+
+class Program
+{
+    static void Main(string[] args)
+    {
+        int max = 1000;
+        Task[] tasks = new Task[max];
+
+        Action<object> action = (object obj) =>
+        {
+            var unsafeObj = new TokenCacheThreadUnsafeICryptoTransformDemo();
+            if (unsafeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
+            {
+                Console.WriteLine("**** We got incorrect Results!!! ****");
+            }
+        };
+
+        for (int i = 0; i < max; i++)
+        {
+            // hash calculated on all threads should be the same:
+            // ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0= (base64)
+            // 
+            tasks[i] = Task.Factory.StartNew(action, "abc");
+        }
+
+        Task.WaitAll(tasks);
+    }
+}

csharp/ql/src/Likely Bugs/ThreadUnSafeICryptoTransformGood.cs

@@ -0,0 +1,41 @@
+internal class TokenCacheThreadUnsafeICryptoTransformDemoFixed
+{
+    // We are replacing the static SHA256 field with an instance one
+    //
+    //private static SHA256 _sha = SHA256.Create();
+    private SHA256 _sha = SHA256.Create();
+
+    public string ComputeHash(string data)
+    {
+        byte[] passwordBytes = UTF8Encoding.UTF8.GetBytes(data);
+        return Convert.ToBase64String(_sha.ComputeHash(passwordBytes));
+    }
+}
+
+class Program
+{
+    static void Main(string[] args)
+    {
+        int max = 1000;
+        Task[] tasks = new Task[max];
+
+        Action<object> action = (object obj) =>
+        {
+            var safeObj = new TokenCacheThreadUnsafeICryptoTransformDemoFixed();
+            if (safeObj.ComputeHash((string)obj) != "ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0=")
+            {
+                Console.WriteLine("**** We got incorrect Results!!! ****");
+            }
+        };
+
+        for (int i = 0; i < max; i++)
+        {
+            // hash calculated on all threads should be the same:
+            // ungWv48Bz+pBQUDeXa4iI7ADYaOWF3qctBD/YfIAFa0= (base64)
+            // 
+            tasks[i] = Task.Factory.StartNew(action, "abc");
+        }
+
+        Task.WaitAll(tasks);
+    }
+}

csharp/ql/src/Likely Bugs/ThreadUnsafeICryptoTransform.qhelp

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Classes that implement <code>System.Security.Cryptography.ICryptoTransform</code> are not thread safe.</p>
+<p>This problem is caused by the way these classes are implemented using Microsoft CAPI/CNG patterns.</p>
+<p>For example, when a hash class implements this interface, there would typically be an instance-specific hash object created (for example using <code>BCryptCreateHash</code> function). This object can be called multiple times to add data to the hash (for example <code>BCryptHashData</code>). Finally, a function is called that finishes the hash and returns the data (for example <code>BCryptFinishHash</code>).</p>
+<p>Allowing the same hash object to be called with data from multiple threads before calling the finish function could potentially lead to incorrect results.</p>
+<p>For example, if you have multiple threads hashing <code>"abc"</code> on a static hash object, you may occasionally obtain the results (incorrectly) for hashing <code>"abcabc"</code>, or face other unexpected behavior.</p>
+<p>It is very unlikely somebody outside Microsoft would write a class that implements <code>ICryptoTransform</code>, and even if they do, it is likely that they will follow the same common pattern as the existing classes implementing this interface.</p>
+<p>Any object that implements <code>System.Security.Cryptography.ICryptoTransform</code> should not be used in concurrent threads as the instance members of such object are also not thread safe.</p>
+<p>Potential problems may not be evident at first, but can range from explicit errors such as exceptions, to incorrect results when sharing an instance of such an object in multiple threads.</p>
+
+</overview>
+<recommendation>
+<p>If the object is shared across instances, you should consider changing the code to use a non-static object of type <code>System.Security.Cryptography.ICryptoTransform</code> instead.</p>
+<p>As an alternative, you could also look into using <code>ThreadStatic</code> attribute, but make sure you read the initialization remarks on the documentation.</p>
+
+</recommendation>
+<example>
+<p>This example demonstrates the dangers of using a static <code>System.Security.Cryptography.ICryptoTransform</code> in a way that generates incorrect results.</p>
+<sample src="ThreadUnSafeICryptoTransformBad.cs" />
+
+<p>A simple fix is to change the <code>_sha</code> field from being a static member to an instance one by removing the <code>static</code> keyword.</p>
+<sample src="ThreadUnSafeICryptoTransformGood.cs" />
+</example>
+
+<references>
+  <li>
+    Microsoft documentation, <a href="https://docs.microsoft.com/en-us/dotnet/api/system.threadstaticattribute?view=netframework-4.7.2">ThreadStaticAttribute Class</a>.
+  </li>
+  <li>
+    Stack Overflow, <a href="https://stackoverflow.com/questions/26592596/why-does-sha1-computehash-fail-under-high-load-with-many-threads">Why does SHA1.ComputeHash fail under high load with many threads?</a>.
+  </li>
+</references>
+
+</qhelp>