Merge remote-tracking branch 'upstream/master' into ASPNetRequestValidationMode

# Conflicts:
#	change-notes/1.24/analysis-csharp.md

Author: calumgrant

change-notes/1.24/analysis-csharp.md

@@ -19,4 +19,7 @@ The following changes in version 1.24 affect C# analysis in all applications.
 
 ## Changes to libraries
 
+* The taint tracking library now tracks flow through (implicit or explicit) conversion operator calls.
+
 ## Changes to autobuilder
+

change-notes/1.24/analysis-javascript.md

@@ -11,9 +11,9 @@
 
 ## New queries
 
-| **Query**                                                                 | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
-|---------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-
+| **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
+|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| Cross-site scripting through exception (`js/xss-through-exception`) | security, external/cwe/cwe-079, external/cwe/cwe-116              | Highlights potential XSS vulnerabilities where an exception is written to the DOM. Results are not shown on LGTM by default. |
 
 ## Changes to existing queries
 
@@ -25,3 +25,4 @@
 
 ## Changes to libraries
 
+* The predicates `RegExpTerm.getSuccessor` and `RegExpTerm.getPredecessor` have been changed to reflect textual, not operational, matching order. This only makes a difference in lookbehind assertions, which are operationally matched backwards. Previously, `getSuccessor` would mimick this, so in an assertion `(?<=ab)` the term `b` would be considered the predecessor, not the successor, of `a`. Textually, however, `a` is still matched before `b`, and this is the order we now follow.

cpp/ql/src/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql

@@ -84,18 +84,18 @@ predicate hasZeroParamDecl(Function f) {
 }
 
 // True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(Function f) {
-  exists(File file | file.compiledAsC() |
-    file = f.getFile() or file.getAnIncludedFile+() = f.getFile()
-  )
+predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
 }
 
 from FunctionCall fc, Function f, Parameter p
 where
   f = fc.getTarget() and
   p = f.getAParameter() and
   hasZeroParamDecl(f) and
-  isCompiledAsC(f) and
+  isCompiledAsC(f.getFile()) and
   not f.isVarargs() and
   not f instanceof BuiltInFunction and
   p.getIndex() < fc.getNumberOfArguments() and

cpp/ql/src/Likely Bugs/Underspecified Functions/TooFewArguments.ql

@@ -24,10 +24,10 @@ predicate hasZeroParamDecl(Function f) {
 }
 
 // True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(Function f) {
-  exists(File file | file.compiledAsC() |
-    file = f.getFile() or file.getAnIncludedFile+() = f.getFile()
-  )
+predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
 }
 
 from FunctionCall fc, Function f
@@ -36,7 +36,7 @@ where
   not f.isVarargs() and
   not f instanceof BuiltInFunction and
   hasZeroParamDecl(f) and
-  isCompiledAsC(f) and
+  isCompiledAsC(f.getFile()) and
   // There is an explicit declaration of the function whose parameter count is larger
   // than the number of call arguments
   exists(FunctionDeclarationEntry fde | fde = f.getADeclarationEntry() |

cpp/ql/src/Likely Bugs/Underspecified Functions/TooManyArguments.ql

@@ -25,18 +25,18 @@ predicate hasZeroParamDecl(Function f) {
 }
 
 // True if this file (or header) was compiled as a C file
-predicate isCompiledAsC(Function f) {
-  exists(File file | file.compiledAsC() |
-    file = f.getFile() or file.getAnIncludedFile+() = f.getFile()
-  )
+predicate isCompiledAsC(File f) {
+  f.compiledAsC()
+  or
+  exists(File src | isCompiledAsC(src) | src.getAnIncludedFile() = f)
 }
 
 from FunctionCall fc, Function f
 where
   f = fc.getTarget() and
   not f.isVarargs() and
   hasZeroParamDecl(f) and
-  isCompiledAsC(f) and
+  isCompiledAsC(f.getFile()) and
   exists(f.getBlock()) and
   // There must not exist a declaration with the number of parameters
   // at least as large as the number of call arguments

csharp/ql/src/semmle/code/csharp/dataflow/internal/TaintTrackingPrivate.qll

@@ -113,6 +113,12 @@ private class LocalTaintExprStepConfiguration extends ControlFlowReachabilityCon
           scope = e2 and
           isSuccessor = true
         )
+      or
+      e2 = any(OperatorCall oc |
+          oc.getTarget().(ConversionOperator).fromLibrary() and
+          e1 = oc.getAnArgument() and
+          isSuccessor = true
+        )
     )
   }
 

