Merge branch 'main' of github.com:github/codeql into SharedDataflow_PostUpdateNodes

Author: yoff

change-notes/1.25/analysis-python.md

@@ -20,4 +20,3 @@ The following changes in version 1.25 affect Python analysis in all applications
 ## Changes to libraries
 
 * Importing `semmle.python.web.HttpRequest` will no longer import `UntrustedStringKind` transitively. `UntrustedStringKind` is the most commonly used non-abstract subclass of `ExternalStringKind`. If not imported (by one mean or another), taint-tracking queries that concern `ExternalStringKind` will not produce any results. Please ensure such queries contain an explicit import (`import semmle.python.security.strings.Untrusted`).
-* Added support for tainted f-strings.

change-notes/1.26/analysis-csharp.md

@@ -19,6 +19,8 @@ The following changes in version 1.26 affect C# analysis in all applications.
 ## Changes to code extraction
 
 * Partial method bodies are extracted. Previously, partial method bodies were skipped completely.
+* Inferring the lengths of implicitely sized arrays is fixed. Previously, multidimensional arrays were always extracted with the same length for
+each dimension. With the fix, the array sizes `2` and `1` are extracted for `new int[,]{{1},{2}}`. Previously `2` and `2` were extracted.
 
 ## Changes to libraries
 

change-notes/1.26/analysis-javascript.md

@@ -27,6 +27,7 @@
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Incomplete URL substring sanitization (`js/incomplete-url-substring-sanitization`) | More results | This query now recognizes additional URLs when the substring check is an inclusion check. |
+| Ambiguous HTML id attribute (`js/duplicate-html-id`) | Results no longer shown | Precision tag reduced to "low". The query is no longer run by default. |
 
 
 ## Changes to libraries

change-notes/1.26/analysis-python.md

@@ -0,0 +1,22 @@
+# Improvements to Python analysis
+
+The following changes in version 1.26 affect Python analysis in all applications.
+
+## General improvements
+
+
+## New queries
+
+| **Query**                   | **Tags**  | **Purpose**                                                        |
+|-----------------------------|-----------|--------------------------------------------------------------------|
+
+
+## Changes to existing queries
+
+| **Query**                  | **Expected impact**    | **Change**                                                       |
+|----------------------------|------------------------|------------------------------------------------------------------|
+
+
+## Changes to libraries
+
+* Added taint tracking support for string formatting through f-strings.

config/identical-files.json

@@ -325,6 +325,10 @@
     "csharp/ql/src/experimental/ir/implementation/raw/gvn/internal/ValueNumberingImports.qll",
     "csharp/ql/src/experimental/ir/implementation/unaliased_ssa/gvn/internal/ValueNumberingImports.qll"
   ],
