C++: Remove FunctionWithWrappers from cpp/tainted-format-string

cpp/ql/lib/semmle/code/cpp/models/implementations/Printf.qll

@@ -10,12 +10,29 @@ import semmle.code.cpp.models.interfaces.Alias
 import semmle.code.cpp.models.interfaces.SideEffect
 import semmle.code.cpp.models.interfaces.NonThrowing
 
+/**
+ * A formatting function that takes its format arguments through a `va_list` parameter.
+ */
+abstract private class VaListFormattingFunction extends FormattingFunction {
+  final override int getFirstFormatArgumentIndex() { none() }
+
+  final int getVaListParameterIndex() { result = this.getNumberOfParameters() - 1 }
+
+  private predicate hasLocaleParameter() { this.getName().matches("%\\_l") }
+
+  final override int getFormatParameterIndex() {
+    if this.hasLocaleParameter()
+    then result = this.getVaListParameterIndex() - 2
+    else result = this.getVaListParameterIndex() - 1
+  }
+}
+
 /**
  * The standard functions `printf`, `wprintf` and their glib variants.
  */
-private class Printf extends FormattingFunction, AliasFunction, NonCppThrowingFunction {
+private class Printf extends FormattingFunction, AliasFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
   Printf() {
-    this instanceof TopLevelFunction and
     (
       this.hasGlobalOrStdOrBslName(["printf", "wprintf"]) or
       this.hasGlobalName(["printf_s", "wprintf_s", "g_printf"])
@@ -37,9 +54,9 @@ private class Printf extends FormattingFunction, AliasFunction, NonCppThrowingFu
 /**
  * The standard functions `fprintf`, `fwprintf` and their glib variants.
  */
-private class Fprintf extends FormattingFunction, NonCppThrowingFunction {
+private class Fprintf extends FormattingFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
   Fprintf() {
-    this instanceof TopLevelFunction and
     (
       this.hasGlobalOrStdOrBslName(["fprintf", "fwprintf"]) or
       this.hasGlobalName("g_fprintf")
@@ -52,12 +69,44 @@ private class Fprintf extends FormattingFunction, NonCppThrowingFunction {
   override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true }
 }
 
+/**
+ * The standard functions `vprintf`, `vwprintf` and their Microsoft variants.
+ */
+private class Vprintf extends VaListFormattingFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
+  Vprintf() {
+    (
+      this.hasGlobalOrStdOrBslName(["vprintf", "vwprintf"]) or
+      this.hasGlobalName(["_vprintf_l", "_vwprintf_l"])
+    ) and
+    not exists(this.getDefinition().getFile().getRelativePath())
+  }
+
+  override predicate isOutputGlobal() { any() }
+}
+
+/**
+ * The standard functions `vfprintf`, `vfwprintf` and their Microsoft variants.
+ */
+private class Vfprintf extends VaListFormattingFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
+  Vfprintf() {
+    (
+      this.hasGlobalOrStdOrBslName(["vfprintf", "vfwprintf"]) or
+      this.hasGlobalName(["_vfprintf_l", "_vfwprintf_l"])
+    ) and
+    not exists(this.getDefinition().getFile().getRelativePath())
+  }
+
+  override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true }
+}
+
 /**
  * The standard function `sprintf` and its Microsoft and glib variants.
  */
-private class Sprintf extends FormattingFunction, NonCppThrowingFunction {
+private class Sprintf extends FormattingFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
   Sprintf() {
-    this instanceof TopLevelFunction and
     (
       this.hasGlobalOrStdOrBslName([
           "sprintf", // sprintf(dst, format, args...)
@@ -95,14 +144,32 @@ private class Sprintf extends FormattingFunction, NonCppThrowingFunction {
   }
 }
 
+/**
+ * The standard function `vsprintf` and its Microsoft variants.
+ */
+private class Vsprintf extends VaListFormattingFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
+  Vsprintf() {
+    (
+      this.hasGlobalOrStdOrBslName("vsprintf") or // vsprintf(dst, format, va_list)
+      this.hasGlobalName([
+          "_vsprintf_l", // _vsprintf_l(dst, format, locale, va_list)
+          "__vswprintf_l" // __vswprintf_l(dst, format, locale, va_list)
+        ])
+    ) and
+    not exists(this.getDefinition().getFile().getRelativePath())
+  }
+
+  override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = false }
+}
+
 /**
  * Implements `Snprintf`.
  */
 private class SnprintfImpl extends Snprintf, AliasFunction, SideEffectFunction,
-  NonCppThrowingFunction
+  NonCppThrowingFunction instanceof TopLevelFunction
 {
   SnprintfImpl() {
-    this instanceof TopLevelFunction and
     (
       this.hasGlobalOrStdOrBslName([
           "snprintf", // C99 defines snprintf
@@ -169,15 +236,102 @@ private class SnprintfImpl extends Snprintf, AliasFunction, SideEffectFunction,
   }
 }
 
+/**
+ * The standard function `vsnprintf`, and its Microsoft variants.
+ */
+private class VsnprintfImpl extends Snprintf, VaListFormattingFunction, AliasFunction,
+  SideEffectFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
+  VsnprintfImpl() {
+    (
+      this.hasGlobalOrStdOrBslName("vsnprintf") // vsnprintf(dst, count, format, va_list)
+      or
+      this.hasGlobalName([
+          "vsnprintf_s", // vsnprintf_s(dst, size, count, format, va_list)
+          "vsprintf_s", // vsprintf_s(dst, size, format, va_list)
+          "vswprintf_s", // vswprintf_s(dst, size, format, va_list)
+          "_vsnprintf", // _vsnprintf(dst, count, format, va_list)
+          "_vsnprintf_l", // _vsnprintf_l(dst, count, format, locale, va_list)
+          "_vsnprintf_s", // _vsnprintf_s(dst, size, count, format, va_list)
+          "_vsnprintf_s_l", // _vsnprintf_s_l(dst, size, count, format, locale, va_list)
+          "_vsnwprintf", // _vsnwprintf(dst, count, format, va_list)
+          "_vsnwprintf_l", // _vsnwprintf_l(dst, count, format, locale, va_list)
+          "_vsnwprintf_s", // _vsnwprintf_s(dst, size, count, format, va_list)
+          "_vsnwprintf_s_l", // _vsnwprintf_s_l(dst, size, count, format, locale, va_list)
+          "_vsprintf_p", // _vsprintf_p(dst, size, format, va_list)
+          "_vsprintf_p_l", // _vsprintf_p_l(dst, size, format, locale, va_list)
+          "_vsprintf_s_l", // _vsprintf_s_l(dst, size, format, locale, va_list)
+          "_vswprintf_p", // _vswprintf_p(dst, count, format, va_list)
+          "_vswprintf_p_l", // _vswprintf_p_l(dst, count, format, locale, va_list)
+          "_vswprintf_s_l" // _vswprintf_s_l(dst, size, format, locale, va_list)
+        ])
+      or
+      this.hasGlobalOrStdOrBslName("vswprintf") and this.getNumberOfParameters() = 4
+      or
+      this.hasGlobalName("_vswprintf_l") and this.getNumberOfParameters() = 5
+    ) and
+    not exists(this.getDefinition().getFile().getRelativePath())
+  }
+
+  override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = false }
+
+  override int getSizeParameterIndex() { result = 1 }
+
+  override predicate returnsFullFormatLength() { this.hasName(["vsnprintf", "vsnprintf_s"]) }
+
+  override predicate parameterNeverEscapes(int index) {
+    index =
+      [
+        this.getOutputParameterIndex(false), this.getFormatParameterIndex(),
+        this.getVaListParameterIndex()
+      ]
+  }
+
+  override predicate parameterEscapesOnlyViaReturn(int index) { none() }
+
+  override predicate parameterIsAlwaysReturned(int index) { none() }
+
+  override predicate hasOnlySpecificReadSideEffects() { any() }
+
+  override predicate hasOnlySpecificWriteSideEffects() { any() }
+
+  override predicate hasSpecificWriteSideEffect(ParameterIndex i, boolean buffer, boolean mustWrite) {
+    i = this.getOutputParameterIndex(false) and buffer = true and mustWrite = false
+    or
+    i = this.getVaListParameterIndex() and buffer = false and mustWrite = false
+  }
+
+  override predicate hasSpecificReadSideEffect(ParameterIndex i, boolean buffer) {
+    i = this.getFormatParameterIndex() and buffer = true
+    or
+    i = this.getVaListParameterIndex() and buffer = false
+  }
+}
+
+/**
+ * The Microsoft `_vscprintf_p` functions and variants.
+ */
+private class Vscprintf extends VaListFormattingFunction, NonCppThrowingFunction instanceof TopLevelFunction
+{
+  Vscprintf() {
+    this.hasGlobalName([
+        "_vscprintf_p", // _vscprintf_p(format, va_list)
+        "_vscprintf_p_l", // _vscprintf_p_l(format, locale, va_list)
+        "_vscwprintf_p", // _vscwprintf_p(format, va_list)
+        "_vscwprintf_p_l" // _vscwprintf_p_l(format, locale, va_list)
+      ]) and
+    not exists(this.getDefinition().getFile().getRelativePath())
+  }
+}
+
 /**
  * The Microsoft `StringCchPrintf` function and variants.
  * See: https://learn.microsoft.com/en-us/windows/win32/api/strsafe/
  *      and
  *      https://learn.microsoft.com/en-us/previous-versions/windows/embedded/ms860435(v=msdn.10)
  */
-private class StringCchPrintf extends FormattingFunction {
+private class StringCchPrintf extends FormattingFunction instanceof TopLevelFunction {
   StringCchPrintf() {
-    this instanceof TopLevelFunction and
     exists(string baseName |
       baseName in [
           "StringCchPrintf", //StringCchPrintf(pszDest, cchDest, pszFormat, ...)
@@ -207,9 +361,8 @@ private class StringCchPrintf extends FormattingFunction {
 /**
  * The standard function `syslog`.
  */
-private class Syslog extends FormattingFunction, NonCppThrowingFunction {
+private class Syslog extends FormattingFunction, NonCppThrowingFunction instanceof TopLevelFunction {
   Syslog() {
-    this instanceof TopLevelFunction and
     this.hasGlobalName("syslog") and
     not exists(this.getDefinition().getFile().getRelativePath())
   }

cpp/ql/lib/semmle/code/cpp/models/interfaces/FormattingFunction.qll

@@ -42,21 +42,6 @@ private Type getAFormatterWideTypeOrDefault() {
  * A standard library function that uses a `printf`-like formatting string.
  */
 abstract class FormattingFunction extends ArrayFunction, TaintFunction {
-  int firstFormatArgumentIndex;
-
-  FormattingFunction() {
-    firstFormatArgumentIndex > 0 and
-    if this.hasDefinition()
-    then firstFormatArgumentIndex = this.getDefinition().getNumberOfParameters()
-    else
-      if this instanceof BuiltInFunction
-      then firstFormatArgumentIndex = this.getNumberOfParameters()
-      else
-        forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() |
-          firstFormatArgumentIndex = fde.getNumberOfParameters()
-        )
-  }
-
   /** Gets the position at which the format parameter occurs. */
   abstract int getFormatParameterIndex();
 
@@ -135,8 +120,21 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
    * Gets the position of the first format argument, corresponding with
    * the first format specifier in the format string. We ignore all
    * implicit function definitions.
+   *
+   * There is no result if the formatting function takes a `va_list` argument.
    */
-  int getFirstFormatArgumentIndex() { result = firstFormatArgumentIndex }
+  int getFirstFormatArgumentIndex() {
+    result > 0 and
+    if this.hasDefinition()
+    then result = this.getDefinition().getNumberOfParameters()
+    else
+      if this instanceof BuiltInFunction
+      then result = this.getNumberOfParameters()
+      else
+        forex(FunctionDeclarationEntry fde | fde = this.getAnExplicitDeclarationEntry() |
+          result = fde.getNumberOfParameters()
+        )
+  }
 
   /**
    * Gets the position of the buffer size argument, if any.

cpp/ql/src/Security/CWE/CWE-134/UncontrolledFormatString.ql

@@ -15,40 +15,26 @@
 
 import cpp
 import semmle.code.cpp.security.Security
-import semmle.code.cpp.security.FunctionWithWrappers
+import semmle.code.cpp.security.PrintfLike
 import semmle.code.cpp.security.FlowSources
 import semmle.code.cpp.ir.dataflow.TaintTracking
 import semmle.code.cpp.ir.IR
 import Flow::PathGraph
 
 predicate isSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
 
-/**
- * Holds if `f` is a printf-like function or a (possibly nested) wrapper
- * that forwards a format-string parameter to one.
- *
- * Functions that *implement* printf-like behavior (e.g. a custom
- * `vsnprintf` variant) internally parse the caller-supplied format string
- * and build small, bounded, local format strings such as `"%d"` or `"%ld"`
- * for inner `sprintf` calls.  Taint that reaches those inner calls via the
- * parsed format specifier is not exploitable, so sinks inside such
- * functions should be excluded.
- */
-private predicate isPrintfImplementation(Function f) {
-  f instanceof PrintfLikeFunction
-  or
-  exists(PrintfLikeFunction printf | printf.wrapperFunction(f, _, _))
+predicate isSink(DataFlow::Node node, FormattingFunction f) {
+  exists(Call c, int i |
+    c.getTarget() = f and
+    i = f.getFormatParameterIndex() and
+    c.getArgument(i) = node.asIndirectExpr()
+  )
 }
 
 module Config implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node node) { isSource(node, _) }
 
-  predicate isSink(DataFlow::Node node) {
-    exists(PrintfLikeFunction printf |
-      printf.outermostWrapperFunctionCall([node.asExpr(), node.asIndirectExpr()], _)
-    ) and
-    not isPrintfImplementation([node.asExpr(), node.asIndirectExpr()].getEnclosingFunction())
-  }
+  predicate isSink(DataFlow::Node node) { isSink(node, _) }
 
   private predicate isArithmeticNonCharType(ArithmeticType type) {
     not type instanceof CharType and
@@ -69,14 +55,14 @@ module Config implements DataFlow::ConfigSig {
 module Flow = TaintTracking::Global<Config>;
 
 from
-  PrintfLikeFunction printf, string printfFunction, string sourceType, DataFlow::Node source,
-  DataFlow::Node sink, Flow::PathNode sourceNode, Flow::PathNode sinkNode
+  Function printf, string sourceType, DataFlow::Node source, DataFlow::Node sink,
+  Flow::PathNode sourceNode, Flow::PathNode sinkNode
 where
   source = sourceNode.getNode() and
   sink = sinkNode.getNode() and
   isSource(source, sourceType) and
-  printf.outermostWrapperFunctionCall([sink.asExpr(), sink.asIndirectExpr()], printfFunction) and
+  isSink(sink, printf) and
   Flow::flowPath(sourceNode, sinkNode)
 select sink, sourceNode, sinkNode,
   "The value of this argument may come from $@ and is being used as a formatting argument to " +
-    printfFunction + ".", source, sourceType
+    printf + ".", source, sourceType

cpp/ql/test/query-tests/Security/CWE/CWE-134/SAMATE/UncontrolledFormatString.expected

@@ -1,7 +1,11 @@
 edges
+| char_connect_socket_w32_vsnprintf_01_bad.c:40:30:40:33 | *data | char_connect_socket_w32_vsnprintf_01_bad.c:47:32:47:35 | *data | provenance |  |
 | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | char_connect_socket_w32_vsnprintf_01_bad.c:100:13:100:60 | ... = ... | provenance |  |
 | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | provenance |  |
+| char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | provenance |  |
+| char_connect_socket_w32_vsnprintf_01_bad.c:100:13:100:60 | ... = ... | char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | provenance |  |
 | char_connect_socket_w32_vsnprintf_01_bad.c:100:13:100:60 | ... = ... | char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | provenance |  |
+| char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | char_connect_socket_w32_vsnprintf_01_bad.c:40:30:40:33 | *data | provenance |  |
 | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | char_console_fprintf_01_bad.c:37:21:37:43 | ... = ... | provenance |  |
 | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | char_console_fprintf_01_bad.c:44:17:44:37 | ... = ... | provenance |  |
 | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | char_console_fprintf_01_bad.c:49:21:49:24 | *data | provenance |  |
@@ -10,9 +14,12 @@ edges
 | char_environment_fprintf_01_bad.c:27:30:27:35 | *call to getenv | char_environment_fprintf_01_bad.c:27:30:27:35 | *call to getenv | provenance |  |
 | char_environment_fprintf_01_bad.c:27:30:27:35 | *call to getenv | char_environment_fprintf_01_bad.c:36:21:36:24 | *data | provenance | TaintFunction |
 nodes
+| char_connect_socket_w32_vsnprintf_01_bad.c:40:30:40:33 | *data | semmle.label | *data |
+| char_connect_socket_w32_vsnprintf_01_bad.c:47:32:47:35 | *data | semmle.label | *data |
 | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | semmle.label | recv output argument |
 | char_connect_socket_w32_vsnprintf_01_bad.c:100:13:100:60 | ... = ... | semmle.label | ... = ... |
 | char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | semmle.label | *data |
+| char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | semmle.label | *data |
 | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | semmle.label | fgets output argument |
 | char_console_fprintf_01_bad.c:37:21:37:43 | ... = ... | semmle.label | ... = ... |
 | char_console_fprintf_01_bad.c:44:17:44:37 | ... = ... | semmle.label | ... = ... |
@@ -22,6 +29,7 @@ nodes
 | char_environment_fprintf_01_bad.c:36:21:36:24 | *data | semmle.label | *data |
 subpaths
 #select
-| char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | The value of this argument may come from $@ and is being used as a formatting argument to badVaSink(data), which calls vsnprintf(format). | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | buffer read by recv |
-| char_console_fprintf_01_bad.c:49:21:49:24 | *data | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | char_console_fprintf_01_bad.c:49:21:49:24 | *data | The value of this argument may come from $@ and is being used as a formatting argument to fprintf(format). | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | string read by fgets |
-| char_environment_fprintf_01_bad.c:36:21:36:24 | *data | char_environment_fprintf_01_bad.c:27:30:27:35 | *call to getenv | char_environment_fprintf_01_bad.c:36:21:36:24 | *data | The value of this argument may come from $@ and is being used as a formatting argument to fprintf(format). | char_environment_fprintf_01_bad.c:27:30:27:35 | *call to getenv | an environment variable |
+| char_connect_socket_w32_vsnprintf_01_bad.c:47:32:47:35 | *data | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | char_connect_socket_w32_vsnprintf_01_bad.c:47:32:47:35 | *data | The value of this argument may come from $@ and is being used as a formatting argument to vsnprintf. | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | buffer read by recv |
+| char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | char_connect_socket_w32_vsnprintf_01_bad.c:125:15:125:18 | *data | The value of this argument may come from $@ and is being used as a formatting argument to badVaSink. | char_connect_socket_w32_vsnprintf_01_bad.c:94:46:94:69 | recv output argument | buffer read by recv |
+| char_console_fprintf_01_bad.c:49:21:49:24 | *data | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | char_console_fprintf_01_bad.c:49:21:49:24 | *data | The value of this argument may come from $@ and is being used as a formatting argument to fprintf. | char_console_fprintf_01_bad.c:30:23:30:35 | fgets output argument | string read by fgets |
+| char_environment_fprintf_01_bad.c:36:21:36:24 | *data | char_environment_fprintf_01_bad.c:27:30:27:35 | *call to getenv | char_environment_fprintf_01_bad.c:36:21:36:24 | *data | The value of this argument may come from $@ and is being used as a formatting argument to fprintf. | char_environment_fprintf_01_bad.c:27:30:27:35 | *call to getenv | an environment variable |

cpp/ql/test/query-tests/Security/CWE/CWE-134/semmle/argv/argvLocal.c

@@ -7,7 +7,7 @@ int printf(const char *format, ...);
 void *memcpy(void *s1, const void *s2, size_t n);
 void *malloc(size_t size);
 void printWrapper(char *correct) {
-	printf(correct);
+	printf(correct); // BAD
 }
 
 int main(int argc, char **argv) {