Merge branch 'master' of github.com:github/codeql into SharedDataflow_Classes

Author: yoff

change-notes/1.26/analysis-cpp.md

@@ -0,0 +1,19 @@
+# Improvements to C/C++ analysis
+
+The following changes in version 1.26 affect C/C++ analysis in all applications.
+
+## General improvements
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+| Inconsistent direction of for loop (`cpp/inconsistent-loop-direction`) | Fewer false positive results | The query now accounts for intentional wrapping of an unsigned loop counter. |
+
+## Changes to libraries
+

change-notes/1.26/analysis-javascript.md

@@ -0,0 +1,30 @@
+# Improvements to JavaScript analysis
+
+## General improvements
+
+* Support for the following frameworks and libraries has been improved:
+  - [fast-json-stable-stringify](https://www.npmjs.com/package/fast-json-stable-stringify)
+  - [fast-safe-stringify](https://www.npmjs.com/package/fast-safe-stringify)
+  - [javascript-stringify](https://www.npmjs.com/package/javascript-stringify)
+  - [js-stringify](https://www.npmjs.com/package/js-stringify)
+  - [json-stable-stringify](https://www.npmjs.com/package/json-stable-stringify)
+  - [json-stringify-safe](https://www.npmjs.com/package/json-stringify-safe)
+  - [json3](https://www.npmjs.com/package/json3)
+  - [object-inspect](https://www.npmjs.com/package/object-inspect)
+  - [pretty-format](https://www.npmjs.com/package/pretty-format)
+  - [stringify-object](https://www.npmjs.com/package/stringify-object)
+
+## New queries
+
+| **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |
+|---------------------------------------------------------------------------------|-------------------------------------------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **Query**                      | **Expected impact**          | **Change**                                                                |
+|--------------------------------|------------------------------|---------------------------------------------------------------------------|
+
+
+## Changes to libraries
+

cpp/ql/src/Likely Bugs/Likely Typos/inconsistentLoopDirection.ql

@@ -50,7 +50,12 @@ predicate illDefinedDecrForStmt(
     DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(lesserOperand)) and
     // `initialCondition` < `terminalCondition`
     (
-      upperBound(initialCondition) < lowerBound(terminalCondition)
+      upperBound(initialCondition) < lowerBound(terminalCondition) and
+      (
+        // exclude cases where the loop counter is `unsigned` (where wrapping behaviour can be used deliberately)
+        v.getUnspecifiedType().(IntegralType).isSigned() or
+        initialCondition.getValue().toInt() = 0
+      )
       or
       (forstmt.conditionAlwaysFalse() or forstmt.conditionAlwaysTrue())
     )

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

@@ -214,6 +214,9 @@ abstract class ImplicitConversionFunction extends MemberFunction {
 }
 
 /**
+ * DEPRECATED: as of C++11 this class does not correspond perfectly with the
+ * language definition of a converting constructor.
+ *
  * A C++ constructor that also defines an implicit conversion. For example the
  * function `MyClass` in the following code is a `ConversionConstructor`:
  * ```
@@ -225,15 +228,16 @@ abstract class ImplicitConversionFunction extends MemberFunction {
  * };
  * ```
  */
-class ConversionConstructor extends Constructor, ImplicitConversionFunction {
+deprecated class ConversionConstructor extends Constructor, ImplicitConversionFunction {
   ConversionConstructor() {
     strictcount(Parameter p | p = getAParameter() and not p.hasInitializer()) = 1 and
-    not hasSpecifier("explicit") and
-    not this instanceof CopyConstructor
+    not hasSpecifier("explicit")
   }
 
   override string getAPrimaryQlClass() {
-    not this instanceof MoveConstructor and result = "ConversionConstructor"
+    not this instanceof CopyConstructor and
+    not this instanceof MoveConstructor and
+    result = "ConversionConstructor"
   }
 
   /** Gets the type this `ConversionConstructor` takes as input. */

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

@@ -65,6 +65,15 @@ predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeT
     // tracking. The flow from expression `x` into `x++` etc. is handled in the
     // case above.
     exprTo = DataFlow::getAnAccessToAssignedVariable(exprFrom.(PostfixCrementOperation))
+    or
+    // In `for (char c : s) { ... c ... }`, this rule propagates taint from `s`
+    // to `c`.
+    exists(RangeBasedForStmt rbf |
+      exprFrom = rbf.getRange() and
+      // It's guaranteed up to at least C++20 that the range-based for loop
+      // desugars to a variable with an initializer.
+      exprTo = rbf.getVariable().getInitializer().getExpr()
+    )
   )
   or
   // Taint can flow through modeled functions

cpp/ql/src/semmle/code/cpp/models/implementations/MemberFunction.qll

@@ -7,9 +7,17 @@ import semmle.code.cpp.models.interfaces.DataFlow
 import semmle.code.cpp.models.interfaces.Taint
 
 /**
- * Model for C++ conversion constructors.
+ * Model for C++ conversion constructors. As of C++11 this does not correspond
+ * perfectly with the language definition of a converting constructor, however,
+ * it does correspond with the constructors we are confident taint should flow
+ * through.
  */
-class ConversionConstructorModel extends ConversionConstructor, TaintFunction {
+class ConversionConstructorModel extends Constructor, TaintFunction {
+  ConversionConstructorModel() {
+    strictcount(Parameter p | p = getAParameter() and not p.hasInitializer()) = 1 and
+    not hasSpecifier("explicit")
+  }
+
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // taint flow from the first constructor argument to the returned object
     input.isParameter(0) and