Merge pull request #14637 from MathiasVP/dataflow-for-realloc

C++: Add a taint model for `realloc`

Author: MathiasVP

cpp/ql/lib/change-notes/2023-10-30-realloc-flow.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added taint models for `realloc` and related functions.

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

@@ -5,6 +5,7 @@
  */
 
 import semmle.code.cpp.models.interfaces.Allocation
+import semmle.code.cpp.models.interfaces.Taint
 
 /**
  * An allocation function (such as `malloc`) that has an argument for the size
@@ -121,7 +122,7 @@ private class CallocAllocationFunction extends AllocationFunction {
  * An allocation function (such as `realloc`) that has an argument for the size
  * in bytes, and an argument for an existing pointer that is to be reallocated.
  */
-private class ReallocAllocationFunction extends AllocationFunction {
+private class ReallocAllocationFunction extends AllocationFunction, TaintFunction {
   int sizeArg;
   int reallocArg;
 
@@ -151,6 +152,10 @@ private class ReallocAllocationFunction extends AllocationFunction {
   override int getSizeArg() { result = sizeArg }
 
   override int getReallocPtrArg() { result = reallocArg }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    input.isParameterDeref(this.getReallocPtrArg()) and output.isReturnValueDeref()
+  }
 }
 
 /**

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

@@ -9,18 +9,17 @@ import semmle.code.cpp.models.interfaces.DataFlow
 import semmle.code.cpp.models.interfaces.Alias
 import semmle.code.cpp.models.interfaces.SideEffect
 
-/**
- * The standard function `memset` and its assorted variants
- */
-private class MemsetFunction extends ArrayFunction, DataFlowFunction, AliasFunction,
+private class MemsetFunctionModel extends ArrayFunction, DataFlowFunction, AliasFunction,
   SideEffectFunction
 {
-  MemsetFunction() {
+  MemsetFunctionModel() {
     this.hasGlobalOrStdOrBslName("memset")
     or
     this.hasGlobalOrStdName("wmemset")
     or
-    this.hasGlobalName([bzero(), "__builtin_memset", "__builtin_memset_chk"])
+    this.hasGlobalName([
+        bzero(), "__builtin_memset", "__builtin_memset_chk", "RtlZeroMemory", "RtlSecureZeroMemory"
+      ])
   }
 
   override predicate hasArrayOutput(int bufParam) { bufParam = 0 }
@@ -60,3 +59,8 @@ private class MemsetFunction extends ArrayFunction, DataFlowFunction, AliasFunct
 }
 
 private string bzero() { result = ["bzero", "explicit_bzero"] }
+
+/**
+ * The standard function `memset` and its assorted variants
+ */
+class MemsetFunction extends Function instanceof MemsetFunctionModel { }

cpp/ql/src/Security/CWE/CWE-497/ExposedSystemData.ql

@@ -15,6 +15,7 @@
 import cpp
 import semmle.code.cpp.ir.dataflow.TaintTracking
 import semmle.code.cpp.models.interfaces.FlowSource
+import semmle.code.cpp.models.implementations.Memset
 import ExposedSystemData::PathGraph
 import SystemData
 
@@ -28,6 +29,10 @@ module ExposedSystemDataConfig implements DataFlow::ConfigSig {
       fc.getArgument(arg).getAChild*() = sink.asIndirectExpr()
     )
   }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node.asIndirectArgument() = any(MemsetFunction func).getACallToThisFunction().getAnArgument()
+  }
 }
 
 module ExposedSystemData = TaintTracking::Global<ExposedSystemDataConfig>;

cpp/ql/src/Security/CWE/CWE-497/PotentiallyExposedSystemData.ql

@@ -28,6 +28,7 @@ import cpp
 import semmle.code.cpp.ir.dataflow.TaintTracking
 import semmle.code.cpp.models.interfaces.FlowSource
 import semmle.code.cpp.security.OutputWrite
+import semmle.code.cpp.models.implementations.Memset
 import PotentiallyExposedSystemData::PathGraph
 import SystemData
 
@@ -49,6 +50,10 @@ module PotentiallyExposedSystemDataConfig implements DataFlow::ConfigSig {
       else child = sink.asExpr()
     )
   }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node.asIndirectArgument() = any(MemsetFunction func).getACallToThisFunction().getAnArgument()
+  }
 }
 
 module PotentiallyExposedSystemData = TaintTracking::Global<PotentiallyExposedSystemDataConfig>;

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

