Merge pull request #3952 from MathiasVP/output-parameter-index-for-UserDefinedFormattingFunction

C++: Add getOutputParameterIndex override to UserDefinedFormattingFunction class.

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

@@ -49,6 +49,18 @@ predicate primitiveVariadicFormatter(TopLevelFunction f, int formatParamIndex) {
   )
 }
 
+/**
+ * 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
@@ -57,6 +69,26 @@ private predicate callsVariadicFormatter(Function f, int formatParamIndex) {
   )
 }
 
+private predicate callsVariadicFormatterOutput(Function f, int outputParamIndex) {
+  exists(FunctionCall fc, int i |
+    fc.getEnclosingFunction() = f and
+    variadicFormatterOutput(fc.getTarget(), i) and
+    fc.getArgument(i) = f.getParameter(outputParamIndex).getAnAccess()
+  )
+}
+
+/**
+ * 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.
+ */
+private predicate variadicFormatterOutput(Function f, int outputParamIndex) {
+  primitiveVariadicFormatterOutput(f, outputParamIndex)
+  or
+  not f.isVarargs() and
+  callsVariadicFormatterOutput(f, outputParamIndex)
+}
+
 /**
  * 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`.
@@ -78,6 +110,8 @@ class UserDefinedFormattingFunction extends FormattingFunction {
   UserDefinedFormattingFunction() { isVarargs() and callsVariadicFormatter(this, _) }
 
   override int getFormatParameterIndex() { callsVariadicFormatter(this, result) }
+
+  override int getOutputParameterIndex() { callsVariadicFormatterOutput(this, result) }
 }
 
 /**

cpp/ql/test/library-tests/dataflow/taint-tests/format.cpp

@@ -112,7 +112,7 @@ void test1()
 	{
 		char buffer[256] = {0};
 		sink(mysprintf(buffer, 256, "%s", string::source()));
-		sink(buffer); // tainted [NOT DETECTED - implement UserDefinedFormattingFunction.getOutputParameterIndex()]
+		sink(buffer); // tainted
 	}
 
 	{

cpp/ql/test/library-tests/dataflow/taint-tests/localTaint.expected

@@ -206,6 +206,8 @@
 | format.cpp:113:21:113:24 | {...} | format.cpp:115:8:115:13 | buffer |  |
 | format.cpp:113:23:113:23 | 0 | format.cpp:113:21:113:24 | {...} | TAINT |
 | format.cpp:114:18:114:23 | ref arg buffer | format.cpp:115:8:115:13 | buffer |  |
+| format.cpp:114:31:114:34 | %s | format.cpp:114:18:114:23 | ref arg buffer | TAINT |
+| format.cpp:114:37:114:50 | call to source | format.cpp:114:18:114:23 | ref arg buffer | TAINT |
 | format.cpp:119:10:119:11 | 0 | format.cpp:120:29:120:29 | i |  |
 | format.cpp:119:10:119:11 | 0 | format.cpp:121:8:121:8 | i |  |
 | format.cpp:120:28:120:29 | ref arg & ... | format.cpp:120:29:120:29 | i [inner post update] |  |

cpp/ql/test/library-tests/dataflow/taint-tests/taint.expected

@@ -22,6 +22,7 @@
 | format.cpp:100:8:100:13 | buffer | format.cpp:99:30:99:43 | call to source |
 | format.cpp:105:8:105:13 | buffer | format.cpp:104:31:104:45 | call to source |
 | format.cpp:110:8:110:14 | wbuffer | format.cpp:109:38:109:52 | call to source |
+| format.cpp:115:8:115:13 | buffer | format.cpp:114:37:114:50 | call to source |
 | format.cpp:157:7:157:22 | access to array | format.cpp:147:12:147:25 | call to source |
 | format.cpp:158:7:158:27 | ... + ... | format.cpp:148:16:148:30 | call to source |
 | movableclass.cpp:44:8:44:9 | s1 | movableclass.cpp:39:21:39:26 | call to source |

cpp/ql/test/library-tests/dataflow/taint-tests/test_diff.expected

@@ -22,6 +22,7 @@
 | format.cpp:100:8:100:13 | format.cpp:99:30:99:43 | AST only |
 | format.cpp:105:8:105:13 | format.cpp:104:31:104:45 | AST only |
 | format.cpp:110:8:110:14 | format.cpp:109:38:109:52 | AST only |
+| format.cpp:115:8:115:13 | format.cpp:114:37:114:50 | AST only |
 | movableclass.cpp:44:8:44:9 | movableclass.cpp:39:21:39:26 | AST only |
 | movableclass.cpp:45:8:45:9 | movableclass.cpp:40:23:40:28 | AST only |
 | movableclass.cpp:46:8:46:9 | movableclass.cpp:42:8:42:13 | AST only |

cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/OverrunWrite.expected

@@ -2,3 +2,4 @@
 | tests.cpp:259:2:259:8 | call to sprintf | This 'call to sprintf' operation requires 17 bytes but the destination is only 10 bytes. |
 | tests.cpp:272:2:272:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. |
 | tests.cpp:273:2:273:8 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. |
+| tests.cpp:308:3:308:9 | call to sprintf | This 'call to sprintf' operation requires 9 bytes but the destination is only 8 bytes. |

cpp/ql/test/query-tests/Security/CWE/CWE-242/semmle/tests/tests.cpp

@@ -289,3 +289,22 @@ void test5(va_list args, float f)
 	vsprintf(buffer4, "123", args); // GOOD
 	vsprintf(buffer4, "1234", args); // BAD: buffer overflow [NOT DETECTED]
 }
+
+namespace custom_sprintf_impl {
+	int sprintf(char *buf, const char *format, ...)
+	{
+		__builtin_va_list args;
+		int i;
+
+		__builtin_va_start(args, format);
+		i = vsprintf(buf, format, args);
+		__builtin_va_end(args);
+		return i;
+	}
+
+	void regression_test1()
+	{
+		char buffer8[8];
+		sprintf(buffer8, "12345678"); // BAD: potential buffer overflow
+	}
+}