Merge branch 'main' into ts4

Author: erik-krogh

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/dataflow/internal/DataFlowUtil.qll

@@ -484,6 +484,17 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   // Expr -> Expr
   exprToExprStep_nocfg(nodeFrom.asExpr(), nodeTo.asExpr())
   or
+  // Assignment -> LValue post-update node
+  //
+  // This is used for assignments whose left-hand side is not a variable
+  // assignment or a storeStep but is still modeled by other means. It could be
+  // a call to `operator*` or `operator[]` where taint should flow to the
+  // post-update node of the qualifier.
+  exists(AssignExpr assign |
+    nodeFrom.asExpr() = assign and
+    nodeTo.(PostUpdateNode).getPreUpdateNode().asExpr() = assign.getLValue()
+  )
+  or
   // Node -> FlowVar -> VariableAccess
   exists(FlowVar var |
     (

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

@@ -82,6 +82,19 @@ predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeT
   exprToDefinitionByReferenceStep(nodeFrom.asExpr(), nodeTo.asDefiningArgument())
   or
   exprToPartialDefinitionStep(nodeFrom.asExpr(), nodeTo.asPartialDefinition())
+  or
+  // Reverse taint: taint that flows from the post-update node of a reference
+  // returned by a function call, back into the qualifier of that function.
+  // This allows taint to flow 'in' through references returned by a modeled
+  // function such as `operator[]`.
+  exists(TaintFunction f, Call call, FunctionInput inModel, FunctionOutput outModel |
+    call.getTarget() = f and
+    inModel.isReturnValueDeref() and
+    outModel.isQualifierObject() and
+    f.hasTaintFlow(inModel, outModel) and
+    nodeFrom.(DataFlow::PostUpdateNode).getPreUpdateNode().asExpr() = call and
+    nodeTo.asDefiningArgument() = call.getQualifier()
+  )
 }
 
 /**

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

@@ -14,10 +14,7 @@ import semmle.code.cpp.models.interfaces.Taint
  */
 class StdSequenceContainerConstructor extends Constructor, TaintFunction {
   StdSequenceContainerConstructor() {
-    this.getDeclaringType().hasQualifiedName("std", "vector") or
-    this.getDeclaringType().hasQualifiedName("std", "deque") or
-    this.getDeclaringType().hasQualifiedName("std", "list") or
-    this.getDeclaringType().hasQualifiedName("std", "forward_list")
+    this.getDeclaringType().hasQualifiedName("std", ["vector", "deque", "list", "forward_list"])
   }
 
   /**
@@ -26,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) {
@@ -36,16 +33,32 @@ 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`.
  */
 class StdSequenceContainerPush extends TaintFunction {
   StdSequenceContainerPush() {
     this.hasQualifiedName("std", "vector", "push_back") or
-    this.hasQualifiedName("std", "deque", "push_back") or
-    this.hasQualifiedName("std", "deque", "push_front") or
-    this.hasQualifiedName("std", "list", "push_back") or
-    this.hasQualifiedName("std", "list", "push_front") or
+    this.hasQualifiedName("std", "deque", ["push_back", "push_front"]) or
+    this.hasQualifiedName("std", "list", ["push_back", "push_front"]) or
     this.hasQualifiedName("std", "forward_list", "push_front")
   }
 
@@ -61,14 +74,10 @@ class StdSequenceContainerPush extends TaintFunction {
  */
 class StdSequenceContainerFrontBack extends TaintFunction {
   StdSequenceContainerFrontBack() {
-    this.hasQualifiedName("std", "array", "front") or
-    this.hasQualifiedName("std", "array", "back") or
-    this.hasQualifiedName("std", "vector", "front") or
-    this.hasQualifiedName("std", "vector", "back") or
-    this.hasQualifiedName("std", "deque", "front") or
-    this.hasQualifiedName("std", "deque", "back") or
-    this.hasQualifiedName("std", "list", "front") or
-    this.hasQualifiedName("std", "list", "back") or
+    this.hasQualifiedName("std", "array", ["front", "back"]) or
+    this.hasQualifiedName("std", "vector", ["front", "back"]) or
+    this.hasQualifiedName("std", "deque", ["front", "back"]) or
+    this.hasQualifiedName("std", "list", ["front", "back"]) or
     this.hasQualifiedName("std", "forward_list", "front")
   }
 
@@ -79,16 +88,36 @@ 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.
  */
 class StdSequenceContainerSwap extends TaintFunction {
   StdSequenceContainerSwap() {
-    this.hasQualifiedName("std", "array", "swap") or
-    this.hasQualifiedName("std", "vector", "swap") or
-    this.hasQualifiedName("std", "deque", "swap") or
-    this.hasQualifiedName("std", "list", "swap") or
-    this.hasQualifiedName("std", "forward_list", "swap")
+    this.hasQualifiedName("std", ["array", "vector", "deque", "list", "forward_list"], "swap")
   }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
@@ -100,3 +129,22 @@ class StdSequenceContainerSwap extends TaintFunction {
     output.isQualifierObject()
   }
 }
+
+/**
+ * The standard container functions `at` and `operator[]`.
+ */
+class StdSequenceContainerAt extends TaintFunction {
+  StdSequenceContainerAt() {
+    this.hasQualifiedName("std", ["vector", "array", "deque"], ["at", "operator[]"])
+  }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from qualifier to referenced return value
+    input.isQualifierObject() and
+    output.isReturnValueDeref()
+    or
+    // reverse flow from returned reference to the qualifier
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
+  }
+}