From fb824ac052f2e2e26bff7805fe8da2952dad34d2 Mon Sep 17 00:00:00 2001
From: Kevin Backhouse <kevinbackhouse@github.com>
Date: Thu, 8 Jul 2021 16:50:55 +0100
Subject: [PATCH] Add custom CodeQL query for null iterator deref.

---
 .../codeql-queries/exiv2-code-scanning.qls    |  3 ++
 .../null_iterator_deref.qhelp                 | 23 ++++++++++
 .../exiv2-cpp-queries/null_iterator_deref.ql  | 46 +++++++++++++++++++
 .../exiv2-cpp-queries/qlpack.yml              |  4 ++
 .github/codeql/codeql-config.yml              |  5 ++
 .github/workflows/codeql-analysis.yml         |  1 +
 6 files changed, 82 insertions(+)
 create mode 100644 .github/codeql-queries/exiv2-code-scanning.qls
 create mode 100644 .github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.qhelp
 create mode 100644 .github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.ql
 create mode 100644 .github/codeql-queries/exiv2-cpp-queries/qlpack.yml
 create mode 100644 .github/codeql/codeql-config.yml

diff --git a/.github/codeql-queries/exiv2-code-scanning.qls b/.github/codeql-queries/exiv2-code-scanning.qls
new file mode 100644
index 0000000000..c371ed848c
--- /dev/null
+++ b/.github/codeql-queries/exiv2-code-scanning.qls
@@ -0,0 +1,3 @@
+# Reusing existing QL Pack
+- import: codeql-suites/cpp-code-scanning.qls
+  from: codeql-cpp
diff --git a/.github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.qhelp b/.github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.qhelp
new file mode 100644
index 0000000000..3020d2c1e9
--- /dev/null
+++ b/.github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.qhelp
@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+        A C++ iterator is a lot like a C pointer: if you dereference it without
+        first checking that it's valid then it can cause a crash.
+        </p>
+    </overview>
+    <recommendation>
+        <p>
+        Always check that the iterator is valid before derefencing it.
+        </p>
+    </recommendation>
+    <example>
+        <p>
+        <a href="https://github.com/Exiv2/exiv2/issues/1763">Issue 1763</a> was caused by
+        <a href="https://github.com/Exiv2/exiv2/blob/9b3ed3f9564b4ea51b43c78671435bde6b862e08/src/canonmn_int.cpp#L2755">this
+        dereference</a> of the iterator <tt>pos</tt>.
+        The bug was <a href="https://github.com/Exiv2/exiv2/pull/1767">fixed</a> by not dereferencing
+        <tt>pos</tt> if <tt>pos == metadata->end()</tt>.
+        </p>
+    </example>
+</qhelp>
diff --git a/.github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.ql b/.github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.ql
new file mode 100644
index 0000000000..cb26f21343
--- /dev/null
+++ b/.github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.ql
@@ -0,0 +1,46 @@
+/**
+ * @name NULL iterator deref
+ * @description Dereferencing an iterator without checking that it's valid could cause a crash.
+ * @kind problem
+ * @id cpp/null-iterator-deref
+ * @tags security
+ *       external/cwe/cwe-476
+ */
+
+import cpp
+import semmle.code.cpp.controlflow.Guards
+import semmle.code.cpp.dataflow.DataFlow
+
+// Holds if `cond` is a condition like `use == table.end()`.
+// `eq_is_true` is `true` for `==`, `false` for '!=`.
+// Note: the `==` is actually an overloaded `operator==`.
+predicate end_condition(GuardCondition cond, Expr use, FunctionCall endCall, boolean eq_is_true) {
+  exists(FunctionCall eq |
+    exists(string opName | eq.getTarget().getName() = opName |
+      opName = "operator==" and eq_is_true = true
+      or
+      opName = "operator!=" and eq_is_true = false
+    ) and
+    DataFlow::localExprFlow(use, eq.getAnArgument()) and
+    DataFlow::localExprFlow(endCall, eq.getAnArgument()) and
+    endCall.getTarget().getName() = "end" and
+    DataFlow::localExprFlow(eq, cond)
+  )
+}
+
+from FunctionCall call, Expr use
+where
+  call.getTarget().getName() = "findKey" and
+  DataFlow::localExprFlow(call, use) and
+  use != call and
+  not use.(AssignExpr).getRValue() = call and
+  not end_condition(_, use, _, _) and
+  not exists(
+    Expr cond_use, FunctionCall endCall, GuardCondition cond, BasicBlock block, boolean branch
+  |
+    end_condition(cond, cond_use, endCall, branch) and
+    DataFlow::localExprFlow(call, cond_use) and
+    cond.controls(block, branch.booleanNot()) and
+    block.contains(use)
+  )
+select call, "Iterator returned by findKey might cause a null deref $@.", use, "here"
diff --git a/.github/codeql-queries/exiv2-cpp-queries/qlpack.yml b/.github/codeql-queries/exiv2-cpp-queries/qlpack.yml
new file mode 100644
index 0000000000..2a058c5b0e
--- /dev/null
+++ b/.github/codeql-queries/exiv2-cpp-queries/qlpack.yml
@@ -0,0 +1,4 @@
+name: exiv2-cpp-queries
+version: 0.0.0
+libraryPathDependencies: codeql-cpp
+suites: exiv2-cpp-suite
diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml
new file mode 100644
index 0000000000..ea1ab97ceb
--- /dev/null
+++ b/.github/codeql/codeql-config.yml
@@ -0,0 +1,5 @@
+name: "Exiv2 CodeQL config"
+
+queries:
+    - uses: ./.github/codeql-queries/exiv2-code-scanning.qls
+    - uses: ./.github/codeql-queries/exiv2-cpp-queries
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index 8297ccb0b9..8783952ca9 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -35,6 +35,7 @@ jobs:
       uses: github/codeql-action/init@v1
       with:
         languages: ${{ matrix.language }}
+        config-file: .github/codeql/codeql-config.yml
         # If you wish to specify custom queries, you can do so here or in a config file.
         # By default, queries listed here will override any specified in a config file.
         # Prefix the list here with "+" to use these queries and those in the config file.