Merge branch 'main' of github.com:github/codeql into python-support-pathlib

Author: yoff

.github/workflows/close-stale.yml

@@ -15,16 +15,16 @@ jobs:
     - uses: actions/stale@v3
       with:
         repo-token: ${{ secrets.GITHUB_TOKEN }}
-        stale-issue-message: 'This issue is stale because it has been open 14 days with no activity. Comment or remove the `stale` label in order to avoid having this issue closed in 7 days.'
+        stale-issue-message: 'This issue is stale because it has been open 14 days with no activity. Comment or remove the `Stale` label in order to avoid having this issue closed in 7 days.'
         close-issue-message: 'This issue was closed because it has been inactive for 7 days.'
         days-before-stale: 14
         days-before-close: 7
-        only-labels: question
-        
+        only-labels: awaiting-response
+
         # do not mark PRs as stale
         days-before-pr-stale: -1
         days-before-pr-close: -1
-        
+
         # Uncomment for dry-run
         # debug-only: true
         # operations-per-run: 1000

cpp/change-notes/2021-04-13-arithmetic-queries.md

@@ -0,0 +1,2 @@
+lgtm
+* The queries cpp/tainted-arithmetic, cpp/uncontrolled-arithmetic, and cpp/arithmetic-with-extreme-values have been improved to produce fewer false positives.

cpp/change-notes/2021-04-21-return-stack-allocated-object.md

@@ -0,0 +1,2 @@
+codescanning
+* The 'Pointer to stack object used as return value' (cpp/return-stack-allocated-object) query has been deprecated, and any uses should be replaced with `Returning stack-allocated memory` (cpp/return-stack-allocated-memory).

cpp/ql/src/Critical/ReturnStackAllocatedObject.ql

@@ -7,6 +7,8 @@
  * @tags reliability
  *       security
  *       external/cwe/cwe-562
+ * @deprecated This query is not suitable for production use and has been deprecated. Use
+ *             cpp/return-stack-allocated-memory instead.
  */
 
 import semmle.code.cpp.pointsto.PointsTo

cpp/ql/src/Likely Bugs/Memory Management/ReturnStackAllocatedMemory.ql

@@ -13,6 +13,7 @@
 
 import cpp
 import semmle.code.cpp.dataflow.EscapesTree
