JS: replace stale mentions of 'captured' with 'local'

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

@@ -40,6 +40,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/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())

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>

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

@@ -0,0 +1,85 @@
+/**
+ * @name Class defines a field that uses an ICryptoTransform class in a way that would be unsafe for concurrent threads.
+ * @description The class has a field that directly or indirectly make use of a static System.Security.Cryptography.ICryptoTransform object.
+ *              Using this an instance of this class in concurrent threads is dangerous as it may not only result in an error, 
+ *              but under some circumstances may also result in incorrect results.
+ * @kind problem
+ * @problem.severity warning
+ * @precision medium
+ * @id cs/thread-unsafe-icryptotransform-field-in-class
+ * @tags concurrency
+ *       security
+ *       external/cwe/cwe-362
+ */
+
+import csharp
+
+class ICryptoTransform extends Class {
+  ICryptoTransform() {
+    this.getABaseType*().hasQualifiedName("System.Security.Cryptography", "ICryptoTransform")
+  }
+}
+
+predicate usesICryptoTransformType( Type t ) {
+  exists(  ICryptoTransform ict |
+    ict = t
+    or usesICryptoTransformType( t.getAChild() )
+  )
+}
+
+predicate hasICryptoTransformMember( Class c) {
+  exists( Field f |
+    f = c.getAMember()
+    and ( 
+      exists( ICryptoTransform ict | ict = f.getType() )
+      or hasICryptoTransformMember(f.getType())
+      or usesICryptoTransformType(f.getType())
+    )
+  )
+}
+
+predicate hasICryptoTransformStaticMemberNested( Class c ) {
+  exists( Field f |
+    f = c.getAMember() |
+    hasICryptoTransformStaticMemberNested( f.getType() )
+    or (
+      f.isStatic() and hasICryptoTransformMember(f.getType())
+      and not exists( Attribute a
+        | a = f.getAnAttribute() | 
+        a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
+      )
+    )
+  )
+}
+
+predicate hasICryptoTransformStaticMember( Class c, string msg) {
+  exists( Field f |
+    f = c.getAMember*()
+    and f.isStatic()
+    and not exists( Attribute a
+      | a = f.getAnAttribute() 
+      and a.getType().getQualifiedName() = "System.ThreadStaticAttribute"
+    )
+    and (
+      exists( ICryptoTransform ict | 
+        ict = f.getType() 
+        and msg = "Static field " + f + " of type " + f.getType() + ", implements 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads."
+      )
+      or 
+      ( 
+        not exists( ICryptoTransform ict | ict = f.getType() ) // Avoid dup messages
+        and exists( Type t | t = f.getType() |  
+          usesICryptoTransformType(t)
+          and msg = "Static field " + f + " of type " + f.getType() + " makes usage of 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads."
+        )
+      )
+    )
+  )
+  or ( hasICryptoTransformStaticMemberNested(c) 
+    and msg = "Class" + c + " implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads."
+  )
+}
+
+from Class c , string s
+  where hasICryptoTransformStaticMember(c, s)
+select c, s

csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.cs

@@ -0,0 +1,114 @@
+// semmle-extractor-options: /r:System.Security.Cryptography.Csp.dll /r:System.Security.Cryptography.Algorithms.dll /r:System.Security.Cryptography.Primitives.dll
+using System;
+using System.Collections.Generic;
+using System.Security.Cryptography;
+
+public class Nest01
+{
+    private readonly SHA256 _sha;
+
+    public Nest01()
+    {
+        _sha = SHA256.Create();
+    }
+}
+
+public class Nest02
+{
+    private readonly Nest01 _n;
+
+    public Nest02()
+    {
+        _n = new Nest01();
+    }
+}
+
+public class ListNonStatic
+{
+    private List<SHA512> _shaList;
+
+    public ListNonStatic()
+    {
+        _shaList = new List<SHA512>();
+    }
+}
+
+/// <summary>
+/// Positive results (classes are not thread safe)
+/// </summary>
+public class Nest03
+{
+    private static readonly Nest01 _n = new Nest01();
+}
+
+public class Nest04
+{
+    static ListNonStatic _list = new ListNonStatic();
+}
+
+public static class StaticMemberChildUsage
+{
+    public enum DigestAlgorithm
+    {
+        SHA1,
+        SHA256,
+    }
+
+    private static readonly Dictionary<DigestAlgorithm, HashAlgorithm> HashMap = new Dictionary<DigestAlgorithm, HashAlgorithm>
+        {
+            { DigestAlgorithm.SHA1, SHA1.Create() },
+            { DigestAlgorithm.SHA256, SHA256.Create() },
+        };
+}
+
+public class StaticMember
+{
+    private static SHA1 _sha1 = SHA1.Create();
+}
+
+public class IndirectStatic2
+{
+    static Nest02 _n = new Nest02();
+}
+
+/// <summary>
+/// Should not be flagged (thread safe)
+/// </summary>
+
+public class IndirectStatic
+{
+    StaticMember tc;
+}
+
+public class TokenCacheFP
+{
+    /// <summary>
+    ///  Should be OK. Not shared between threads
+    /// </summary>
+    [ThreadStatic]
+    private static SHA1 _sha1 = SHA1.Create();
+
+    private string ComputeHash(string password)
+    {
+        return password;
+    }
+}
+
+public class TokenCacheNonStat
+{
+    /// <summary>
+    ///  Should be OK. Not shared between threads
+    /// </summary>
+    private SHA1 _sha1;
+
+    public TokenCacheNonStat()
+    {
+        _sha1 = SHA1.Create();
+    }
+
+    private string ComputeHash(string password)
+    {
+        return password;
+    }
+}
+

csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.expected

@@ -0,0 +1,5 @@
+| ThreadUnsafeICryptoTransform.cs:39:14:39:19 | Nest03 | ClassNest03 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
+| ThreadUnsafeICryptoTransform.cs:44:14:44:19 | Nest04 | ClassNest04 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |
+| ThreadUnsafeICryptoTransform.cs:49:21:49:42 | StaticMemberChildUsage | Static field HashMap of type Dictionary<DigestAlgorithm,HashAlgorithm> makes usage of 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads. |
+| ThreadUnsafeICryptoTransform.cs:64:14:64:25 | StaticMember | Static field _sha1 of type SHA1, implements 'System.Security.Cryptography.ICryptoTransform', but it does not have an attribute [ThreadStatic]. The usage of this class is unsafe for concurrent threads. |
+| ThreadUnsafeICryptoTransform.cs:69:14:69:28 | IndirectStatic2 | ClassIndirectStatic2 implementation depends on a static object of type 'System.Security.Cryptography.ICryptoTransform' in a way that is unsafe for concurrent threads. |

csharp/ql/test/query-tests/Likely Bugs/ThreadUnsafeICryptoTransform/ThreadUnsafeICryptoTransform.qlref

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