Safer std::vector indexing

[kevinbackhouse]

Fixes: GHSA-v5g7-46xf-h728

There is an out-of-bounds read at value.cpp, line 699. If the string is something like "type= x", then type will be empty due to the space character immediately after "type=".

This is the same kind of bug as I fixed in #1735. The solution is to use std::vector::at() rather than operator[], except when the code does clear bounds checking of the index.

There are three commits:

1. Regression test for GHSA-v5g7-46xf-h728
2. Fix for GHSA-v5g7-46xf-h728
3. Defensive fixes for similar unsafe uses of operator[]. (Found with a CodeQL query.)

[kevinbackhouse]

This is the CodeQL query that I used to find the other unsafe uses of operator[]. It's a more sophisticated version of the one that I used previously in #1735. (That one only looked for accesses of an array named value_. Without that clause, the results of that query are too noisy.)

/**
 * @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
 * @id cpp/unsafe-vector-access
 * @tags security
 */

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")
  }
}

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, ClassTemplateInstantiation t, TemplateClass c
where
  c = call.getTemplateClass() and
  t = call.getClassTemplateInstantiation() and
  not exists(Expr arg |
    t.getSimpleName() = "array" and
    arg = call.getArgument(0) and
    lowerBound(arg) >= 0 and
    upperBound(arg) < t.getTemplateArgument(1).(Literal).getValue().toInt()
  ) 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 standard libraries and in the xmpsdk subdirectory.
  exists(string path |
    path = call.getLocation().getFile().getRelativePath() and
    not path.matches("xmpsdk/%")
  )
select call, "Unsafe use of operator[]. Use the at() method instead."