Updating non-const source logic and associated tests and expected files.

Author: bdrodes

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

@@ -26,51 +26,94 @@ import semmle.code.cpp.ir.implementation.raw.Instruction
 class UncalledFunction extends Function {
   UncalledFunction() {
     not exists(Call c | c.getTarget() = this) and
-    not this.(MemberFunction).overrides(_)
+    // TODO: Need rationale here, added based on suggestion
+    //but unclear of the scenario being avoided
+    not this.(MemberFunction).overrides(_) and
+    // Ignore functions that appear to be function pointers
+    not exists(FunctionAccess fa | fa.getTarget() = this)
   }
 }
 
 /**
  * Holds if `node` is a non-constant source of data flow.
  * This is defined as either:
  * 1) a `FlowSource`
- * 2) a parameter of an 'uncalled' function (i.e., a function that is not called in the program)
+ * 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
  *
+ * With exception to `FlowSource` all non-const values have a type that is not const
+ * (declared without a `const` specifier)
+ * ASSUMPTION: any const values are assumed to be static if their assignment is not seen (
+ * i.e., assuming users did not get non-const data and cast into a const
+ *
  * The latter two cases address identifying standard string manipulation libraries as input sources
  * e.g., strcpy, but it will identify unknown function calls as possible non-constant source
  * since it cannot be determined if the out argument or return is constant.
  */
 predicate isNonConst(DataFlow::Node node) {
   node instanceof FlowSource
   or
-  exists(UncalledFunction f | f.getAParameter() = node.asParameter())
+  // Parameters of uncalled functions that aren't const
+  exists(UncalledFunction f, Parameter p, Type t |
+    not t.isConst() and
+    f.getAParameter() = p and
+    p = node.asParameter() and
+    not f.getType().getUnderlyingType().isConst() and
+    (
+      // const char* means (const char)*, so the pointer is not const, the pointed to value is
+      // Grab the base type if a pointer, as this is the type we will check for const-ness
+      if p.getType().getUnderlyingType() instanceof PointerType
+      then t = p.getType().getUnderlyingType().(PointerType).getBaseType()
+      else t = p.getType().getUnderlyingType()
+    )
+  )
   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
-  (
-    node instanceof DataFlow::DefinitionByReferenceNode or
-    node.asIndirectExpr() instanceof Call
-  ) and
-  not exists(Function func, FunctionInput input, FunctionOutput output, CallInstruction call |
-    // 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
-    (
-      func.(DataFlowFunction).hasDataFlow(input, output) or
-      func.(TaintFunction).hasTaintFlow(input, output)
+  // The function's output must also not be const to be considered a non-const source
+  exists(Type t |
+    not t.isConst() and
+    exists(Call c |
+      exists(Expr arg | c.getAnArgument() = arg |
+        arg = node.asDefiningArgument() and
+        (
+          // const char* means (const char)*, so the pointer is not const, the pointed to value is
+          // Grab the base type if a pointer, as this is the type we will check for const-ness
+          if arg.getType().getUnderlyingType() instanceof PointerType
+          then t = arg.getType().getUnderlyingType().(PointerType).getBaseType()
+          else t = arg.getType().getUnderlyingType()
+        )
+      )
+      or
+      c = node.asIndirectExpr() and
+      (
+        // const char* means (const char)*, so the pointer is not const, the pointed to value is
+        // Grab the base type if a pointer, as this is the type we will check for const-ness
+        if c.getType().getUnderlyingType() instanceof PointerType
+        then t = c.getType().getUnderlyingType().(PointerType).getBaseType()
+        else t = c.getType().getUnderlyingType()
+      )
+    ) and
+    not exists(Function func, FunctionInput input, FunctionOutput output, CallInstruction call |
+      // 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
+      (
+        func.(DataFlowFunction).hasDataFlow(input, output) or
+        func.(TaintFunction).hasTaintFlow(input, output)
+      ) and
+      node = callOutput(call, output) and
+      call.getStaticCallTarget() = func
     ) and
-    node = callOutput(call, output) and
-    call.getStaticCallTarget() = func
-  ) 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()]
+    not exists(Call c |
+      c.getTarget().hasDefinition() and
+      if node instanceof DataFlow::DefinitionByReferenceNode
+      then c.getAnArgument() = node.asDefiningArgument()
+      else c = [node.asExpr(), node.asIndirectExpr()]
+    )
   )
 }
 

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

@@ -1,15 +1,19 @@
 | 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. |
-| 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. |
+| 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: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: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); // BAD through call from c.do_some_printing(c.ext_fmt_str())
+    snprintf(buf, 32, fmt0); // GOOD, all paths to year use const char*
   }
 };
 
@@ -76,12 +76,12 @@ void diagnostic(const char *fmt, ...)
 }
 
 void bar(void) {
-    diagnostic (some_instance->get_fmt());  // BAD
+    diagnostic (some_instance->get_fmt());  // GOOD get_fmt is const char* assumed static
 }
 
 namespace ns {
 
-  class blab {
+  class blab { 
     void out1(void) {
       char *fmt = (char *)__builtin_alloca(10);
       diagnostic(fmt);  // BAD

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

@@ -107,7 +107,7 @@ int main(int argc, char **argv) {
     printf(hello); // GOOD
   }
   if (gettext_debug) {
-    printf(new char[100]); // BAD [FALSE NEGATIVE]
+    printf(new char[100]); // BAD [FALSE NEGATIVE: uninitialized value not considered a source]
   }
   {
     const char *hello = "Hello, World\n";
@@ -197,7 +197,7 @@ void wchar_t_test_bad(wchar_t* str){
   wprintf(wstr); // BAD
 }
 
-const char* get_string();
+char* get_string();
 
 void pointer_arithmetic_test_on_bad_string(){
    {