Merge pull request #723 from Semmle/qlucie/master

Master-to-next merge

Author: Max Schaefer

change-notes/1.20/analysis-csharp.md

@@ -17,6 +17,7 @@
 
 ## Changes to code extraction
 
+* Fix extraction of `for` statements where the condition declares new variables using `is`.
 * Initializers of `stackalloc` arrays are now extracted.
 
 ## Changes to QL libraries

change-notes/1.20/analysis-javascript.md

@@ -2,7 +2,7 @@
 
 ## General improvements
 
-* Support for popular libraries has been improved. Consequently, queries may produce more results on code bases that use the following features:
+* Support for popular libraries has been improved. Consequently, queries may produce better results on code bases that use the following features:
   - client-side code, for example [React](https://reactjs.org/)
   - cookies and webstorage, for example [js-cookie](https://github.com/js-cookie/js-cookie)
   - server-side code, for example [hapi](https://hapijs.com/)
@@ -18,15 +18,18 @@
 | Incomplete regular expression for hostnames (`js/incomplete-hostname-regexp`) | correctness, security, external/cwe/cwe-020 |  Highlights hostname sanitizers that are likely to be incomplete, indicating a violation of [CWE-020](https://cwe.mitre.org/data/definitions/20.html). Results are shown on LGTM by default.|
 | 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. |
 | 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
 
 | **Query**                                  | **Expected impact**          | **Change**                                                                   |
 |--------------------------------------------|------------------------------|------------------------------------------------------------------------------|
-| Client-side cross-site scripting           | More results                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection. |
+| Client-side cross-site scripting           | More true-positive results, fewer false-positive results.                 | This rule now recognizes WinJS functions that are vulnerable to HTML injection, and no longer flags certain safe uses of jQuery. |
 | Insecure randomness | More results | This rule now flags insecure uses of `crypto.pseudoRandomBytes`. |
+| 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. |
+| Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. |
 
 ## Changes to QL libraries

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

@@ -55,10 +55,18 @@ class Macro extends PreprocessorDirective, @ppd_define {
 }
 
 /**
- * A macro access (macro expansion or other macro access).
+ * A macro access.  For example:
+ * ```
+ * #ifdef MACRO1     // this line contains a MacroAccess
+ *   int x = MACRO2; // this line contains a MacroAccess
+ * #endif
+ * ```
+ * 
+ * See also `MacroInvocation`, which represents only macro accesses
+ * that are expanded (such as in the second line of the example above).
  */
 class MacroAccess extends Locatable, @macroinvocation {
-  /** Gets the macro being invoked. */
+  /** Gets the macro that is being accessed. */
   Macro getMacro() { macroinvocations(underlyingElement(this),unresolveElement(result),_,_) }
 
   /**
@@ -73,7 +81,7 @@ class MacroAccess extends Locatable, @macroinvocation {
   }
 
   /**
-   * Gets the location of this macro invocation. For a nested invocation, where
+   * Gets the location of this macro access. For a nested access, where
    * `exists(this.getParentInvocation())`, this yields a location either inside
    * a `#define` directive or inside an argument to another macro.
    */
@@ -126,14 +134,22 @@ class MacroAccess extends Locatable, @macroinvocation {
 
   override string toString() { result = this.getMacro().getHead() }
 
-  /** Gets the name of the invoked macro. */
+  /** Gets the name of the accessed macro. */
   string getMacroName() {
     result = getMacro().getName()
   }
 }
 
 /**
- * A macro invocation (macro expansion).
+ * A macro invocation (macro access that is expanded).  For example:
+ * ```
+ * #ifdef MACRO1
+ *   int x = MACRO2; // this line contains a MacroInvocation
+ * #endif
+ * ```
+ * 
+ * See also `MacroAccess`, which also represents macro accesses where the macro
+ * is checked but not expanded (such as in the first line of the example above).
  */
 class MacroInvocation extends MacroAccess {
   MacroInvocation() {
@@ -174,7 +190,7 @@ class MacroInvocation extends MacroAccess {
   /**
    * Gets the top-level expression associated with this macro invocation,
    * if any. Note that this predicate will fail if the top-level expanded
-   * element is a statement rather than an expression.
+   * element is not an expression (for example if it is a statement).
    */
   Expr getExpr() {
     result = getAnExpandedElement() and
@@ -185,7 +201,7 @@ class MacroInvocation extends MacroAccess {
   /**
    * Gets the top-level statement associated with this macro invocation, if
    * any. Note that this predicate will fail if the top-level expanded
-   * element is an expression rather than a statement.
+   * element is not a statement (for example if it is an expression).
    */
   Stmt getStmt() {
     result = getAnExpandedElement() and

csharp/extractor/Semmle.Extraction.CSharp/Entities/Accessor.cs

@@ -58,7 +58,7 @@ public override void Populate()
             }
             else
             {
-                Context.ModelError(symbol, "Undhandled accessor kind");
+                Context.ModelError(symbol, "Unhandled accessor kind");
                 return;
             }
 

csharp/extractor/Semmle.Extraction.CSharp/Entities/Constructor.cs

@@ -118,7 +118,7 @@ public override IId Id
                     if (symbol.IsStatic) tb.Append("static");
                     tb.Append(ContainingType);
                     AddParametersToId(Context, tb, symbol);
-                    tb.Append("; constructor");
+                    tb.Append(";constructor");
                 });
             }
         }

csharp/extractor/Semmle.Extraction.CSharp/Entities/Event.cs

@@ -30,12 +30,13 @@ public override void Populate()
             Context.Emit(Tuples.events(this, symbol.GetName(), ContainingType, type.TypeRef, Create(Context, symbol.OriginalDefinition)));
 
             var adder = symbol.AddMethod;
-            if (adder != null)
-                EventAccessor.Create(Context, adder);
-
             var remover = symbol.RemoveMethod;
-            if (remover != null)
-                EventAccessor.Create(Context, remover);
+
+            if (!(adder is null))
+                Method.Create(Context, adder);
+
+            if (!(remover is null))
+                Method.Create(Context, remover);
 
             ExtractModifiers();
             BindComments();

csharp/extractor/Semmle.Extraction.CSharp/Entities/Expression.cs

@@ -117,17 +117,19 @@ public static ExprKind UnaryOperatorKind(Context cx, ExprKind originalKind, Expr
         public void OperatorCall(ExpressionSyntax node)
         {
             var @operator = cx.GetSymbolInfo(node);
-            var method = @operator.Symbol as IMethodSymbol;
-
-            if (GetCallType(cx, node) == CallType.Dynamic)
+            if (@operator.Symbol is IMethodSymbol method)
             {
-                UserOperator.OperatorSymbol(method.Name, out string operatorName);
-                cx.Emit(Tuples.dynamic_member_name(this, operatorName));
-                return;
-            }
 
-            if (method != null)
+                var callType = GetCallType(cx, node);
+                if (callType == CallType.Dynamic)
+                {
+                    UserOperator.OperatorSymbol(method.Name, out string operatorName);
+                    cx.Emit(Tuples.dynamic_member_name(this, operatorName));
+                    return;
+                }
+
                 cx.Emit(Tuples.expr_call(this, Method.Create(cx, method)));
+            }
         }
 
         public enum CallType
@@ -148,12 +150,9 @@ public static CallType GetCallType(Context cx, ExpressionSyntax node)
         {
             var @operator = cx.GetSymbolInfo(node);
 
-            if (@operator.Symbol != null)
+            if (@operator.Symbol is IMethodSymbol method)
             {
-                var method = @operator.Symbol as IMethodSymbol;
-
-                var containingSymbol = method.ContainingSymbol as ITypeSymbol;
-                if (containingSymbol != null && containingSymbol.TypeKind == Microsoft.CodeAnalysis.TypeKind.Dynamic)
+                if (method.ContainingSymbol is ITypeSymbol containingSymbol && containingSymbol.TypeKind == Microsoft.CodeAnalysis.TypeKind.Dynamic)
                 {
                     return CallType.Dynamic;
                 }