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

Name: Unterminated variadic call

Description: Calling a variadic function without a sentinel value may result in a buffer overflow if the function expects a specific value to terminate the argument list.

ID: cpp/unterminated-variadic-call

Kind: problem

Severity: warning

Precision: medium

Query: UnterminatedVarargsCall.ql
/**
 * @name Unterminated variadic call
 * @description Calling a variadic function without a sentinel value
 *              may result in a buffer overflow if the function expects
 *              a specific value to terminate the argument list.
 * @kind problem
 * @problem.severity warning
 * @precision medium
 * @id cpp/unterminated-variadic-call
 * @tags reliability
 *       security
 *       external/cwe/cwe-121
 */

import cpp

/**
 * Gets a normalized textual representation of `e`'s value.
 * The result is the same as `Expr.getValue()`, except if there is a
 * trailing `".0"` then it is removed. This means that, for example,
 * the values of `-1` and `-1.0` would be considered the same.
 */
string normalisedExprValue(Expr e) {
  result = e.getValue().regexpReplaceAll("\\.0$", "")
}

/**
 * A variadic function which is not a formatting function.
 */
class VarargsFunction extends Function {
  VarargsFunction() {
    this.isVarargs() and
    not this instanceof FormattingFunction
  }

  Expr trailingArgumentIn(FunctionCall fc) {
    fc = this.getACallToThisFunction() and
    result = fc.getArgument(fc.getNumberOfArguments() - 1)
  }

  string trailingArgValue(FunctionCall fc) {
    result = normalisedExprValue(this.trailingArgumentIn(fc))
  }

  private
  int trailingArgValueCount(string value) {
    result = strictcount(FunctionCall fc | trailingArgValue(fc) = value)
  }

  string nonTrailingVarArgValue(FunctionCall fc, int index) {
    fc = this.getACallToThisFunction() and
    index >= this.getNumberOfParameters() and
    index < fc.getNumberOfArguments() - 1 and
    result = normalisedExprValue(fc.getArgument(index))
  }

  private
  int totalCount() {
    result = strictcount(FunctionCall fc | fc = this.getACallToThisFunction())
  }

  string normalTerminator(int cnt) {
    (
      result = "0" or result = "-1"
    ) and (
      cnt = trailingArgValueCount(result)
    ) and (
      2 * cnt > totalCount()
    ) and not exists(FunctionCall fc, int index |
      // terminator value is used in a non-terminating position
      nonTrailingVarArgValue(fc, index) = result
    )
  }

  predicate isWhitelisted() {
    this.hasQualifiedName("open") or
    this.hasQualifiedName("fcntl") or
    this.hasQualifiedName("ptrace")
  }
}

from VarargsFunction f, FunctionCall fc, string terminator, int cnt
where terminator = f.normalTerminator(cnt)
  and fc = f.getACallToThisFunction()
  and not normalisedExprValue(f.trailingArgumentIn(fc)) = terminator
  and not f.isWhitelisted()
select fc, "Calls to $@ should use the value " + terminator
         + " as a terminator (" + cnt + " calls do).", f, f.getQualifiedName()

The program calls a function that expects the variable argument list to be terminated with a sentinel value (typically NULL, 0 or -1). In this case, the sentinel value has been omitted as a final argument. This defect may result in incorrect behavior of the function and unintended stack memory access, leading to incorrect program results, instability, and even vulnerability to buffer overflow style attacks.

Recommendation

Each description of a defect highlighted by this rule includes a suggested value for the terminator. Check that this value is correct, then add it to the end of the call.

Example

#include <stdarg.h>

void pushStrings(char *firstString, ...)
{
	va_list args;
	char *arg;

	va_start(args, firstString);

	// process inputs, beginning with firstString, ending when NULL is reached
	arg = firstString;
	while (arg != NULL)
	{
		// push the string
		pushString(arg);
	
		// move on to the next input
		arg = va_arg(args, char *);
	}

	va_end(args);
}

void badFunction()
{
	pushStrings("hello", "world", NULL); // OK
	
	pushStrings("apple", "pear", "banana", NULL); // OK

	pushStrings("car", "bus", "train"); // BAD, not terminated with the expected NULL
}

In this example, the third call to pushStrings is not correctly terminated. This call should be updated to include NULL as the fourth and final argument to this call.

References
  • Common Weakness Enumeration: CWE-121.