Merge branch 'Exiv2:0.27-maintenance' into 0.27-maintenance

.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

.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>

.github/codeql-queries/exiv2-cpp-queries/null_iterator_deref.ql

@@ -0,0 +1,47 @@
+/**
+ * @name NULL iterator deref
+ * @description Dereferencing an iterator without checking that it's valid could cause a crash.
+ * @kind problem
+ * @problem.severity warning
+ * @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"

.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

.github/codeql-queries/exiv2-cpp-queries/unsafe_vector_access.qhelp

@@ -0,0 +1,30 @@
+<!DOCTYPE qhelp SYSTEM "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+        The <a href="https://en.cppreference.com/w/cpp/container/vector/operator_at"><tt>operator[]</tt></a> method of <a href="https://en.cppreference.com/w/cpp/container/vector"><tt>std::vector</tt></a> does not do any bounds checking on the index. It is safer to use the <a href="https://en.cppreference.com/w/cpp/container/vector/at"><tt>at()</tt></a> method, which does do bounds checking.
+        </p>
+    </overview>
+    <recommendation>
+        <p>
+        Use the <a href="https://en.cppreference.com/w/cpp/container/vector/at"><tt>at()</tt></a> method, rather than <a href="https://en.cppreference.com/w/cpp/container/vector/operator_at"><tt>operator[]</tt></a>.
+        </p>
+        <p>
+          Some uses of <a href="https://en.cppreference.com/w/cpp/container/vector/operator_at"><tt>operator[]</tt></a> are safe because they are protected by a bounds check. The query recognises the following safe coding patterns:
+        </p>
+        <ul>
+          <li><tt>if (!x.empty()) { ...x[0]... }</tt></li>
+          <li><tt>if (x.length()) { ...x[0]... }</tt></li>
+          <li><tt>if (x.size() > 2) { ...x[2]... }</tt></li>
+          <li><tt>if (x.size() == 2) { ...x[1]... }</tt></li>
+          <li><tt>if (x.size() != 0) { ...x[0]... }</tt></li>
+          <li><tt>if (i < x.size()) { ... x[i] ... }</tt></li>
+          <li><tt>if (!x.empty()) { ... x[x.size() - 1] ... }</tt></li>
+        </ul>
+    </recommendation>
+    <example>
+        <p>
+          <a href="https://github.com/Exiv2/exiv2/issues/1706">#1706</a> was caused by a lack of bounds-checking on <a href="https://github.com/Exiv2/exiv2/blob/15098f4ef50cc721ad0018218acab2ff06e60beb/include/exiv2/value.hpp#L1639">this array access</a>. The bug was <a href="https://github.com/Exiv2/exiv2/pull/1735">fixed</a> calling the <a href="https://en.cppreference.com/w/cpp/container/vector/at"><tt>at()</tt></a> method instead.
+        </p>
+    </example>
+</qhelp>

.github/codeql-queries/exiv2-cpp-queries/unsafe_vector_access.ql

@@ -0,0 +1,181 @@
+/**
+ * @name Unsafe vector access
+ * @description std::vector::operator[] does not do any runtime
+ *              bounds-checking, so it is safer to use std::vector::at()
+ * @kind problem
+ * @problem.severity warning
+ * @id cpp/unsafe-vector-access
+ * @tags security
+ *       external/cwe/cwe-125
+ */
+
+import cpp
+import semmle.code.cpp.controlflow.Guards
+import semmle.code.cpp.dataflow.DataFlow
+import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
+import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+import semmle.code.cpp.valuenumbering.HashCons
+
+// A call to `operator[]`.
+class ArrayIndexCall extends FunctionCall {
+  ClassTemplateInstantiation ti;
+  TemplateClass tc;
+
+  ArrayIndexCall() {
+    this.getTarget().getName() = "operator[]" and
+    ti = this.getQualifier().getType().getUnspecifiedType() and
+    tc = ti.getTemplate() and
+    tc.getSimpleName() != "map" and
+    tc.getSimpleName() != "match_results"
+  }
+
+  ClassTemplateInstantiation getClassTemplateInstantiation() { result = ti }
+
+  TemplateClass getTemplateClass() { result = tc }
+}
+
+// A call to `size` or `length`.
+class SizeCall extends FunctionCall {
+  string fname;
+
+  SizeCall() {
+    fname = this.getTarget().getName() and
+    (fname = "size" or fname = "length")
+  }
+}
+
+// `x[i]` is safe if `x` is a `std::array` (fixed-size array)
+// and `i` within the bounds of the array.
+predicate indexK_with_fixedarray(ClassTemplateInstantiation t, ArrayIndexCall call) {
+  t = call.getClassTemplateInstantiation() and
+  exists(Expr idx |
+    t.getSimpleName() = "array" and
+    idx = call.getArgument(0) and
+    lowerBound(idx) >= 0 and
+    upperBound(idx) < t.getTemplateArgument(1).(Literal).getValue().toInt()
+  )
+}
+
+// Holds if `cond` is a Boolean condition that checks the size of
+// the array. It handles the following code patterns:
+//
+// 1.  `if (!x.empty()) { ... }`
+// 2.  `if (x.length()) { ... }`
+// 3.  `if (x.size() > 2) { ... }`
+// 4.  `if (x.size() == 2) { ... }`
+// 5.  `if (x.size() != 0) { ... }`
+//
+// If it safe to assume that `x.size() >= minsize` on the exit `branch`.
+predicate minimum_size_cond(Expr cond, Expr arrayExpr, int minsize, boolean branch) {
+  // `if (!x.empty()) { ...x[0]... }`
+  exists(FunctionCall emptyCall |
+    cond = emptyCall and
+    arrayExpr = emptyCall.getQualifier() and
+    emptyCall.getTarget().getName() = "empty" and
+    minsize = 1 and
+    branch = false
+  )
+  or
+  // `if (x.length()) { ...x[0]... }`
+  exists(SizeCall sizeCall |
+    cond = sizeCall and
+    arrayExpr = sizeCall.getQualifier() and
+    minsize = 1 and
+    branch = true
+  )
+  or
+  // `if (x.size() > 2) { ...x[2]... }`
+  exists(SizeCall sizeCall, Expr k, RelationStrictness strict |
+    arrayExpr = sizeCall.getQualifier() and
+    relOpWithSwapAndNegate(cond, sizeCall, k, Greater(), strict, branch)
+  |
+    strict = Strict() and minsize = 1 + k.getValue().toInt()
+    or
+    strict = Nonstrict() and minsize = k.getValue().toInt()
+  )
+  or
+  // `if (x.size() == 2) { ...x[1]... }`
+  exists(SizeCall sizeCall, Expr k |
+    arrayExpr = sizeCall.getQualifier() and
+    eqOpWithSwapAndNegate(cond, sizeCall, k, true, branch) and
+    minsize = k.getValue().toInt()
+  )
+  or
+  // `if (x.size() != 0) { ...x[0]... }`
+  exists(SizeCall sizeCall, Expr k |
+    arrayExpr = sizeCall.getQualifier() and
+    eqOpWithSwapAndNegate(cond, sizeCall, k, false, branch) and
+    k.getValue().toInt() = 0 and
+    minsize = 1
+  )
+}
+
+// Array accesses like these are safe:
+// `if (!x.empty()) { ... x[0] ... }`
+// `if (x.size() > 2) { ... x[2] ... }`
+predicate indexK_with_check(GuardCondition guard, ArrayIndexCall call) {
+  exists(Expr arrayExpr, BasicBlock block, int i, int minsize, boolean branch |
+    minimum_size_cond(guard, arrayExpr, minsize, branch) and
+    (
+      globalValueNumber(arrayExpr) = globalValueNumber(call.getQualifier()) or
+      hashCons(arrayExpr) = hashCons(call.getQualifier())
+    ) and
+    guard.controls(block, branch) and
+    block.contains(call) and
+    i = call.getArgument(0).getValue().toInt() and
+    0 <= i and
+    i < minsize
+  )
+}
+
+// Array accesses like this are safe:
+// `if (i < x.size()) { ... x[i] ... }`
+predicate indexI_with_check(GuardCondition guard, ArrayIndexCall call) {
+  exists(Expr idx, SizeCall sizeCall, BasicBlock block, boolean branch |
+    relOpWithSwapAndNegate(guard, idx, sizeCall, Lesser(), Strict(), branch) and
+    (
+      globalValueNumber(sizeCall.getQualifier()) = globalValueNumber(call.getQualifier()) and
+      globalValueNumber(idx) = globalValueNumber(call.getArgument(0))
+      or
+      hashCons(sizeCall.getQualifier()) = hashCons(call.getQualifier()) and
+      hashCons(idx) = hashCons(call.getArgument(0))
+    ) and
+    guard.controls(block, branch) and
+    block.contains(call)
+  )
+}
+
+// Array accesses like this are safe:
+// `if (!x.empty()) { ... x[x.size() - 1] ... }`
+predicate index_last_with_check(GuardCondition guard, ArrayIndexCall call) {
+  exists(Expr arrayExpr, SubExpr minusExpr, SizeCall sizeCall, BasicBlock block, boolean branch |
+    minimum_size_cond(guard, arrayExpr, _, branch) and
+    (
+      globalValueNumber(arrayExpr) = globalValueNumber(call.getQualifier()) or
+      hashCons(arrayExpr) = hashCons(call.getQualifier())
+    ) and
+    guard.controls(block, branch) and
+    block.contains(call) and
+    minusExpr = call.getArgument(0) and
+    minusExpr.getRightOperand().getValue().toInt() = 1 and
+    DataFlow::localExprFlow(sizeCall, minusExpr.getLeftOperand()) and
+    (
+      globalValueNumber(sizeCall.getQualifier()) = globalValueNumber(call.getQualifier()) or
+      hashCons(sizeCall.getQualifier()) = hashCons(call.getQualifier())
+    )
+  )
+}
+
+from ArrayIndexCall call
+where
+  not indexK_with_fixedarray(_, call) and
+  not indexK_with_check(_, call) and
+  not indexI_with_check(_, call) and
+  not index_last_with_check(_, call) and
+  // Ignore accesses like this: `vsnprintf(&buffer[0], buffer.size(), format, args)`
+  // That's pointer arithmetic, not a deref, so it's usually a false positive.
+  not exists(AddressOfExpr addrExpr | addrExpr.getOperand() = call) and
+  // Ignore results in the xmpsdk directory.
+  not call.getLocation().getFile().getRelativePath().matches("xmpsdk/%")
+select call, "Unsafe use of operator[]. Use the at() method instead."

