Merge branch 'master' into connect-ir-dataflow-models

Author: Robert Marsh

change-notes/1.24/analysis-cpp.md

@@ -26,6 +26,10 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 
 ## Changes to libraries
 
+* The data-flow library has been improved when flow through functions needs to be
+  combined with both taint tracking and flow through fields allowing more flow
+  to be tracked. This affects and improves some security queries, which may
+  report additional results.
 * Created the `semmle.code.cpp.models.interfaces.Allocation` library to model allocation such as `new` expressions and calls to `malloc`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
 * Created the `semmle.code.cpp.models.interfaces.Deallocation` library to model deallocation such as `delete` expressions and calls to `free`. This in intended to replace the functionality in `semmle.code.cpp.commons.Alloc` with a more consistent and useful interface.
 * The new class `StackVariable` should be used in place of `LocalScopeVariable`
@@ -40,4 +44,4 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had
   the following improvements:
   * The library now models data flow through `strdup` and similar functions.
-  
+  * The library now models data flow through formatting functions such as `sprintf`.

change-notes/1.24/analysis-csharp.md

@@ -6,8 +6,12 @@ The following changes in version 1.24 affect C# analysis in all applications.
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Assembly path injection (`cs/assembly-path-injection`) | security, external/cwe/cwe-114 | Finds user-controlled data used to load an assembly. |
 | Insecure configuration for ASP.NET requestValidationMode (`cs/insecure-request-validation-mode`) | security, external/cwe/cwe-016 | Finds where this attribute has been set to a value less than 4.5, which turns off some validation features and makes the application less secure. |
-| Page request validation is disabled (`cs/web/request-validation-disabled`) | security, frameworks/asp.net, external/cwe/cwe-016 | Finds where ASP.NET page request validation has been disabled, which could makes the application less secure. |
+| Insecure SQL connection (`cs/insecure-sql-connection`) | security, external/cwe/cwe-327 | Finds unencrypted SQL connection strings. |
+| Page request validation is disabled (`cs/web/request-validation-disabled`) | security, frameworks/asp.net, external/cwe/cwe-016 | Finds where ASP.NET page request validation has been disabled, which could make the application less secure. |
+| Serialization check bypass (`cs/serialization-check-bypass`) | security, external/cwe/cwe-20 | Finds where data is not validated in a deserialization method. |
+| XML injection (`cs/xml-injection`) | security, external/cwe/cwe-091 | Finds user-controlled data that is used to write directly to an XML document. |
 
 ## Changes to existing queries
 
@@ -25,9 +29,12 @@ The following changes in version 1.24 affect C# analysis in all applications.
 
 ## Changes to libraries
 
