Semmle 1.20
Skip to end of metadata
Go to start of metadata

Name: Unchecked return value used as offset

Description: Using a return value as a pointer offset without checking that the value is positive may lead to buffer overruns.

ID: cpp/missing-negativity-test

Kind: problem

Severity: warning

Query: MissingNegativityTest.ql
/**
 * @name Unchecked return value used as offset
 * @description  Using a return value as a pointer offset without checking that the value is positive
 *               may lead to buffer overruns.
 * @kind problem
 * @id cpp/missing-negativity-test
 * @problem.severity warning
 * @tags reliability
 *       security
 *       external/cwe/cwe-823
 */
import cpp
import Negativity

class IntegralReturnValue extends FunctionCall
{
  IntegralReturnValue() {
    this.getType().getUnderlyingType() instanceof IntegralType
  }

  predicate isChecked() {
    exists(ControlFlowNode def, ControlFlowNode test, Variable v |
      exprDefinition(v, def, this) and
      definitionReaches(def, test) and
      errorSuccessor(v, test.getASuccessor()))
  }
}

class FunctionWithNegativeReturn extends Function
{
  FunctionWithNegativeReturn() {
    this.getType().getUnderlyingType() instanceof IntegralType and
    ( exists(ReturnStmt ret |
        ret.getExpr().getValue().toInt() < 0 and
        ret.getEnclosingFunction() = this)
      or
      count(IntegralReturnValue val | val.getTarget() = this and val.isChecked()) * 100 /
      count(IntegralReturnValue val | val.getTarget() = this) >= 80)
  }
}

predicate dangerousUse(IntegralReturnValue val, Expr use)
{
  exists(ArrayExpr ae | ae.getArrayOffset() = val and use = val)
  or
  exists(LocalScopeVariable v, ControlFlowNode def, ArrayExpr ae |
    exprDefinition(v, def, val) and
    use = ae.getArrayOffset() and
    not boundsChecked(v, use) and
    definitionUsePair(v, def, use))
  or
  (use.getParent().(AddExpr).getAnOperand() = val and
    val = use and
    use.getType().getUnderlyingType() instanceof PointerType)
  or
  exists(LocalScopeVariable v, ControlFlowNode def, AddExpr add |
    exprDefinition(v, def, val) and
    definitionUsePair(v, def, use) and
    add.getAnOperand() = use and
    not boundsChecked(v, use) and
    add.getType().getUnderlyingType() instanceof PointerType)
}

from FunctionWithNegativeReturn f, IntegralReturnValue val, Expr dangerous
where val.getTarget() = f
  and dangerousUse(val, dangerous)
select dangerous, "Dangerous use of possibly negative value (return value of '" + f.getName() + "')."

This query finds pointer arithmetic expressions that use a value returned from a function without checking that the value is positive. Most pointer arithmetic and almost all array element accesses use a positive value for offsets. A negative value is likely to be a defect in the returning function. Checking pointer offsets (particularly if they derive from user input) is necessary to avoid buffer overruns.

The query looks only at the return values of functions that may return a negative value (not all functions).

This check is an approximation, so some results may not be actual defects in the program. It is not possible in general to compute the actual branch taken in conditional statements such as "if" without running the program with all possible input data. This means that it is not possible to determine if a particular statement is going to be executed.

Recommendation

Review the function. Determine whether it needs to check that the value is positive before performing pointer arithmetic.

Example

The example below shows an example of this problem. There is no check to ensure that the value of recordIdx is positive and safe to use as an array offset.

Record records[SIZE] = ...;

int f() {
    int recordIdx = 0;
    recordIdx = readUserInput(); //recordIdx is returned from a function
        // there is no check so it could be negative
    doFoo(&(records[recordIdx])); //but is not checked before use as an array offset
}

References