.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

.github/workflows/codeql-analysis.yml

@@ -0,0 +1,61 @@
+# For most projects, this workflow file will not need changing; you simply need
+# to commit it to your repository.
+#
+# You may wish to alter this file to override the set of languages analyzed,
+# or to provide custom queries or build logic.
+name: "CodeQL"
+
+on:
+  push:
+    branches: [0.27-maintenance, main]
+  pull_request:
+    # The branches below must be a subset of the branches above
+    branches: [0.27-maintenance, main]
+  workflow_dispatch:
+
+jobs:
+  analyze:
+    name: Analyze
+    runs-on: ubuntu-latest
+
+    strategy:
+      fail-fast: false
+      matrix:
+        language: [ 'cpp' ]
+        # CodeQL supports [ 'cpp', 'csharp', 'go', 'java', 'javascript', 'python' ]
+        # Learn more...
+        # https://docs.github.com/en/github/finding-security-vulnerabilities-and-errors-in-your-code/configuring-code-scanning#overriding-automatic-language-detection
+
+    steps:
+    - name: Checkout repository
+      uses: actions/checkout@v3
+
+    # Initializes the CodeQL tools for scanning.
+    - name: Initialize CodeQL
+      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.
+        # queries: ./path/to/local/query, your-org/your-repo/queries@main
+
+    # Autobuild attempts to build any compiled languages  (C/C++, C#, or Java).
+    # If this step fails, then you should remove it and run the build manually (see below)
+    - name: Autobuild
+      uses: github/codeql-action/autobuild@v1
+
+    # ℹ️ Command-line programs to run using the OS shell.
+    # 📚 https://git.io/JvXDl
+
+    # ✏️ If the Autobuild fails above, remove it and uncomment the following three lines
+    #    and modify them (or add more) to build your code if your project
+    #    uses a compiled language
+
+    #- run: |
+    #   make bootstrap
+    #   make release
+
+    - name: Perform CodeQL Analysis
+      uses: github/codeql-action/analyze@v1