Merge pull request github#15516 from microsoft/51-2cppnon-constant-format-alter-not-const-source

C++: Change sources in `NonConstantFormat.ql`

Author: MathiasVP

cpp/ql/src/Likely Bugs/Format/NonConstantFormat.ql

@@ -1,9 +1,13 @@
 /**
  * @name Non-constant format string
- * @description Passing a non-constant 'format' string to a printf-like function can lead
+ * @description Passing a value that is not a string literal 'format' string to a printf-like function can lead
  *              to a mismatch between the number of arguments defined by the 'format' and the number
  *              of arguments actually passed to the function. If the format string ultimately stems
  *              from an untrusted source, this can be used for exploits.
+ *              This query finds format strings coming from non-literal sources. Note that format strings of
+ *              type `const char*` it is still considered non-constant if the value is not coming from a string
+ *              literal. For example, for a parameter with type `const char*` of an exported function that is
+ *              used as a format string, there is no way to ensure the originating value was a string literal.
  * @kind problem
  * @problem.severity recommendation
  * @security-severity 9.3
@@ -16,135 +20,118 @@
  */
 
 import semmle.code.cpp.ir.dataflow.TaintTracking
-import semmle.code.cpp.models.implementations.GetText
 import semmle.code.cpp.commons.Printf
+import semmle.code.cpp.security.FlowSources
+import semmle.code.cpp.ir.dataflow.internal.ModelUtil
+import semmle.code.cpp.models.interfaces.DataFlow
+import semmle.code.cpp.models.interfaces.Taint
+import semmle.code.cpp.ir.IR
 
