Merge branch 'main' into better-path-explanation-for-this-indirection

Author: MathiasVP

.github/workflows/docs-review.yml

@@ -26,4 +26,4 @@ jobs:
           PR_NUMBER: ${{ github.event.pull_request.number }}
         run: |
           gh pr comment "$PR_NUMBER" --repo "github/codeql" \
-            --body "Hello @github/docs-content-codeql: this PR is ready for docs review."
+            --body "Hello @github/docs-content-codeql - this PR is ready for docs review."

.github/workflows/generate-query-help-docs.yml

@@ -38,24 +38,39 @@ predicate isNonEscapingArgument(Expr escaped) {
   )
 }
 
+pragma[noinline]
+predicate callToMemsetWithRelevantVariable(
+  LocalVariable v, VariableAccess acc, FunctionCall call, MemsetFunction memset
+) {
+  not v.isStatic() and
+  // Reference-typed variables get special treatment in `variableAddressEscapesTree` so we leave them
+  // out of this query.
+  not v.getUnspecifiedType() instanceof ReferenceType and
+  call.getTarget() = memset and
+  acc = v.getAnAccess() and
+  // `v` escapes as the argument to `memset`
+  variableAddressEscapesTree(acc, call.getArgument(0).getFullyConverted())
+}
+
+pragma[noinline]
+predicate relevantVariable(LocalVariable v, FunctionCall call, MemsetFunction memset) {
+  exists(VariableAccess acc, VariableAccess anotherAcc |
+    callToMemsetWithRelevantVariable(v, acc, call, memset) and
+    // `v` is not only just used in the call to `memset`.
+    anotherAcc = v.getAnAccess() and
+    acc != anotherAcc and
+    not anotherAcc.isUnevaluated()
+  )
+}
+
 from FunctionCall call, LocalVariable v, MemsetFunction memset
 where
-  call.getTarget() = memset and
+  relevantVariable(v, call, memset) and
   not isFromMacroDefinition(call) and
-  // `v` escapes as the argument to `memset`
-  variableAddressEscapesTree(v.getAnAccess(), call.getArgument(0).getFullyConverted()) and
-  // ... and `v` doesn't escape anywhere else.
+  // `v` doesn't escape anywhere else.
   forall(Expr escape | variableAddressEscapesTree(v.getAnAccess(), escape) |
     isNonEscapingArgument(escape)
   ) and
-  not v.isStatic() and
-  // Reference-typed variables get special treatment in `variableAddressEscapesTree` so we leave them
-  // out of this query.
-  not v.getUnspecifiedType() instanceof ReferenceType and
-  // `v` is not only just used in the call to `memset`.
-  exists(Access acc |
-    acc = v.getAnAccess() and not call.getArgument(0).getAChild*() = acc and not acc.isUnevaluated()
-  ) and
   // There is no later use of `v`.
   not v.getAnAccess() = call.getASuccessor*() and
   // Not using the `-fno-builtin-memset` flag

cpp/ql/src/Security/CWE/CWE-014/MemsetMayBeDeleted.ql

@@ -0,0 +1,7 @@
+if(len<0) return 1;
+memset(dest, source, len); // GOOD: variable `len` checked before call
+
+...
+
+memset(dest, source, len); // BAD: variable `len` checked after call
+if(len<0) return 1;

cpp/ql/src/experimental/Security/CWE/CWE-020/LateCheckOfFunctionArgument.c

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Checking the function argument after calling the function itself. This situation looks suspicious and requires the attention of the developer. It may be necessary to add validation before calling the function</p>
+
+
+</overview>
+<recommendation>
+
+<p>We recommend checking before calling the function.</p>
+
+</recommendation>
+<example>
+<p>The following example demonstrates an erroneous and fixed use of function argument validation.</p>
+<sample src="LateCheckOfFunctionArgument.c" />
+
+</example>
+<references>
+
+<li>
+  CWE Common Weakness Enumeration:
+  <a href="https://cwe.mitre.org/data/definitions/20.html"> CWE-20: Improper Input Validation</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/experimental/Security/CWE/CWE-020/LateCheckOfFunctionArgument.qhelp

@@ -0,0 +1,66 @@
+/**
+ * @name Late Check Of Function Argument
+ * @description --Checking the function argument after calling the function itself.
+ *              --This situation looks suspicious and requires the attention of the developer.
+ *              --It may be necessary to add validation before calling the function.
+ * @kind problem
+ * @id cpp/late-check-of-function-argument
+ * @problem.severity warning
+ * @precision medium
+ * @tags correctness
+ *       security
+ *       external/cwe/cwe-20
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+
+/** Holds for a function `f` that has an argument at index `apos` used for positioning in a buffer. */
+predicate numberArgument(Function f, int apos) {
+  f.hasGlobalOrStdName("write") and apos = 2
+  or
+  f.hasGlobalOrStdName("read") and apos = 2
+  or
+  f.hasGlobalOrStdName("lseek") and apos = 1
+  or
+  f.hasGlobalOrStdName("memmove") and apos = 2
+  or
+  f.hasGlobalOrStdName("memset") and apos = 2
+  or
+  f.hasGlobalOrStdName("memcpy") and apos = 2
+  or
+  f.hasGlobalOrStdName("memcmp") and apos = 2
+  or
+  f.hasGlobalOrStdName("strncat") and apos = 2
+  or
+  f.hasGlobalOrStdName("strncpy") and apos = 2
+  or
+  f.hasGlobalOrStdName("strncmp") and apos = 2
+  or
+  f.hasGlobalOrStdName("snprintf") and apos = 1
+  or
+  f.hasGlobalOrStdName("strndup") and apos = 2
+}
+
+class IfCompareWithZero extends IfStmt {
+  IfCompareWithZero() { this.getCondition().(RelationalOperation).getAChild().getValue() = "0" }
+
+  Expr noZerroOperand() {
+    if this.getCondition().(RelationalOperation).getGreaterOperand().getValue() = "0"
+    then result = this.getCondition().(RelationalOperation).getLesserOperand()
+    else result = this.getCondition().(RelationalOperation).getGreaterOperand()
+  }
+}
+
+from FunctionCall fc, IfCompareWithZero ifc, int na
+where
+  numberArgument(fc.getTarget(), na) and
+  globalValueNumber(fc.getArgument(na)) = globalValueNumber(ifc.noZerroOperand()) and
+  dominates(fc, ifc) and
+  not exists(IfStmt ifc1 |
+    dominates(ifc1, fc) and
+    globalValueNumber(fc.getArgument(na)) = globalValueNumber(ifc1.getCondition().getAChild*())
+  )
+select fc,
+  "The value of argument '$@' appears to be checked after the call, rather than before it.",
+  fc.getArgument(na), fc.getArgument(na).toString()