Merge branch 'main' into unsafe-use-of-this-query

Author: MathiasVP

config/identical-files.json

@@ -358,6 +358,14 @@
     "cpp/ql/test/TestUtilities/InlineExpectationsTest.qll",
     "python/ql/test/TestUtilities/InlineExpectationsTest.qll"
   ],
+  "C++ ExternalAPIs": [
+    "cpp/ql/src/Security/CWE/CWE-020/ExternalAPIs.qll",
+    "cpp/ql/src/Security/CWE/CWE-020/ir/ExternalAPIs.qll"
+  ],
+  "C++ SafeExternalAPIFunction": [
+    "cpp/ql/src/Security/CWE/CWE-020/SafeExternalAPIFunction.qll",
+    "cpp/ql/src/Security/CWE/CWE-020/ir/SafeExternalAPIFunction.qll"
+  ],
   "XML": [
     "cpp/ql/src/semmle/code/cpp/XML.qll",
     "csharp/ql/src/semmle/code/csharp/XML.qll",
@@ -409,5 +417,12 @@
     "java/ql/src/Metrics/Files/CommentedOutCodeReferences.qhelp",
     "javascript/ql/src/Comments/CommentedOutCodeReferences.qhelp",
     "python/ql/src/Lexical/CommentedOutCodeReferences.qhelp"
+  ],
+  "IDE Contextual Queries": [
+    "cpp/ql/src/IDEContextual.qll",
+    "csharp/ql/src/IDEContextual.qll",
+    "java/ql/src/IDEContextual.qll",
+    "javascript/ql/src/IDEContextual.qll",
+    "python/ql/src/analysis/IDEContextual.qll"
   ]
 }

cpp/change-notes/2020-11-02-unused-local-variable.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Two issues causing the 'Unused local variable' query (`cpp/unused-local-variable`) to produce false positive results have been fixed.

cpp/ql/src/Best Practices/Unused Entities/UnusedLocals.ql

@@ -57,5 +57,12 @@ where
   not declarationHasSideEffects(v) and
   not exists(AsmStmt s | f = s.getEnclosingFunction()) and
   not v.getAnAttribute().getName() = "unused" and
-  not any(ErrorExpr e).getEnclosingFunction() = f // unextracted expr likely used `v`
+  not any(ErrorExpr e).getEnclosingFunction() = f and // unextracted expr may use `v`
+  not exists(
+    Literal l // this case can be removed when the `myFunction2( [obj](){} );` test case doesn't depend on this exclusion
+  |
+    l.getEnclosingFunction() = f and
+    not exists(l.getValue())
+  ) and
+  not any(ConditionDeclExpr cde).getEnclosingFunction() = f // this case can be removed when the `if (a = b; a)` test case doesn't depend on this exclusion
 select v, "Variable " + v.getName() + " is not used"

cpp/ql/src/IDEContextual.qll

@@ -0,0 +1,22 @@
+/**
+ * Provides shared predicates related to contextual queries in the code viewer.
+ */
+
+import semmle.files.FileSystem
+
+/**
+ * Returns the `File` matching the given source file name as encoded by the VS
+ * Code extension.
+ */
+cached
+File getFileBySourceArchiveName(string name) {
+  // The name provided for a file in the source archive by the VS Code extension
+  // has some differences from the absolute path in the database:
+  // 1. colons are replaced by underscores
+  // 2. there's a leading slash, even for Windows paths: "C:/foo/bar" ->
+  //    "/C_/foo/bar"
+  // 3. double slashes in UNC prefixes are replaced with a single slash
+  // We can handle 2 and 3 together by unconditionally adding a leading slash
+  // before replacing double slashes.
+  name = ("/" + result.getAbsolutePath().replaceAll(":", "_")).replaceAll("//", "/")
+}

cpp/ql/src/Likely Bugs/Leap Year/UncheckedReturnValueForTimeFunctions.ql

