Merge pull request #4754 from geoffw0/modelchanges3

C++: Expose more information in FormattingFunction and make subclasses private.

Author: MathiasVP

cpp/change-notes/2020-11-05-formatting-function.md

@@ -0,0 +1,4 @@
+lgtm,codescanning
+* `FormattingFunction.getOutputParameterIndex` now has a parameter identifying whether the output at that index is a buffer or a stream.
+* `FormattingFunction` now has a predicate `isOutputGlobal` indicating when the output is to a global stream.
+* The `primitiveVariadicFormatter` and `variadicFormatter` predicates have more parameters exposing information about the function.

cpp/ql/src/semmle/code/cpp/commons/Printf.qll

@@ -34,66 +34,87 @@ class AttributeFormattingFunction extends FormattingFunction {
 
 /**
  * A standard function such as `vprintf` that has a format parameter
- * and a variable argument list of type `va_arg`.
+ * and a variable argument list of type `va_arg`. `formatParamIndex` indicates
+ * the format parameter and `type` indicates the type of `vprintf`:
+ *  - `""` is a `vprintf` variant, `outputParamIndex` is `-1`.
+ *  - `"f"` is a `vfprintf` variant, `outputParamIndex` indicates the output stream parameter.
+ *  - `"s"` is a `vsprintf` variant, `outputParamIndex` indicates the output buffer parameter.
+ *  - `"?"` if the type cannot be deteremined.  `outputParamIndex` is `-1`.
  */
-predicate primitiveVariadicFormatter(TopLevelFunction f, int formatParamIndex) {
-  f.getName().regexpMatch("_?_?va?[fs]?n?w?printf(_s)?(_p)?(_l)?") and
+predicate primitiveVariadicFormatter(
+  TopLevelFunction f, string type, int formatParamIndex, int outputParamIndex
+) {
+  type = f.getName().regexpCapture("_?_?va?([fs]?)n?w?printf(_s)?(_p)?(_l)?", 1) and
   (
     if f.getName().matches("%\\_l")
     then formatParamIndex = f.getNumberOfParameters() - 3
     else formatParamIndex = f.getNumberOfParameters() - 2
-  )
+  ) and
+  if type = "" then outputParamIndex = -1 else outputParamIndex = 0 // Conveniently, these buffer parameters are all at index 0.
 }
 
-/**
- * A standard function such as `vsprintf` that has an output parameter
- * and a variable argument list of type `va_arg`.
- */
-private predicate primitiveVariadicFormatterOutput(TopLevelFunction f, int outputParamIndex) {
-  // note: this might look like the regular expression in `primitiveVariadicFormatter`, but
-  // there is one important difference: the [fs] part is not optional, as these classify
-  // the `printf` variants that write to a buffer.
-  // Conveniently, these buffer parameters are all at index 0.
-  f.getName().regexpMatch("_?_?va?[fs]n?w?printf(_s)?(_p)?(_l)?") and outputParamIndex = 0
-}
-
-private predicate callsVariadicFormatter(Function f, int formatParamIndex) {
-  exists(FunctionCall fc, int i |
-    variadicFormatter(fc.getTarget(), i) and
+private predicate callsVariadicFormatter(
+  Function f, string type, int formatParamIndex, int outputParamIndex
+) {
+  // calls a variadic formatter with `formatParamIndex`, `outputParamIndex` linked
+  exists(FunctionCall fc, int format, int output |
+    variadicFormatter(fc.getTarget(), type, format, output) and
     fc.getEnclosingFunction() = f and
-    fc.getArgument(i) = f.getParameter(formatParamIndex).getAnAccess()
+    fc.getArgument(format) = f.getParameter(formatParamIndex).getAnAccess() and
+    fc.getArgument(output) = f.getParameter(outputParamIndex).getAnAccess()
   )
-}
-
-private predicate callsVariadicFormatterOutput(Function f, int outputParamIndex) {
-  exists(FunctionCall fc, int i |
+  or
+  // calls a variadic formatter with only `formatParamIndex` linked
+  exists(FunctionCall fc, string calledType, int format, int output |
+    variadicFormatter(fc.getTarget(), calledType, format, output) and
     fc.getEnclosingFunction() = f and
-    variadicFormatterOutput(fc.getTarget(), i) and
-    fc.getArgument(i) = f.getParameter(outputParamIndex).getAnAccess()
+    fc.getArgument(format) = f.getParameter(formatParamIndex).getAnAccess() and
+    not fc.getArgument(output) = f.getParameter(_).getAnAccess() and
+    (
+      calledType = "" and
+      type = ""
+      or
+      calledType != "" and
+      type = "?" // we probably should have an `outputParamIndex` link but have lost it.
+    ) and
+    outputParamIndex = -1
   )
 }
 
 /**
- * Holds if `f` is a function such as `vprintf` that takes variable argument list
- * of type `va_arg` and writes formatted output to a buffer given as a parameter at
- * index `outputParamIndex`, if any.
+ * Holds if `f` is a function such as `vprintf` that has a format parameter
+ * and a variable argument list of type `va_arg`. `formatParamIndex` indicates
+ * the format parameter and `type` indicates the type of `vprintf`:
+ *  - `""` is a `vprintf` variant, `outputParamIndex` is `-1`.
+ *  - `"f"` is a `vfprintf` variant, `outputParamIndex` indicates the output stream parameter.
+ *  - `"s"` is a `vsprintf` variant, `outputParamIndex` indicates the output buffer parameter.
+ *  - `"?"` if the type cannot be deteremined.  `outputParamIndex` is `-1`.
  */
-private predicate variadicFormatterOutput(Function f, int outputParamIndex) {
-  primitiveVariadicFormatterOutput(f, outputParamIndex)
+predicate variadicFormatter(Function f, string type, int formatParamIndex, int outputParamIndex) {
+  primitiveVariadicFormatter(f, type, formatParamIndex, outputParamIndex)
   or
   not f.isVarargs() and
-  callsVariadicFormatterOutput(f, outputParamIndex)
+  callsVariadicFormatter(f, type, formatParamIndex, outputParamIndex)
+}
+
+/**
+ * A standard function such as `vprintf` that has a format parameter
+ * and a variable argument list of type `va_arg`.
+ *
+ * DEPRECATED: Use the four argument version instead.
+ */
+deprecated predicate primitiveVariadicFormatter(TopLevelFunction f, int formatParamIndex) {
+  primitiveVariadicFormatter(f, _, formatParamIndex, _)
 }
 
 /**
  * Holds if `f` is a function such as `vprintf` that has a format parameter
  * (at `formatParamIndex`) and a variable argument list of type `va_arg`.
+ *
+ * DEPRECATED: Use the four argument version instead.
  */
-predicate variadicFormatter(Function f, int formatParamIndex) {
-  primitiveVariadicFormatter(f, formatParamIndex)
-  or
-  not f.isVarargs() and
-  callsVariadicFormatter(f, formatParamIndex)
+deprecated predicate variadicFormatter(Function f, int formatParamIndex) {
+  variadicFormatter(f, _, formatParamIndex, _)
 }
 
 /**
@@ -103,11 +124,17 @@ predicate variadicFormatter(Function f, int formatParamIndex) {
 class UserDefinedFormattingFunction extends FormattingFunction {
   override string getAPrimaryQlClass() { result = "UserDefinedFormattingFunction" }
 
-  UserDefinedFormattingFunction() { isVarargs() and callsVariadicFormatter(this, _) }
+  UserDefinedFormattingFunction() { isVarargs() and callsVariadicFormatter(this, _, _, _) }
+
+  override int getFormatParameterIndex() { callsVariadicFormatter(this, _, result, _) }
 
-  override int getFormatParameterIndex() { callsVariadicFormatter(this, result) }
+  override int getOutputParameterIndex(boolean isStream) {
+    callsVariadicFormatter(this, "f", _, result) and isStream = true
+    or
+    callsVariadicFormatter(this, "s", _, result) and isStream = false
+  }
 
-  override int getOutputParameterIndex() { callsVariadicFormatterOutput(this, result) }
+  override predicate isOutputGlobal() { callsVariadicFormatter(this, "", _, _) }
 }
 
 /**

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

@@ -11,7 +11,7 @@ import semmle.code.cpp.models.interfaces.Alias
 /**
  * The standard functions `printf`, `wprintf` and their glib variants.
  */
-class Printf extends FormattingFunction, AliasFunction {
+private class Printf extends FormattingFunction, AliasFunction {
   Printf() {
     this instanceof TopLevelFunction and
     (
@@ -31,6 +31,8 @@ class Printf extends FormattingFunction, AliasFunction {
     hasGlobalName("wprintf_s")
   }
 
+  override predicate isOutputGlobal() { any() }
+
   override predicate parameterNeverEscapes(int n) { n = 0 }
 
   override predicate parameterEscapesOnlyViaReturn(int n) { none() }
@@ -41,7 +43,7 @@ class Printf extends FormattingFunction, AliasFunction {
 /**
  * The standard functions `fprintf`, `fwprintf` and their glib variants.
  */
-class Fprintf extends FormattingFunction {
+private class Fprintf extends FormattingFunction {
   Fprintf() {
     this instanceof TopLevelFunction and
     (
@@ -56,7 +58,7 @@ class Fprintf extends FormattingFunction {
 
   deprecated override predicate isWideCharDefault() { hasGlobalOrStdName("fwprintf") }
 
-  override int getOutputParameterIndex() { result = 0 }
+  override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = true }
 }
 
 /**
@@ -109,7 +111,9 @@ private class Sprintf extends FormattingFunction {
     result = 1
   }
 
-  override int getOutputParameterIndex() { not hasGlobalName("g_strdup_printf") and result = 0 }
+  override int getOutputParameterIndex(boolean isStream) {
+    not hasGlobalName("g_strdup_printf") and result = 0 and isStream = false
+  }
 
   override int getFirstFormatArgumentIndex() {
     if hasGlobalName("__builtin___sprintf_chk")
@@ -164,7 +168,7 @@ private class SnprintfImpl extends Snprintf {
         .getSize() > 1
   }
 
-  override int getOutputParameterIndex() { result = 0 }
+  override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = false }
 
   override int getFirstFormatArgumentIndex() {
     exists(string name |
@@ -224,20 +228,22 @@ private class StringCchPrintf extends FormattingFunction {
         .getSize() > 1
   }
 
-  override int getOutputParameterIndex() { result = 0 }
+  override int getOutputParameterIndex(boolean isStream) { result = 0 and isStream = false }
 
   override int getSizeParameterIndex() { result = 1 }
 }
 
 /**
  * The standard function `syslog`.
  */
-class Syslog extends FormattingFunction {
+private class Syslog extends FormattingFunction {
   Syslog() {
     this instanceof TopLevelFunction and
     hasGlobalName("syslog") and
     not exists(getDefinition().getFile().getRelativePath())
   }
 
   override int getFormatParameterIndex() { result = 1 }
+
+  override predicate isOutputGlobal() { any() }
 }

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

@@ -108,10 +108,26 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
     result = getAFormatterWideTypeOrDefault() // may have more than one result
   }
 
+  /**
+   * Gets the position at which the output parameter, if any, occurs. If
+   * `isStream` is `true`, the output parameter is a stream (that is, this
+   * function behaves like `fprintf`). If `isStream` is `false`, the output
+   * parameter is a buffer (that is, this function behaves like `sprintf`).
+   */
+  int getOutputParameterIndex(boolean isStream) { none() }
+
   /**
    * Gets the position at which the output parameter, if any, occurs.
+   *
+   * DEPRECATED: use `getOutputParameterIndex(boolean isStream)` instead.
+   */
+  deprecated int getOutputParameterIndex() { result = getOutputParameterIndex(_) }
+
+  /**
+   * Holds if this function outputs to a global stream such as standard output,
+   * standard error or a system log. For example `printf`.
    */
-  int getOutputParameterIndex() { none() }
+  predicate isOutputGlobal() { none() }
 
   /**
    * Gets the position of the first format argument, corresponding with
@@ -141,18 +157,18 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
   }
 
   override predicate hasArrayWithVariableSize(int bufParam, int countParam) {
-    bufParam = getOutputParameterIndex() and
+    bufParam = getOutputParameterIndex(false) and
     countParam = getSizeParameterIndex()
   }
 
   override predicate hasArrayWithUnknownSize(int bufParam) {
-    bufParam = getOutputParameterIndex() and
+    bufParam = getOutputParameterIndex(false) and
     not exists(getSizeParameterIndex())
   }
 
   override predicate hasArrayInput(int bufParam) { bufParam = getFormatParameterIndex() }
 
-  override predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex() }
+  override predicate hasArrayOutput(int bufParam) { bufParam = getOutputParameterIndex(false) }
 
   override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
     exists(int arg |
@@ -161,7 +177,7 @@ abstract class FormattingFunction extends ArrayFunction, TaintFunction {
         arg >= getFirstFormatArgumentIndex()
       ) and
       input.isParameterDeref(arg) and
-      output.isParameterDeref(getOutputParameterIndex())
+      output.isParameterDeref(getOutputParameterIndex(_))
     )
   }
 }

cpp/ql/src/semmle/code/cpp/security/BufferWrite.qll

@@ -229,7 +229,7 @@ class SprintfBW extends BufferWriteCall {
     result = this.(FormattingFunctionCall).getFormatArgument(_)
   }
 
-  override Expr getDest() { result = getArgument(f.getOutputParameterIndex()) }
+  override Expr getDest() { result = getArgument(f.getOutputParameterIndex(false)) }
 
   override int getMaxData() {
     exists(FormatLiteral fl |

cpp/ql/src/semmle/code/cpp/security/FileWrite.qll

@@ -3,7 +3,6 @@
  */
 
 import cpp
-import semmle.code.cpp.models.implementations.Printf
 
 /**
  * A function call that writes to a file.
@@ -144,8 +143,8 @@ private predicate fileWrite(Call write, Expr source, Expr dest) {
     )
     or
     // fprintf
-    s >= f.(Fprintf).getFormatParameterIndex() and
-    d = f.(Fprintf).getOutputParameterIndex()
+    s >= f.(FormattingFunction).getFormatParameterIndex() and
+    d = f.(FormattingFunction).getOutputParameterIndex(true)
   )
   or
   // file stream using '<<', 'put' or 'write'

cpp/ql/src/semmle/code/cpp/security/OutputWrite.qll

@@ -59,11 +59,9 @@ private predicate outputWrite(Expr write, Expr source) {
   exists(Function f, int arg |
     f = write.(Call).getTarget() and source = write.(Call).getArgument(arg)
   |
-    // printf
-    arg >= f.(Printf).getFormatParameterIndex()
-    or
-    // syslog
-    arg >= f.(Syslog).getFormatParameterIndex()
+    // printf / syslog
+    f.(FormattingFunction).isOutputGlobal() and
+    arg >= f.(FormattingFunction).getFormatParameterIndex()
     or
     // puts, putchar
     (

cpp/ql/src/semmle/code/cpp/security/PrintfLike.qll

@@ -17,7 +17,7 @@ predicate printfLikeFunction(Function func, int formatArg) {
   formatArg = func.(FormattingFunction).getFormatParameterIndex() and
   not func instanceof UserDefinedFormattingFunction
   or
-  primitiveVariadicFormatter(func, formatArg)
+  primitiveVariadicFormatter(func, _, formatArg, _)
   or
   exists(ExternalData data |
     // TODO Do this \ to / conversion in the toolchain?

cpp/ql/src/semmle/code/cpp/security/TaintTrackingImpl.qll

@@ -478,7 +478,7 @@ private predicate copyValueBetweenArguments(Function f, int sourceArg, int destA
   or
   exists(FormattingFunction ff | ff = f |
     sourceArg in [ff.getFormatParameterIndex() .. maxArgIndex(f)] and
-    destArg = ff.getOutputParameterIndex()
+    destArg = ff.getOutputParameterIndex(false)
   )
 }
 

cpp/ql/test/query-tests/Likely Bugs/Format/WrongTypeFormatArguments/Linux_signed_chars/WrongTypeFormatArguments.expected

@@ -53,6 +53,8 @@
 | printf1.h:231:25:231:25 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:234:25:234:25 | i | This argument should be of type 'char *' but is of type 'int' |
 | printf1.h:235:22:235:22 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:276:32:276:32 | s | This argument should be of type 'int' but is of type 'char *' |
+| printf1.h:278:17:278:17 | s | This argument should be of type 'int' but is of type 'char *' |
 | real_world.h:61:21:61:22 | & ... | This argument should be of type 'int *' but is of type 'short *' |
 | real_world.h:62:22:62:23 | & ... | This argument should be of type 'short *' but is of type 'int *' |
 | real_world.h:63:22:63:24 | & ... | This argument should be of type 'short *' but is of type 'unsigned int *' |