-// For the following `...gettext` functions, we assume that
-// all translations preserve the type and order of `%` specifiers
-// (and hence are safe to use as format strings).  This
-// assumption is hard-coded into the query.
-predicate whitelistFunction(Function f, int arg) {
-  // basic variations of gettext
-  f.getName() = "_" and arg = 0
-  or
-  exists(FunctionInput input |
-    f.(GetTextFunction).hasDataFlow(input, _) and
-    input.isParameterDeref(arg)
-  )
-}
-
-// we assume that ALL uses of the `_` macro (and calls to `gettext`)
-// return constant string literals
-predicate underscoreMacro(Expr e) {
-  exists(MacroInvocation mi |
-    mi.getMacroName() = "_" and
-    mi.getExpr() = e
-  )
-  or
-  e = any(GetTextFunction gettext).getACallToThisFunction()
+class UncalledFunction extends Function {
+  UncalledFunction() {
+    not exists(Call c | c.getTarget() = this) and
+    // Ignore functions that appear to be function pointers
+    // function pointers may be seen as uncalled statically
+    not exists(FunctionAccess fa | fa.getTarget() = this)
+  }
 }
 
 /**
- * Holds if `t` cannot hold a character array, directly or indirectly.
+ * Holds if `node` is a non-constant source of data flow for non-const format string detection.
+ * This is defined as either:
+ * 1) a `FlowSource`
+ * 2) a parameter of an 'uncalled' function
+ * 3) an argument to a function with no definition that is not known to define the output through its input
+ * 4) an out arg of a function with no definition that is not known to define the output through its input
+ *
+ * The latter two cases address identifying standard string manipulation libraries as input sources
+ * e.g., strcpy. More simply, functions without definitions that are known to manipulate the
+ * input to produce an output are not sources. Instead the ultimate source of input to these functions
+ * should be considered as the source.
+ *
+ * False Negative Implication: This approach has false negatives (fails to identify non-const sources)
+ * when the source is a field of a struct or object and the initialization is not observed statically.
+ * There are 3 general cases where this can occur:
+ * 1) Parameters of uncalled functions that are structs/objects and a field is accessed for a format string.
+ * 2) A local variable that is a struct/object and initialization of the field occurs in code that is unseen statically.
+ *    e.g., an object constructor isn't known statically, or a function sets fields
+ *    of a struct, but the function is not known statically.
+ * 3) A function meeting cases (3) and (4) above returns (through an out argument or return value)
+ *    a struct or object where a field containing a format string has been initialized.
+ *
+ * Note, uninitialized variables used as format strings are never detected by design.
+ * Uninitialized variables are a separate vulnerability concern and should be addressed by a separate query.
  */
-predicate cannotContainString(Type t, boolean isIndirect) {
-  isIndirect = false and
-  exists(Type unspecified |
-    unspecified = t.getUnspecifiedType() and
-    not unspecified instanceof UnknownType
-  |
-    unspecified instanceof BuiltInType or
-    unspecified instanceof IntegralOrEnumType
+predicate isNonConst(DataFlow::Node node) {
+  node instanceof FlowSource
+  or
+  // Parameters of uncalled functions that aren't const
+  exists(UncalledFunction f, Parameter p |
+    f.getAParameter() = p and
+    p = node.asParameter()
   )
-}
-
-predicate isNonConst(DataFlow::Node node, boolean isIndirect) {
-  exists(Expr e |
-    e = node.asExpr() and isIndirect = false
-    or
-    e = node.asIndirectExpr() and isIndirect = true
-  |
-    exists(FunctionCall fc | fc = e |
-      not (
-        whitelistFunction(fc.getTarget(), _) or
-        fc.getTarget().hasDefinition()
-      )
-    )
-    or
-    exists(Parameter p | p = e.(VariableAccess).getTarget() |
-      p.getFunction().getName() = "main" and p.getType() instanceof PointerType
+  or
+  // Consider as an input any out arg of a function or a function's return where the function is not:
+  // 1. a function with a known dataflow or taintflow from input to output and the `node` is the output
+  // 2. a function where there is a known definition
+  // i.e., functions that with unknown bodies and are not known to define the output through its input
+  //       are considered as possible non-const sources
+  // The function's output must also not be const to be considered a non-const source
+  exists(Function func, CallInstruction call |
+    // NOTE: could use `Call` getAnArgument() instead of `CallInstruction` but requires two
+    // variables representing the same call in ordoer to use `callOutput` below.
+    exists(Expr arg |
+      call.getPositionalArgumentOperand(_).getDef().getUnconvertedResultExpression() = arg and
+      arg = node.asDefiningArgument()
     )
     or
-    e instanceof CrementOperation
-    or
-    e instanceof AddressOfExpr
-    or
-    e instanceof ReferenceToExpr
-    or
-    e instanceof AssignPointerAddExpr
-    or
-    e instanceof AssignPointerSubExpr
-    or
-    e instanceof PointerArithmeticOperation
-    or
-    e instanceof FieldAccess
-    or
-    e instanceof PointerDereferenceExpr
-    or
-    e instanceof AddressOfExpr
-    or
-    e instanceof ExprCall
-    or
-    e instanceof NewArrayExpr
-    or
-    exists(Variable v | v = e.(VariableAccess).getTarget() |
-      v.getType().(ArrayType).getBaseType() instanceof CharType and
-      exists(AssignExpr ae |
-        ae.getLValue().(ArrayExpr).getArrayBase().(VariableAccess).getTarget() = v
-      )
+    call.getUnconvertedResultExpression() = node.asIndirectExpr()
+  |
+    func = call.getStaticCallTarget() and
+    not exists(FunctionOutput output |
+      // NOTE: we must include dataflow and taintflow. e.g., including only dataflow we will find sprintf
+      // variant function's output are now possible non-const sources
+      pragma[only_bind_out](func).(DataFlowFunction).hasDataFlow(_, output) or
+      pragma[only_bind_out](func).(TaintFunction).hasTaintFlow(_, output)
+    |
+      node = callOutput(call, output)
     )
+  ) and
+  not exists(Call c |
+    c.getTarget().hasDefinition() and
+    if node instanceof DataFlow::DefinitionByReferenceNode
+    then c.getAnArgument() = node.asDefiningArgument()
+    else c = [node.asExpr(), node.asIndirectExpr()]
   )
-  or
-  node instanceof DataFlow::DefinitionByReferenceNode and
-  isIndirect = true
-}
-
-pragma[noinline]
-predicate isBarrierNode(DataFlow::Node node) {
-  underscoreMacro([node.asExpr(), node.asIndirectExpr()])
-  or
-  exists(node.asExpr()) and
-  cannotContainString(node.getType(), false)
 }
 
+/**
+ * Holds if `sink` is a sink is a format string of any
+ * `FormattingFunctionCall`.
+ */
 predicate isSinkImpl(DataFlow::Node sink, Expr formatString) {
   [sink.asExpr(), sink.asIndirectExpr()] = formatString and
   exists(FormattingFunctionCall fc | formatString = fc.getArgument(fc.getFormatParameterIndex()))
 }
 
 module NonConstFlowConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) {
-    exists(boolean isIndirect, Type t |
-      isNonConst(source, isIndirect) and
-      t = source.getType() and
-      not cannotContainString(t, isIndirect)
-    )
-  }
+  predicate isSource(DataFlow::Node source) { isNonConst(source) }
 
   predicate isSink(DataFlow::Node sink) { isSinkImpl(sink, _) }
 
-  predicate isBarrier(DataFlow::Node node) { isBarrierNode(node) }
+  predicate isBarrier(DataFlow::Node node) {
+    // Ignore tracing non-const through array indices
+    exists(ArrayExpr a | a.getArrayOffset() = node.asExpr())
+  }
 }
 
 module NonConstFlow = TaintTracking::Global<NonConstFlowConfig>;
 
-from FormattingFunctionCall call, Expr formatString
+from FormattingFunctionCall call, Expr formatString, DataFlow::Node sink
 where
   call.getArgument(call.getFormatParameterIndex()) = formatString and
-  exists(DataFlow::Node sink |
-    NonConstFlow::flowTo(sink) and
-    isSinkImpl(sink, formatString)
-  )
+  NonConstFlow::flowTo(sink) and
+  isSinkImpl(sink, formatString)
 select formatString,
   "The format string argument to " + call.getTarget().getName() +
     " should be constant to prevent security issues and other potential errors."

cpp/ql/src/change-notes/2024-02-16-non-constant-format.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "non-constant format string" query (`cpp/non-constant-format`) has been updated to produce fewer false positives.

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.c

@@ -23,7 +23,7 @@ extern char *dcngettext (const char *__domainname, const char *__msgid1,
 extern char *any_random_function(const char *);
 
 #define NULL ((void*)0)
-#define _(X) any_random_function((X))
+#define _(X) gettext(X)
 
 int main(int argc, char **argv) {
 	if(argc > 1)
@@ -40,10 +40,9 @@ int main(int argc, char **argv) {
 	printf(gettext("%d arguments\n"), argc-1); // GOOD
 	printf(any_random_function("%d arguments\n"), argc-1); // BAD
 
-	// Even though `_` is mapped to `some_random_function` above,
-	// the following call should not  be flagged.
-	printf(_(any_random_function("%d arguments\n")),
-			argc-1); // GOOD
+
+
+	printf(_(any_random_function("%d arguments\n")), argc-1); // BAD
 
 	return 0;
 }

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

@@ -1,24 +1,21 @@
 | NonConstantFormat.c:30:10:30:16 | access to array | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | NonConstantFormat.c:41:9:41:27 | call to any_random_function | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| NonConstantFormat.c:45:9:45:48 | call to gettext | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | nested.cpp:21:23:21:26 | fmt0 | The format string argument to snprintf should be constant to prevent security issues and other potential errors. |
 | nested.cpp:79:32:79:38 | call to get_fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. |
 | nested.cpp:87:18:87:20 | fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. |
 | test.cpp:51:10:51:21 | call to make_message | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:57:12:57:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:60:12:60:21 | call to const_wash | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:61:12:61:26 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:62:12:62:17 | + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:63:12:63:18 | * ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:64:12:64:18 | & ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:65:12:65:39 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:67:10:67:35 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:70:12:70:20 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:76:12:76:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:82:12:82:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:88:12:88:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:93:12:93:18 | ++ ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:100:12:100:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:110:12:110:24 | new[] | The format string argument to printf should be constant to prevent security issues and other potential errors. |
-| test.cpp:115:12:115:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:130:20:130:26 | access to array | The format string argument to sprintf should be constant to prevent security issues and other potential errors. |
 | test.cpp:157:12:157:15 | data | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:170:12:170:14 | res | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:195:31:195:33 | str | The format string argument to StringCchPrintfW should be constant to prevent security issues and other potential errors. |
+| test.cpp:197:11:197:14 | wstr | The format string argument to wprintf should be constant to prevent security issues and other potential errors. |
+| test.cpp:205:12:205:20 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:206:12:206:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:211:12:211:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:217:12:217:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:223:12:223:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:228:12:228:18 | ++ ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:235:12:235:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:242:12:242:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
+| test.cpp:247:12:247:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/nested.cpp

@@ -18,7 +18,7 @@ extern "C" int snprintf ( char * s, int n, const char * format, ... );
 struct A {
   void do_print(const char *fmt0) {
     char buf[32];
-    snprintf(buf, 32, fmt0); // GOOD [FALSE POSITIVE]
+    snprintf(buf, 32, fmt0); // BAD, all paths from unknown const char*, not assuming literal
   }
 };
 
@@ -34,12 +34,12 @@ struct C {
   void do_some_printing(const char *fmt) {
     b.do_printing(fmt);
   }
-  const char *ext_fmt_str(void);
+  const char *ext_fmt_str(void); // NOTE: not assuming result is literal
 };
 
 void foo(void) {
   C c;
-  c.do_some_printing(c.ext_fmt_str()); // BAD [NOT DETECTED]
+  c.do_some_printing(c.ext_fmt_str()); 
 }
 
 struct some_class {
@@ -76,12 +76,12 @@ void diagnostic(const char *fmt, ...)
 }
 
 void bar(void) {
-    diagnostic (some_instance->get_fmt());  // BAD
+    diagnostic (some_instance->get_fmt());  // BAD const char* but not assuming literal
 }
 
 namespace ns {
 
-  class blab {
+  class blab { 
     void out1(void) {
       char *fmt = (char *)__builtin_alloca(10);
       diagnostic(fmt);  // BAD