Merge pull request #1771 from kevinbackhouse/codeql-unsafe-vector-access

CodeQL query to detect unsafe uses of std::vector::operator[]

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

@@ -2,6 +2,7 @@
  * @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

.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,179 @@
+/**
+ * @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)
+select call, "Unsafe use of operator[]. Use the at() method instead."