+import semmle.code.cpp.models.interfaces.PointerWrapper
 import semmle.code.cpp.dataflow.DataFlow
 
 /**
@@ -39,6 +40,10 @@ predicate hasNontrivialConversion(Expr e) {
     e instanceof ParenthesisExpr
   )
   or
+  // A smart pointer can be stack-allocated while the data it points to is heap-allocated.
+  // So we exclude such "conversions" from this predicate.
+  e = any(PointerWrapper wrapper).getAnUnwrapperFunction().getACallToThisFunction()
+  or
   hasNontrivialConversion(e.getConversion())
 }
 

cpp/ql/src/experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.c

@@ -0,0 +1,17 @@
+while(flagsLoop)
+{
+  ...
+  if(flagsIf) break;
+  ...
+}while(flagsLoop); // BAD: when exiting through `break`, it is possible to get into an eternal loop.
+...
+while(flagsLoop)
+{
+  ...
+  if(flagsIf) break;
+  ...
+} // GOOD: correct cycle
+...
+if(intA+intB) return 1; // BAD: possibly no comparison
+...
+if(intA+intB>intC) return 1; // GOOD: correct comparison

cpp/ql/src/experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>In some situations, after code refactoring, parts of the old constructs may remain. They are correctly accepted by the compiler, but can critically affect program execution. For example, if you switch from `do {...} while ();` to `while () {...}` forgetting to remove the old construct completely, you get `while(){...}while();` which may be vulnerable. These code snippets look suspicious and require the developer's attention.</p>
+
+
+</overview>
+<recommendation>
+
+<p>We recommend that you use more explicit code transformations.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates the erroneous and corrected sections of the code.</p>
+<sample src="InsufficientControlFlowManagementAfterRefactoringTheCode.c" />
+
+</example>
+<references>
+
+<li>
+  CWE Common Weakness Enumeration:
+  <a href="https://cwe.mitre.org/data/definitions/691.html"> CWE-691: Insufficient Control Flow Management</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql

@@ -0,0 +1,119 @@
+/**
+ * @name Errors After Refactoring
+ * @description --In some situations, after code refactoring, parts of the old constructs may remain.
+ *              --They are correctly accepted by the compiler, but can critically affect program execution.
+ *              --For example, if you switch from `do {...} while ();` to `while () {...}` with errors, you run the risk of running out of resources.
+ *              --These code snippets look suspicious and require the developer's attention.
+ * @kind problem
+ * @id cpp/errors-after-refactoring
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-691
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.HashCons
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/**
+ * Using `while` directly after the body of another` while`.
+ */
+class UsingWhileAfterWhile extends WhileStmt {
+  /**
+   * Using a loop call after another loop has finished running can result in an eternal loop.
+   * For example, perhaps as a result of refactoring, the `do ... while ()` loop was incorrectly corrected.
+   * Even in the case of deliberate use of such an expression, it is better to correct it.
+   */
+  UsingWhileAfterWhile() {
+    exists(WhileStmt wh1 |
+      wh1.getStmt().getAChild*().(BreakStmt).(ControlFlowNode).getASuccessor().getASuccessor() =
+        this and
+      hashCons(wh1.getCondition()) = hashCons(this.getCondition()) and
+      this.getStmt() instanceof EmptyStmt
+    )
+    or
+    exists(ForStmt fr1 |
+      fr1.getStmt().getAChild*().(BreakStmt).(ControlFlowNode).getASuccessor().getASuccessor() =
+        this and
+      hashCons(fr1.getCondition()) = hashCons(this.getCondition()) and
+      this.getStmt() instanceof EmptyStmt
+    )
+  }
+}
+
+/**
+ * Using arithmetic in a condition.
+ */
+class UsingArithmeticInComparison extends BinaryArithmeticOperation {
+  /**
+   * Using arithmetic operations in a comparison operation can be dangerous.
+   * For example, part of the comparison may have been lost as a result of refactoring.
+   * Even if you deliberately use such an expression, it is better to add an explicit comparison.
+   */
+  UsingArithmeticInComparison() {
+    this.getParent*() instanceof IfStmt and
+    not this.getAChild*().isConstant() and
+    not this.getParent*() instanceof Call and
+    not this.getParent*() instanceof AssignExpr and
+    not this.getParent*() instanceof ArrayExpr and
+    not this.getParent*() instanceof RemExpr and
+    not this.getParent*() instanceof AssignBitwiseOperation and
+    not this.getParent*() instanceof AssignArithmeticOperation and
+    not this.getParent*() instanceof EqualityOperation and
+    not this.getParent*() instanceof RelationalOperation
+  }
+
+  /** Holds when the expression is inside the loop body. */
+  predicate insideTheLoop() { exists(Loop lp | lp.getStmt().getAChild*() = this.getParent*()) }
+
+  /** Holds when the expression is used in binary operations. */
+  predicate workingWithValue() {
+    this.getParent*() instanceof BinaryBitwiseOperation or
+    this.getParent*() instanceof NotExpr
+  }
+
+  /** Holds when the expression contains a pointer. */
+  predicate workingWithPointer() {
+    this.getAChild*().getFullyConverted().getType() instanceof DerivedType
+  }
+
+  /** Holds when a null comparison expression exists. */
+  predicate compareWithZero() {
+    exists(Expr exp |
+      exp instanceof ComparisonOperation and
+      (
+        globalValueNumber(exp.getAChild*()) = globalValueNumber(this) or
+        hashCons(exp.getAChild*()) = hashCons(this)
+      ) and
+      (
+        exp.(ComparisonOperation).getLeftOperand().getValue() = "0" or
+        exp.(ComparisonOperation).getRightOperand().getValue() = "0"
+      )
+    )
+  }
+
+  /** Holds when a comparison expression exists. */
+  predicate compareWithOutZero() {
+    exists(Expr exp |
+      exp instanceof ComparisonOperation and
+      (
+        globalValueNumber(exp.getAChild*()) = globalValueNumber(this) or
+        hashCons(exp.getAChild*()) = hashCons(this)
+      )
+    )
+  }
+}
+
+from Expr exp
+where
+  exp instanceof UsingArithmeticInComparison and
+  not exp.(UsingArithmeticInComparison).workingWithValue() and
+  not exp.(UsingArithmeticInComparison).workingWithPointer() and
+  not exp.(UsingArithmeticInComparison).insideTheLoop() and
+  not exp.(UsingArithmeticInComparison).compareWithZero() and
+  exp.(UsingArithmeticInComparison).compareWithOutZero()
+  or
+  exists(WhileStmt wst | wst instanceof UsingWhileAfterWhile and exp = wst.getCondition())
+select exp, "this expression needs your attention"

cpp/ql/src/experimental/Security/CWE/CWE-788/AccessOfMemoryLocationAfterEndOfBufferUsingStrncat.ql

@@ -11,54 +11,32 @@
  */
 
 import cpp