+* The data-flow library has been improved when flow through methods needs to be
+  combined with both taint tracking and flow through fields allowing more flow
+  to be tracked. This affects and improves most security queries, which may
+  report additional results.
 * The taint tracking library now tracks flow through (implicit or explicit) conversion operator calls.
 * [Code contracts](https://docs.microsoft.com/en-us/dotnet/framework/debug-trace-profile/code-contracts) are now recognized, and are treated like any other assertion methods.
 * Expression nullability flow state is given by the predicates `Expr.hasNotNullFlowState()` and `Expr.hasMaybeNullFlowState()`.
 
 ## Changes to autobuilder
-

change-notes/1.24/analysis-java.md

@@ -10,19 +10,25 @@ The following changes in version 1.24 affect Java analysis in all applications.
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
+| Disabled Spring CSRF protection (`java/spring-disabled-csrf-protection`) | security, external/cwe/cwe-352 | Finds disabled Cross-Site Request Forgery (CSRF) protection in Spring. |
 | Failure to use HTTPS or SFTP URL in Maven artifact upload/download (`java/maven/non-https-url`) | security, external/cwe/cwe-300, external/cwe/cwe-319, external/cwe/cwe-494, external/cwe/cwe-829 | Finds use of insecure protocols during Maven dependency resolution. Results are shown on LGTM by default. |
+| Left shift by more than the type width (`java/lshift-larger-than-type-width`) | correctness | Finds left shifts of ints by 32 bits or more and left shifts of longs by 64 bits or more. Results are shown on LGTM by default. |
 | Suspicious date format (`java/suspicious-date-format`) | correctness | Finds date format patterns that use placeholders that are likely to be incorrect. |
 
 ## Changes to existing queries
 
 | **Query**                    | **Expected impact**    | **Change**                        |
 |------------------------------|------------------------|-----------------------------------|
 | Dereferenced variable may be null (`java/dereferenced-value-may-be-null`) | Fewer false positives | Final fields with a non-null initializer are no longer reported. |
-| Expression always evaluates to the same value (`java/evaluation-to-constant`) | Fewer false positives | Expressions of the form `0 * x` are usually intended and no longer reported. |
+| Expression always evaluates to the same value (`java/evaluation-to-constant`) | Fewer false positives | Expressions of the form `0 * x` are usually intended and no longer reported. Also left shift of ints by 32 bits and longs by 64 bits are no longer reported as they are not constant, these results are instead reported by the new query `java/lshift-larger-than-type-width`. |
 | Useless null check (`java/useless-null-check`) | More true positives | Useless checks on final fields with a non-null initializer are now reported. |
 
 ## Changes to libraries
 
+* The data-flow library has been improved when flow through methods needs to be
+  combined with both taint tracking and flow through fields allowing more flow
+  to be tracked. This affects and improves most security queries, which may
+  report additional results.
 * Identification of test classes has been improved. Previously, one of the
   match conditions would classify any class with a name containing the string
   "Test" as a test class, but now this matching has been replaced with one that

change-notes/1.24/analysis-javascript.md

@@ -38,6 +38,7 @@
 | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | 
 | Expression has no effect (`js/useless-expression`) | Fewer false positive results | The query now recognizes block-level flow type annotations and ignores the first statement of a try block. |
 | Use of call stack introspection in strict mode (`js/strict-mode-call-stack-introspection`) | Fewer false positive results | The query no longer flags expression statements. |
+| Missing CSRF middleware (`js/missing-token-validation`) | Fewer false positive results | The query reports fewer duplicates and only flags handlers that explicitly access cookie data. |
 
 ## Changes to libraries
 

config/identical-files.json

@@ -82,6 +82,14 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/IRType.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/IRType.qll"
   ],
+  "IR IRConfiguration": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/IRConfiguration.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/IRConfiguration.qll"
+  ],
+  "IR UseSoundEscapeAnalysis": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/UseSoundEscapeAnalysis.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/UseSoundEscapeAnalysis.qll"
+  ],
   "IR Operand Tag": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/internal/OperandTag.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/internal/OperandTag.qll"
@@ -182,9 +190,14 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstructionImports.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstructionImports.qll"
   ],
-  "C++ SSA AliasAnalysis": [
+  "SSA AliasAnalysis": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll"
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll"
+  ],
+  "C++ SSA AliasAnalysisImports": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysisImports.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysisImports.qll"
   ],
   "C++ IR ValueNumberingImports": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/internal/ValueNumberingImports.qll",
@@ -195,6 +208,10 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll"
   ],
+  "IR AliasConfiguration (unaliased_ssa)": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll"
+  ],
   "IR SSA SSAConstruction": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SSAConstruction.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/internal/SSAConstruction.qll",

cpp/ql/src/Architecture/FeatureEnvy.ql