+  "Inline Test Expectations": [
+    "cpp/ql/test/TestUtilities/InlineExpectationsTest.qll",
+    "python/ql/test/TestUtilities/InlineExpectationsTest.qll"
+  ],
   "XML": [
     "cpp/ql/src/semmle/code/cpp/XML.qll",
     "csharp/ql/src/semmle/code/csharp/XML.qll",

cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.cpp

@@ -0,0 +1,11 @@
+void test(char *arg1, int *arg2) {
+    if (arg1[0] == 'A') {
+        if (arg2 != NULL) { //maybe redundant
+            *arg2 = 42;
+        }
+    }
+    if (arg1[1] == 'B')
+    {
+        *arg2 = 54; //dereferenced without checking first
+    }
+}

cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.qhelp

@@ -0,0 +1,29 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>This rule finds comparisons of a function parameter to null that occur when in another path the parameter is dereferenced without a guard check.  It's
+likely either the check is not required and can be removed, or it should be added before the dereference
+so that a null pointer dereference does not occur.</p>
+</overview>
+
+<recommendation>
+<p>A check should be added to before the dereference, in a way that prevents a null pointer value from
+being dereferenced.  If it's clear that the pointer cannot be null, consider removing the check instead.</p>
+</recommendation>
+
+<example>
+<sample src="RedundantNullCheckParam.cpp" />
+</example>
+
+<references>
+<li>
+  <a href="https://www.owasp.org/index.php/Null_Dereference">
+    Null Dereference
+  </a> 
+</li>
+</references>
+
+</qhelp>

cpp/ql/src/experimental/Likely Bugs/RedundantNullCheckParam.ql

@@ -0,0 +1,56 @@
+/**
+ * @name Redundant null check or missing null check of parameter
+ * @description Checking a parameter for nullness in one path,
+ *              and not in another is likely to be a sign that either
+ *              the check can be removed, or added in the other case.
+ * @kind problem
+ * @id cpp/redundant-null-check-param
+ * @problem.severity recommendation
+ * @tags reliability
+ *       security
+ *       external/cwe/cwe-476
+ */
+
+import cpp
+
+predicate blockDominates(Block check, Block access) {
+  check.getLocation().getStartLine() <= access.getLocation().getStartLine() and
+  check.getLocation().getEndLine() >= access.getLocation().getEndLine()
+}
+
+predicate isCheckedInstruction(VariableAccess unchecked, VariableAccess checked) {
+  checked = any(VariableAccess va | va.getTarget() = unchecked.getTarget()) and
+  //Simple test if the first access in this code path is dereferenced
+  not dereferenced(checked) and
+  blockDominates(checked.getEnclosingBlock(), unchecked.getEnclosingBlock())
+}
+
+predicate candidateResultUnchecked(VariableAccess unchecked) {
+  not isCheckedInstruction(unchecked, _)
+}
+
+predicate candidateResultChecked(VariableAccess check, EqualityOperation eqop) {
+  //not dereferenced to check against pointer, not its pointed value
+  not dereferenced(check) and
+  //assert macros are not taken into account
+  not check.isInMacroExpansion() and
+  // is part of a comparison against some constant NULL
+  eqop.getAnOperand() = check and
+  eqop.getAnOperand() instanceof NullValue
+}
+
+from VariableAccess unchecked, VariableAccess check, EqualityOperation eqop, Parameter param
+where
+  // a dereference
+  dereferenced(unchecked) and
+  // for a function parameter
+  unchecked.getTarget() = param and
+  // this function parameter is not overwritten
+  count(param.getAnAssignment()) = 0 and
+  check.getTarget() = param and
+  // which is once checked
+  candidateResultChecked(check, eqop) and
+  // and which has not been checked before in this code path
+  candidateResultUnchecked(unchecked)
+select check, "This null check is redundant or there is a missing null check before $@ ", unchecked,
+  "where dereferencing happens"

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

@@ -23,7 +23,7 @@ class StdSequenceContainerConstructor extends Constructor, TaintFunction {
    */
   int getAValueTypeParameterIndex() {
     getParameter(result).getUnspecifiedType().(ReferenceType).getBaseType() =
-      getDeclaringType().getTemplateArgument(0) // i.e. the `T` of this `std::vector<T>`
+      getDeclaringType().getTemplateArgument(0).(Type).getUnspecifiedType() // i.e. the `T` of this `std::vector<T>`
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
@@ -33,6 +33,24 @@ class StdSequenceContainerConstructor extends Constructor, TaintFunction {
   }
 }
 
+/**
+ * The standard container function `data`.
+ */
+class StdSequenceContainerData extends TaintFunction {
+  StdSequenceContainerData() { this.hasQualifiedName("std", ["array", "vector"], "data") }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from container itself (qualifier) to return value
+    input.isQualifierObject() and
+    output.isReturnValueDeref()
+    or
+    // reverse flow from returned reference to the qualifier (for writes to
+    // `data`)
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
+  }
+}
+
 /**
  * The standard container functions `push_back` and `push_front`.
  */
@@ -70,6 +88,30 @@ class StdSequenceContainerFrontBack extends TaintFunction {
   }
 }
 
