Merge remote-tracking branch 'upstream/main' into main

Author: haby0

.github/workflows/codeql-analysis.yml

@@ -2,7 +2,13 @@ name: "Code scanning - action"
 
 on:
   push:
+    branches:
+      - main
+      - 'rc/*'
   pull_request:
+    branches:
+      - main
+      - 'rc/*'
   schedule:
     - cron: '0 9 * * 1'
 
@@ -14,16 +20,7 @@ jobs:
     steps:
     - name: Checkout repository
       uses: actions/checkout@v2
-      with:
-        # We must fetch at least the immediate parents so that if this is
-        # a pull request then we can checkout the head.
-        fetch-depth: 2
-
-    # If this run was triggered by a pull request event, then checkout
-    # the head of the pull request instead of the merge commit.
-    - run: git checkout HEAD^2
-      if: ${{ github.event_name == 'pull_request' }}
-      
+
     # Initializes the CodeQL tools for scanning.
     - name: Initialize CodeQL
       uses: github/codeql-action/init@v1

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

@@ -50,7 +50,7 @@ jobs:
       uses: ammaraskar/sphinx-action@8b4f60114d7fd1faeba1a712269168508d4750d2 # v0.4
       with:
         docs-folder: "query-help/"
-        pre-build-command: "python -m pip install --upgrade recommonmark"
+        pre-build-command: "python -m pip install --upgrade recommonmark && python -m pip install --upgrade sphinx-markdown-tables"
         build-command: "sphinx-build -b dirhtml . _build" 
     - name: Upload HTML artifacts
       uses: actions/upload-artifact@v2

.gitignore

@@ -17,6 +17,9 @@
 # Byte-compiled python files
 *.pyc
 
+# python virtual environment folder 
+.venv/
+
 # It's useful (though not required) to be able to unpack codeql in the ql checkout itself
 /codeql/
 

cpp/change-notes/2020-02-04-unsigned-difference-expression-compared-zero.md

@@ -0,0 +1,2 @@
+lgtm
+* A new query (`cpp/unsigned-difference-expression-compared-zero`) is run but not yet displayed on LGTM. The query finds unsigned subtractions used in relational comparisons with the value 0. This query was originally submitted as an experimental query by @ihsinme in https://github.com/github/codeql/pull/4745.

cpp/config/suites/cpp/correctness

@@ -10,6 +10,7 @@
 + semmlecode-cpp-queries/Likely Bugs/Underspecified Functions/MistypedFunctionArguments.ql: /Correctness/Dangerous Conversions
 + semmlecode-cpp-queries/Security/CWE/CWE-253/HResultBooleanConversion.ql: /Correctness/Dangerous Conversions
 + semmlecode-cpp-queries/Likely Bugs/OO/UnsafeUseOfThis.ql: /Correctness/Dangerous Conversions
++ semmlecode-cpp-queries/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql: /Correctness/Dangerous Conversions
   # Consistent Use
 + semmlecode-cpp-queries/Critical/ReturnValueIgnored.ql: /Correctness/Consistent Use
 + semmlecode-cpp-queries/Likely Bugs/InconsistentCheckReturnNull.ql: /Correctness/Consistent Use

cpp/ql/src/Security/CWE/CWE-020/ExternalAPIsSpecific.qll

@@ -46,7 +46,7 @@ class UntrustedDataToExternalAPIConfig extends TaintTracking::Configuration {
   UntrustedDataToExternalAPIConfig() { this = "UntrustedDataToExternalAPIConfig" }
 
   override predicate isSource(DataFlow::Node source) {
-    exists(RemoteFlowFunction remoteFlow |
+    exists(RemoteFlowSourceFunction remoteFlow |
       remoteFlow = source.asExpr().(Call).getTarget() and
       remoteFlow.hasRemoteFlowSource(_, _)
     )

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.c

@@ -0,0 +1,5 @@
+unsigned limit = get_limit();
+unsigned total = 0;
+while (limit - total > 0) { // wrong: if `total` is greater than `limit` this will underflow and continue executing the loop.
+  total += get_data();
+}

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.qhelp

@@ -0,0 +1,31 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+This rule finds relational comparisons between the result of an unsigned subtraction and the value <code>0</code>.
+Such comparisons are likely to be wrong as the value of an unsigned subtraction can never be negative. So the
+relational comparison ends up checking whether the result of the subtraction is equal to <code>0</code>.
+This is probably not what the programmer intended.
+</p>
+</overview>
+<recommendation>
+
+<p>If a relational comparison is intended, consider casting the result of the subtraction to a signed type.
+   If the intention was to test for equality, consider replacing the relational comparison with an equality test.
+</p>
+
+</recommendation>
+<example>
+<sample src="UnsignedDifferenceExpressionComparedZero.c" />
+
+</example>
+<references>
+
+<li>SEI CERT C Coding Standard:
+<a href="https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules">INT02-C. Understand integer conversion rules</a>.
+</li>
+
+</references>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql

@@ -0,0 +1,49 @@
+/**
+ * @name Unsigned difference expression compared to zero
+ * @description A subtraction with an unsigned result can never be negative. Using such an expression in a relational comparison with `0` is likely to be wrong.
+ * @kind problem
+ * @id cpp/unsigned-difference-expression-compared-zero
+ * @problem.severity warning
+ * @precision medium
+ * @tags security
+ *       correctness
+ *       external/cwe/cwe-191
+ */
+
+import cpp
+import semmle.code.cpp.commons.Exclusions
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+import semmle.code.cpp.controlflow.Guards
+
+/** Holds if `sub` is guarded by a condition which ensures that `left >= right`. */
+pragma[noinline]
+predicate isGuarded(SubExpr sub, Expr left, Expr right) {
+  exists(GuardCondition guard |
+    guard.controls(sub.getBasicBlock(), true) and
+    guard.ensuresLt(left, right, 0, sub.getBasicBlock(), false)
+  )
+}
+
+/** Holds if `sub` will never be negative. */
+predicate nonNegative(SubExpr sub) {
+  not exprMightOverflowNegatively(sub.getFullyConverted())
+  or
+  // The subtraction is guarded by a check of the form `left >= right`.
+  exists(GVN left, GVN right |
+    // This is basically a poor man's version of a directional unbind operator.
+    strictcount([left, globalValueNumber(sub.getLeftOperand())]) = 1 and
+    strictcount([right, globalValueNumber(sub.getRightOperand())]) = 1 and
+    isGuarded(sub, left.getAnExpr(), right.getAnExpr())
+  )
+}
+
+from RelationalOperation ro, SubExpr sub
+where
+  not isFromMacroDefinition(ro) and
+  not isFromMacroDefinition(sub) and
+  ro.getLesserOperand().getValue().toInt() = 0 and
+  ro.getGreaterOperand() = sub and
+  sub.getFullyConverted().getUnspecifiedType().(IntegralType).isUnsigned() and
+  not nonNegative(sub)
+select ro, "Unsigned subtraction can never be negative."

cpp/ql/src/Security/CWE/CWE-457/InitializationFunctions.qll

@@ -353,7 +353,9 @@ class InitializationFunction extends Function {
           // Destination range is zeroed out on failure, assuming first two parameters are valid
           "memcpy_s",
           // This zeroes the memory unconditionally
-          "SeCreateAccessState"
+          "SeCreateAccessState",
+          // Argument initialization is optional, but always succeeds
+          "KeGetCurrentProcessorNumberEx"
         ]
     )
   }