@@ -25,7 +25,8 @@ predicate functionUsesFunction(Function source, Function f, File target) {
 }
 
 predicate dependencyCount(Function source, File target, int res) {
-  res = strictcount(Declaration d |
+  res =
+    strictcount(Declaration d |
       functionUsesVariable(source, d, target) or
       functionUsesFunction(source, d, target)
     )

cpp/ql/src/Architecture/General Top-Level Information/GeneralStatistics.ql

@@ -38,14 +38,16 @@ where
   n = count(Function f | f.fromSource()).toString()
   or
   l = "Number of Lines Of Code" and
-  n = sum(File f, int toSum |
+  n =
+    sum(File f, int toSum |
       f.fromSource() and toSum = f.getMetrics().getNumberOfLinesOfCode()
     |
       toSum
     ).toString()
   or
   l = "Self-Containedness" and
-  n = (
+  n =
+    (
         100 * sum(Class c | c.fromSource() | c.getMetrics().getEfferentSourceCoupling()) /
           sum(Class c | c.fromSource() | c.getMetrics().getEfferentCoupling())
       ).toString() + "%"

cpp/ql/src/Architecture/Refactoring Opportunities/ClassesWithManyFields.ql

@@ -80,11 +80,8 @@ class VariableDeclarationLine extends TVariableDeclarationInfo {
    * (that is, the first is 0, the second is 1 and so on).
    */
   private int getRank() {
-    line = rank[result](VariableDeclarationLine vdl, int l |
-        vdl = TVariableDeclarationLine(c, f, l)
-      |
-        l
-      )
+    line =
+      rank[result](VariableDeclarationLine vdl, int l | vdl = TVariableDeclarationLine(c, f, l) | l)
   }
 
   /**
@@ -133,7 +130,8 @@ class VariableDeclarationGroup extends VariableDeclarationLine {
    * Gets the number of uniquely named `VariableDeclarationEntry`s in this group.
    */
   int getCount() {
-    result = count(VariableDeclarationLine l |
+    result =
+      count(VariableDeclarationLine l |
         l = getProximateNext*()
       |
         l.getAVDE().getVariable().getName()
@@ -166,7 +164,8 @@ class ExtClass extends Class {
 
 from ExtClass c, int n, VariableDeclarationGroup vdg, string suffix
 where
-  n = strictcount(string fieldName |
+  n =
+    strictcount(string fieldName |
       exists(Field f |
         f.getDeclaringType() = c and
         fieldName = f.getName() and

cpp/ql/src/Best Practices/Likely Errors/EmptyBlock.ql

@@ -50,21 +50,24 @@ class BlockOrNonChild extends Element {
 
   private int getNonContiguousStartRankIn(AffectedFile file) {
     // When using `rank` with `order by`, the ranks may not be contiguous.
-    this = rank[result](BlockOrNonChild boc, int startLine, int startCol |
+    this =
+      rank[result](BlockOrNonChild boc, int startLine, int startCol |
         boc.getLocation().hasLocationInfo(file.getAbsolutePath(), startLine, startCol, _, _)
       |
         boc order by startLine, startCol
       )
   }
 
   int getStartRankIn(AffectedFile file) {
-    this.getNonContiguousStartRankIn(file) = rank[result](int rnk |
+    this.getNonContiguousStartRankIn(file) =
+      rank[result](int rnk |
         exists(BlockOrNonChild boc | boc.getNonContiguousStartRankIn(file) = rnk)
       )
   }
 
   int getNonContiguousEndRankIn(AffectedFile file) {
-    this = rank[result](BlockOrNonChild boc, int endLine, int endCol |
+    this =
+      rank[result](BlockOrNonChild boc, int endLine, int endCol |
         boc.getLocation().hasLocationInfo(file.getAbsolutePath(), _, _, endLine, endCol)
       |
         boc order by endLine, endCol
@@ -79,9 +82,8 @@ predicate emptyBlockContainsNonchild(Block b) {
   emptyBlock(_, b) and
   exists(BlockOrNonChild c, AffectedFile file |
     c.(BlockOrNonChild).getStartRankIn(file) = 1 + b.(BlockOrNonChild).getStartRankIn(file) and
-    c.(BlockOrNonChild).getNonContiguousEndRankIn(file) < b
-          .(BlockOrNonChild)
-          .getNonContiguousEndRankIn(file)
+    c.(BlockOrNonChild).getNonContiguousEndRankIn(file) <
+      b.(BlockOrNonChild).getNonContiguousEndRankIn(file)
   )
 }
 

cpp/ql/src/Best Practices/Magic Constants/MagicConstants.qll

@@ -307,7 +307,8 @@ predicate nonTrivialValue(string value, Literal literal) {
 }
 
 predicate valueOccurrenceCount(string value, int n) {
-  n = strictcount(Location loc |
+  n =
+    strictcount(Location loc |
       exists(Literal lit | lit.getLocation() = loc | nonTrivialValue(value, lit)) and
       // Exclude generated files (they do not have the same maintainability
       // concerns as ordinary source files)
@@ -338,7 +339,8 @@ predicate check(Literal lit, string value, int n, File f) {
 }
 
 predicate checkWithFileCount(string value, int overallCount, int fileCount, File f) {
-  fileCount = strictcount(Location loc |
+  fileCount =
+    strictcount(Location loc |
       exists(Literal lit | lit.getLocation() = loc | check(lit, value, overallCount, f))
     )
 }
@@ -364,7 +366,8 @@ predicate firstOccurrence(Literal lit, string value, int n) {
 predicate magicConstant(Literal e, string msg) {
   exists(string value, int n |
     firstOccurrence(e, value, n) and
-    msg = "Magic constant: literal '" + value + "' is repeated " + n.toString() +
+    msg =
+      "Magic constant: literal '" + value + "' is repeated " + n.toString() +
         " times and should be encapsulated in a constant."
   )
 }