+/**
+ * The standard container function `assign`.
+ */
+class StdSequenceContainerAssign extends TaintFunction {
+  StdSequenceContainerAssign() {
+    this.hasQualifiedName("std", ["vector", "deque", "list", "forward_list"], "assign")
+  }
+
+  /**
+   * Gets the index of a parameter to this function that is a reference to the
+   * value type of the container.
+   */
+  int getAValueTypeParameterIndex() {
+    getParameter(result).getUnspecifiedType().(ReferenceType).getBaseType() =
+      getDeclaringType().getTemplateArgument(0).(Type).getUnspecifiedType() // i.e. the `T` of this `std::vector<T>`
+  }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from parameter to string itself (qualifier) and return value
+    input.isParameterDeref(getAValueTypeParameterIndex()) and
+    output.isQualifierObject()
+  }
+}
+
 /**
  * The standard container `swap` functions.
  */

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

@@ -8,15 +8,33 @@ class StdBasicString extends TemplateClass {
 }
 
 /**
- * The `std::string` functions `c_str` and  `data`.
+ * The `std::string` function `c_str`.
  */
 class StdStringCStr extends TaintFunction {
-  StdStringCStr() { this.hasQualifiedName("std", "basic_string", ["c_str", "data"]) }
+  StdStringCStr() { this.hasQualifiedName("std", "basic_string", "c_str") }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // flow from string itself (qualifier) to return value
     input.isQualifierObject() and
-    output.isReturnValue()
+    output.isReturnValueDeref()
+  }
+}
+
+/**
+ * The `std::string` function `data`.
+ */
+class StdStringData extends TaintFunction {
+  StdStringData() { this.hasQualifiedName("std", "basic_string", "data") }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from string itself (qualifier) to return value
+    input.isQualifierObject() and
+    output.isReturnValueDeref()
+    or
+    // reverse flow from returned reference to the qualifier (for writes to
+    // `data`)
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
   }
 }
 
@@ -53,17 +71,18 @@ class StdStringAppend extends TaintFunction {
    * Gets the index of a parameter to this function that is a string (or
    * character).
    */
-  int getAStringParameter() {
+  int getAStringParameterIndex() {
     getParameter(result).getType() instanceof PointerType or
     getParameter(result).getType() instanceof ReferenceType or
-    getParameter(result).getType() = getDeclaringType().getTemplateArgument(0) // i.e. `std::basic_string::CharT`
+    getParameter(result).getUnspecifiedType() =
+      getDeclaringType().getTemplateArgument(0).(Type).getUnspecifiedType() // i.e. `std::basic_string::CharT`
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // flow from string and parameter to string (qualifier) and return value
     (
       input.isQualifierObject() or
-      input.isParameterDeref(getAStringParameter())
+      input.isParameterDeref(getAStringParameterIndex())
     ) and
     (
       output.isQualifierObject() or
@@ -82,15 +101,16 @@ class StdStringAssign extends TaintFunction {
    * Gets the index of a parameter to this function that is a string (or
    * character).
    */
-  int getAStringParameter() {
+  int getAStringParameterIndex() {
     getParameter(result).getType() instanceof PointerType or
     getParameter(result).getType() instanceof ReferenceType or
-    getParameter(result).getType() = getDeclaringType().getTemplateArgument(0) // i.e. `std::basic_string::CharT`
+    getParameter(result).getUnspecifiedType() =
+      getDeclaringType().getTemplateArgument(0).(Type).getUnspecifiedType() // i.e. `std::basic_string::CharT`
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     // flow from parameter to string itself (qualifier) and return value
-    input.isParameterDeref(getAStringParameter()) and
+    input.isParameterDeref(getAStringParameterIndex()) and
     (
       output.isQualifierObject() or
       output.isReturnValueDeref()