@@ -50,10 +50,12 @@ class SafeTimeGatheringFunction extends Function {
 class TimeConversionFunction extends Function {
   TimeConversionFunction() {
     this.getQualifiedName() =
-      ["FileTimeToSystemTime", "SystemTimeToFileTime", "SystemTimeToTzSpecificLocalTime",
-          "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime",
-          "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime",
-          "RtlTimeToSecondsSince1970", "_mkgmtime"]
+      [
+        "FileTimeToSystemTime", "SystemTimeToFileTime", "SystemTimeToTzSpecificLocalTime",
+        "SystemTimeToTzSpecificLocalTimeEx", "TzSpecificLocalTimeToSystemTime",
+        "TzSpecificLocalTimeToSystemTimeEx", "RtlLocalTimeToSystemTime",
+        "RtlTimeToSecondsSince1970", "_mkgmtime"
+      ]
   }
 }
 

cpp/ql/src/Security/CWE/CWE-020/CountUntrustedDataToExternalAPI.qhelp

@@ -0,0 +1,48 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Using unsanitized untrusted data in an external API can cause a variety of security issues. This query reports 
+all external APIs that are used with untrusted data, along with how frequently the API is used, and how many 
+unique sources of untrusted data flow to this API. This query is designed primarily to help identify which APIs 
+may be relevant for security analysis of this application.</p>
+
+<p>An external API is defined as a call to a function that is not defined in the source code, and is not
+modeled as a taint step in the default taint library. External APIs may be from the C++ standard library,
+third party dependencies or from internal dependencies. The query will report the function name, along with
+either <code>[param x]</code>, where <code>x</code> indicates the position of the parameter receiving the
+untrusted data or <code>[qualifier]</code> indicating the untrusted data is used as the qualifier to the
+function call.</p>
+
+</overview>
+<recommendation>
+
+<p>For each result:</p>
+
+<ul>
+  <li>If the result highlights a known sink, no action is required.</li>
+  <li>If the result highlights an unknown sink for a problem, then add modeling for the sink to the relevant query.</li>
+  <li>If the result represents a call to an external API which transfers taint, add the appropriate modeling, and 
+  re-run the query to determine what new results have appeared due to this additional modeling.</li>
+</ul>
+
+<p>Otherwise, the result is likely uninteresting. Custom versions of this query can extend the <code>SafeExternalAPIFunction</code> 
+class to exclude known safe external APIs from future analysis.</p>
+
+</recommendation>
+<example>
+
+<p>If the query were to return the API <code>fputs [param 1]</code> 
+then we should first consider whether this a security relevant sink. In this case, this is writing to a <code>FILE*</code>, so we should 
+consider whether this is an XSS sink. If it is, we should confirm that it is handled by the XSS query.</p>
+
+<p>If the query were to return the API <code>strcat [param 1]</code>, then this should be
+reviewed as a possible taint step, because tainted data would flow from the 1st argument to the 0th argument of the call.</p>
+
+<p>Note that both examples are correctly handled by the standard taint tracking library and XSS query.</p>
+</example>
+<references>
+
+</references>
+</qhelp>

cpp/ql/src/Security/CWE/CWE-020/CountUntrustedDataToExternalAPI.ql

@@ -0,0 +1,17 @@
+/**
+ * @name Frequency counts for external APIs that are used with untrusted data
+ * @description This reports the external APIs that are used with untrusted data, along with how
+ *              frequently the API is called, and how many unique sources of untrusted data flow
+ *              to it.
+ * @id cpp/count-untrusted-data-external-api
+ * @kind table
+ * @tags security external/cwe/cwe-20
+ */
+
+import cpp
+import ExternalAPIs
+
+from ExternalAPIUsedWithUntrustedData externalAPI
+select externalAPI, count(externalAPI.getUntrustedDataNode()) as numberOfUses,
+  externalAPI.getNumberOfUntrustedSources() as numberOfUntrustedSources order by
+    numberOfUntrustedSources desc

cpp/ql/src/Security/CWE/CWE-020/ExternalAPISinkExample.cpp

@@ -0,0 +1,13 @@
+#include <cstdio>
+
+void do_get(FILE* request, FILE* response) {
+  char page[1024];
+  fgets(page, 1024, request);
+
+  char buffer[1024];
+  strcat(buffer, "The page \"");
+  strcat(buffer, page);
+  strcat(buffer, "\" was not found.");
+
+  fputs(buffer, response);
+}

cpp/ql/src/Security/CWE/CWE-020/ExternalAPITaintStepExample.cpp

@@ -0,0 +1,13 @@
+#include <cstdio>
+
+void do_get(FILE* request, FILE* response) {
+  char user_id[1024];
+  fgets(user_id, 1024, request);
+
+  char buffer[1024];
+  strcat(buffer, "SELECT * FROM user WHERE user_id='");
+  strcat(buffer, user_id);
+  strcat(buffer, "'");
+
+  // ...
+}

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

@@ -0,0 +1,50 @@
+/**
+ * Definitions for reasoning about untrusted data used in APIs defined outside the
+ * database.
+ */
+
+private import cpp
+private import semmle.code.cpp.models.interfaces.DataFlow
+private import semmle.code.cpp.models.interfaces.Taint
+import ExternalAPIsSpecific
+
+/** A node representing untrusted data being passed to an external API. */
+class UntrustedExternalAPIDataNode extends ExternalAPIDataNode {
+  UntrustedExternalAPIDataNode() { any(UntrustedDataToExternalAPIConfig c).hasFlow(_, this) }
+
+  /** Gets a source of untrusted data which is passed to this external API data node. */
+  DataFlow::Node getAnUntrustedSource() {
+    any(UntrustedDataToExternalAPIConfig c).hasFlow(result, this)
+  }
+}
+
+private newtype TExternalAPI =
+  TExternalAPIParameter(Function f, int index) {
+    exists(UntrustedExternalAPIDataNode n |
+      f = n.getExternalFunction() and
+      index = n.getIndex()
+    )
+  }
+
+/** An external API which is used with untrusted data. */
+class ExternalAPIUsedWithUntrustedData extends TExternalAPI {
+  /** Gets a possibly untrusted use of this external API. */
+  UntrustedExternalAPIDataNode getUntrustedDataNode() {
+    this = TExternalAPIParameter(result.getExternalFunction(), result.getIndex())
+  }
+
+  /** Gets the number of untrusted sources used with this external API. */
+  int getNumberOfUntrustedSources() {
+    result = strictcount(getUntrustedDataNode().getAnUntrustedSource())
+  }
+
+  /** Gets a textual representation of this element. */
+  string toString() {
+    exists(Function f, int index, string indexString |
+      if index = -1 then indexString = "qualifier" else indexString = "param " + index
+    |
+      this = TExternalAPIParameter(f, index) and
+      result = f.toString() + " [" + indexString + "]"
+    )
+  }
+}