@@ -6643,6 +6643,9 @@ WARNING: Module TaintTracking has been deprecated and may be removed in future (
 | taint.cpp:729:27:729:32 | endptr | taint.cpp:729:26:729:32 | & ... |  |
 | taint.cpp:731:7:731:12 | ref arg endptr | taint.cpp:732:8:732:13 | endptr |  |
 | taint.cpp:732:8:732:13 | endptr | taint.cpp:732:7:732:13 | * ... | TAINT |
+| taint.cpp:738:17:738:31 | call to indirect_source | taint.cpp:739:30:739:35 | source |  |
+| taint.cpp:739:22:739:28 | call to realloc | taint.cpp:740:7:740:10 | dest |  |
+| taint.cpp:739:30:739:35 | source | taint.cpp:739:22:739:28 | call to realloc | TAINT |
 | vector.cpp:16:43:16:49 | source1 | vector.cpp:17:26:17:32 | source1 |  |
 | vector.cpp:16:43:16:49 | source1 | vector.cpp:31:38:31:44 | source1 |  |
 | vector.cpp:17:21:17:33 | call to vector | vector.cpp:19:14:19:14 | v |  |

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

@@ -730,4 +730,12 @@ void test_strtol(char *source) {
 	sink(l); // $ ast,ir
 	sink(endptr); // $ ast,ir
 	sink(*endptr); // $ ast,ir
+}
+
+void *realloc(void *, size_t);
+
+void test_realloc() {
+	char *source = indirect_source();
+	char *dest = (char*)realloc(source, 16);
+	sink(dest); // $ ir MISSING: ast
 }

cpp/ql/test/query-tests/Security/CWE/CWE-497/semmle/tests/PotentiallyExposedSystemData.expected

@@ -10,6 +10,7 @@ edges
 | tests.cpp:131:14:131:35 | call to getenv indirection | tests.cpp:107:30:107:32 | msg indirection |
 | tests.cpp:132:14:132:35 | call to getenv indirection | tests.cpp:114:30:114:32 | msg indirection |
 | tests.cpp:133:14:133:35 | call to getenv indirection | tests.cpp:122:30:122:32 | msg indirection |
+| tests.cpp:139:17:139:22 | call to getenv indirection | tests.cpp:141:15:141:20 | secret indirection |
 | tests_passwd.cpp:16:8:16:15 | call to getpwnam indirection | tests_passwd.cpp:18:29:18:31 | pwd indirection |
 | tests_passwd.cpp:16:8:16:15 | call to getpwnam indirection | tests_passwd.cpp:19:26:19:28 | pwd indirection |
 nodes
@@ -37,6 +38,8 @@ nodes
 | tests.cpp:132:14:132:35 | call to getenv indirection | semmle.label | call to getenv indirection |
 | tests.cpp:133:14:133:35 | call to getenv indirection | semmle.label | call to getenv indirection |
 | tests.cpp:133:14:133:35 | call to getenv indirection | semmle.label | call to getenv indirection |
+| tests.cpp:139:17:139:22 | call to getenv indirection | semmle.label | call to getenv indirection |
+| tests.cpp:141:15:141:20 | secret indirection | semmle.label | secret indirection |
 | tests_passwd.cpp:16:8:16:15 | call to getpwnam indirection | semmle.label | call to getpwnam indirection |
 | tests_passwd.cpp:18:29:18:31 | pwd indirection | semmle.label | pwd indirection |
 | tests_passwd.cpp:19:26:19:28 | pwd indirection | semmle.label | pwd indirection |
@@ -56,5 +59,6 @@ subpaths
 | tests.cpp:119:7:119:12 | buffer indirection | tests.cpp:132:14:132:35 | call to getenv indirection | tests.cpp:119:7:119:12 | buffer indirection | This operation potentially exposes sensitive system data from $@. | tests.cpp:132:14:132:35 | call to getenv indirection | call to getenv indirection |
 | tests.cpp:124:15:124:17 | msg indirection | tests.cpp:133:14:133:35 | call to getenv indirection | tests.cpp:124:15:124:17 | msg indirection | This operation potentially exposes sensitive system data from $@. | tests.cpp:133:14:133:35 | call to getenv indirection | call to getenv indirection |
 | tests.cpp:133:14:133:35 | call to getenv indirection | tests.cpp:133:14:133:35 | call to getenv indirection | tests.cpp:133:14:133:35 | call to getenv indirection | This operation potentially exposes sensitive system data from $@. | tests.cpp:133:14:133:35 | call to getenv indirection | call to getenv indirection |
+| tests.cpp:141:15:141:20 | secret indirection | tests.cpp:139:17:139:22 | call to getenv indirection | tests.cpp:141:15:141:20 | secret indirection | This operation potentially exposes sensitive system data from $@. | tests.cpp:139:17:139:22 | call to getenv indirection | call to getenv indirection |
 | tests_passwd.cpp:18:29:18:31 | pwd indirection | tests_passwd.cpp:16:8:16:15 | call to getpwnam indirection | tests_passwd.cpp:18:29:18:31 | pwd indirection | This operation potentially exposes sensitive system data from $@. | tests_passwd.cpp:16:8:16:15 | call to getpwnam indirection | call to getpwnam indirection |
 | tests_passwd.cpp:19:26:19:28 | pwd indirection | tests_passwd.cpp:16:8:16:15 | call to getpwnam indirection | tests_passwd.cpp:19:26:19:28 | pwd indirection | This operation potentially exposes sensitive system data from $@. | tests_passwd.cpp:16:8:16:15 | call to getpwnam indirection | call to getpwnam indirection |

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

@@ -132,3 +132,13 @@ void test5()
 	myOutputFn4(getenv("SECRET_TOKEN")); // BAD: outputs the SECRET_TOKEN environment variable
 	myOutputFn5(getenv("SECRET_TOKEN")); // BAD: outputs the SECRET_TOKEN environment variable
 }
+
+void RtlZeroMemory(void* dst, size_t len);
+
+void test_clear_memory(char *username) {
+	char* secret = getenv("SECRET_TOKEN");
+
+	printf("%s", secret); // BAD
+	RtlZeroMemory(secret, 1024);
+	printf("%s", secret); // GOOD
+}