csharp/ql/test/library-tests/dataflow/local/DataFlowStep.expected

@@ -586,6 +586,9 @@
 | LocalDataFlow.cs:480:67:480:68 | os | LocalDataFlow.cs:486:32:486:33 | access to parameter os |
 | LocalDataFlow.cs:483:21:483:21 | access to parameter x | LocalDataFlow.cs:483:16:483:21 | ... = ... |
 | LocalDataFlow.cs:486:32:486:33 | access to parameter os | LocalDataFlow.cs:486:26:486:33 | ... = ... |
+| LocalDataFlow.cs:491:41:491:44 | args | LocalDataFlow.cs:493:29:493:32 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | [post] access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
 | SSA.cs:5:17:5:17 | SSA entry def(this.S) | SSA.cs:67:9:67:14 | access to field S |
 | SSA.cs:5:17:5:17 | this | SSA.cs:67:9:67:12 | this access |
 | SSA.cs:5:26:5:32 | tainted | SSA.cs:8:24:8:30 | access to parameter tainted |

csharp/ql/test/library-tests/dataflow/local/LocalDataFlow.cs

@@ -485,4 +485,12 @@ public void AssignmentFlow(IDisposable x, IEnumerable<object> os)
         IEnumerable<object> os2;
         foreach(var o in os2 = os) { }
     }
+
+    public static implicit operator LocalDataFlow(string[] args) => null;
+
+    public void ConversionFlow(string[] args)
+    {
+        Span<object> span = args; // flow (library operator)
+        LocalDataFlow x = args; // no flow (source code operator)
+    }
 }

csharp/ql/test/library-tests/dataflow/local/TaintTrackingStep.expected

@@ -736,6 +736,11 @@
 | LocalDataFlow.cs:480:67:480:68 | os | LocalDataFlow.cs:486:32:486:33 | access to parameter os |
 | LocalDataFlow.cs:483:21:483:21 | access to parameter x | LocalDataFlow.cs:483:16:483:21 | ... = ... |
 | LocalDataFlow.cs:486:32:486:33 | access to parameter os | LocalDataFlow.cs:486:26:486:33 | ... = ... |
+| LocalDataFlow.cs:491:41:491:44 | args | LocalDataFlow.cs:491:41:491:44 | args |
+| LocalDataFlow.cs:491:41:491:44 | args | LocalDataFlow.cs:493:29:493:32 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | [post] access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
+| LocalDataFlow.cs:493:29:493:32 | access to parameter args | LocalDataFlow.cs:493:29:493:32 | call to operator implicit conversion |
+| LocalDataFlow.cs:493:29:493:32 | access to parameter args | LocalDataFlow.cs:494:27:494:30 | access to parameter args |
 | SSA.cs:5:17:5:17 | SSA entry def(this.S) | SSA.cs:67:9:67:14 | access to field S |
 | SSA.cs:5:17:5:17 | this | SSA.cs:67:9:67:12 | this access |
 | SSA.cs:5:26:5:32 | tainted | SSA.cs:5:26:5:32 | tainted |

csharp/ql/test/library-tests/frameworks/EntityFramework/EntityFrameworkCore.cs

@@ -50,7 +50,7 @@ void TestDataFlow()
             Sink(taintSource);  // Tainted
             Sink(new RawSqlString(taintSource));  // Tainted
             Sink((RawSqlString)taintSource);  // Tainted
-            Sink((RawSqlString)(FormattableString)$"{taintSource}");  // Not tainted
+            Sink((RawSqlString)(FormattableString)$"{taintSource}");  // Tainted, but not reported because conversion operator is in a stub .cs file
 
             // Tainted via database, even though technically there were no reads or writes to the database in this particular case.
             var p1 = new Person { Name = taintSource };