+import semmle.code.cpp.models.implementations.Strcat
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 
 /**
- * A call to `strncat` of the form `strncat(buff, str, someExpr - strlen(buf))`, for some expression `someExpr` equal to `sizeof(buff)`.
+ * Holds if `call` is a call to `strncat` such that `sizeArg` and `destArg` are the size and
+ * destination arguments, respectively.
  */
-class WrongCallStrncat extends FunctionCall {
-  Expr leftsomeExpr;
-
-  WrongCallStrncat() {
-    this.getTarget().hasGlobalOrStdName("strncat") and
-    // the expression of the first argument in `strncat` and `strnlen` is identical
-    globalValueNumber(this.getArgument(0)) =
-      globalValueNumber(this.getArgument(2).(SubExpr).getRightOperand().(StrlenCall).getStringExpr()) and
-    // using a string constant often speaks of manually calculating the length of the required buffer.
-    (
-      not this.getArgument(1) instanceof StringLiteral and
-      not this.getArgument(1) instanceof CharLiteral
-    ) and
-    // for use in predicates
-    leftsomeExpr = this.getArgument(2).(SubExpr).getLeftOperand()
-  }
-
-  /**
-   * Holds if the left side of the expression `someExpr` equal to `sizeof(buf)`.
-   */
-  predicate isExpressionEqualSizeof() {
-    // the left side of the expression `someExpr` is `sizeof(buf)`.
-    globalValueNumber(this.getArgument(0)) =
-      globalValueNumber(leftsomeExpr.(SizeofExprOperator).getExprOperand())
-    or
-    // value of the left side of the expression `someExpr` equal  `sizeof(buf)` value, and `buf` is array.
-    leftsomeExpr.getValue().toInt() = this.getArgument(0).getType().getSize()
-  }
-
-  /**
-   * Holds if the left side of the expression `someExpr` equal to variable containing the length of the memory allocated for the buffer.
-   */
-  predicate isVariableEqualValueSizegBuffer() {
-    // the left side of expression `someExpr` is the variable that was used in the function of allocating memory for the buffer`.
-    exists(AllocationExpr alc |
-      leftsomeExpr.(VariableAccess).getTarget() =
-        alc.(FunctionCall).getArgument(0).(VariableAccess).getTarget()
-    )
-  }
+predicate interestringCallWithArgs(Call call, Expr sizeArg, Expr destArg) {
+  exists(StrcatFunction strcat |
+    strcat = call.getTarget() and
+    sizeArg = call.getArgument(strcat.getParamSize()) and
+    destArg = call.getArgument(strcat.getParamDest())
+  )
 }
 
-from WrongCallStrncat sc
+from FunctionCall call, Expr sizeArg, Expr destArg, SubExpr sub, int n
 where
-  sc.isExpressionEqualSizeof() or
-  sc.isVariableEqualValueSizegBuffer()
-select sc, "if the used buffer is full, writing out of the buffer is possible"
+  interestringCallWithArgs(call, sizeArg, destArg) and
+  // The destination buffer is an array of size n
+  destArg.getUnspecifiedType().(ArrayType).getSize() = n and
+  // The size argument is equivalent to a subtraction
+  globalValueNumber(sizeArg).getAnExpr() = sub and
+  // ... where the left side of the subtraction is the constant n
+  globalValueNumber(sub.getLeftOperand()).getAnExpr().getValue().toInt() = n and
+  // ... and the right side of the subtraction is a call to `strlen` where the argument is the
+  // destination buffer.
+  globalValueNumber(sub.getRightOperand()).getAnExpr().(StrlenCall).getStringExpr() =
+    globalValueNumber(destArg).getAnExpr()
+select call, "Possible out-of-bounds write due to incorrect size argument."

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

@@ -18,20 +18,20 @@ private class UniqueOrSharedPtr extends Class, PointerWrapper {
 }
 
 /** Any function that unwraps a pointer wrapper class to reveal the underlying pointer. */
-private class PointerWrapperDataFlow extends DataFlowFunction {
-  PointerWrapperDataFlow() {
+private class PointerWrapperFlow extends TaintFunction, DataFlowFunction {
+  PointerWrapperFlow() {
     this = any(PointerWrapper wrapper).getAnUnwrapperFunction() and
     not this.getUnspecifiedType() instanceof ReferenceType
   }
 
-  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
-    input.isQualifierAddress() and output.isReturnValue()
-    or
-    input.isQualifierObject() and output.isReturnValueDeref()
-    or
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     input.isReturnValueDeref() and
     output.isQualifierObject()
   }
+
+  override predicate hasDataFlow(FunctionInput input, FunctionOutput output) {
+    input.isQualifierObject() and output.isReturnValue()
+  }
 }
 
 /**