Merge pull request github#6198 from RasmusWL/fix-cleartext-logging

Python: Some minor fixes to `py/clear-text-logging-sensitive-data`

Author: tausbn

python/ql/src/semmle/python/dataflow/new/SensitiveDataSources.qll

@@ -76,28 +76,16 @@ private module SensitiveDataModeling {
   }
 
   /**
-   * Gets a reference to a string constant that, if used as the key in a lookup,
-   * indicates the presence of sensitive data with `classification`.
-   */
-  private DataFlow::LocalSourceNode sensitiveLookupStringConst(
-    DataFlow::TypeTracker t, SensitiveDataClassification classification
-  ) {
-    t.start() and
-    nameIndicatesSensitiveData(result.asExpr().(StrConst).getText(), classification)
-    or
-    exists(DataFlow::TypeTracker t2 |
-      result = sensitiveLookupStringConst(t2, classification).track(t2, t)
-    )
-  }
-
-  /**
-   * Gets a reference to a string constant that, if used as the key in a lookup,
-   * indicates the presence of sensitive data with `classification`.
-   *
-   * Also see `extraStepForCalls`.
+   * Gets a reference (in local scope) to a string constant that, if used as the key in
+   * a lookup, indicates the presence of sensitive data with `classification`.
    */
   DataFlow::Node sensitiveLookupStringConst(SensitiveDataClassification classification) {
-    sensitiveLookupStringConst(DataFlow::TypeTracker::end(), classification).flowsTo(result)
+    // Note: If this is implemented with type-tracking, we will get cross-talk as
+    // illustrated in python/ql/test/experimental/dataflow/sensitive-data/test.py
+    exists(DataFlow::LocalSourceNode source |
+      nameIndicatesSensitiveData(source.asExpr().(StrConst).getText(), classification) and
+      source.flowsTo(result)
+    )
   }
 
   /** A function call that is considered a source of sensitive data. */
@@ -118,6 +106,8 @@ private module SensitiveDataModeling {
   /**
    * Tracks any modeled source of sensitive data (with any classification),
    * to limit the scope of `extraStepForCalls`. See it's QLDoc for more context.
+   *
+   * Also see `extraStepForCalls`.
    */
   private DataFlow::LocalSourceNode possibleSensitiveCallable(DataFlow::TypeTracker t) {
     t.start() and
@@ -129,6 +119,8 @@ private module SensitiveDataModeling {
   /**
    * Tracks any modeled source of sensitive data (with any classification),
    * to limit the scope of `extraStepForCalls`. See it's QLDoc for more context.
+   *
+   * Also see `extraStepForCalls`.
    */
   private DataFlow::Node possibleSensitiveCallable() {
     possibleSensitiveCallable(DataFlow::TypeTracker::end()).flowsTo(result)

python/ql/src/semmle/python/security/dataflow/CleartextLoggingCustomizations.qll

@@ -51,7 +51,7 @@ module CleartextLogging {
   }
 
   /** A piece of data printed, considered as a flow sink. */
-  class PrintedDataAsSink extends Sink, DataFlow::CallCfgNode {
+  class PrintedDataAsSink extends Sink {
     PrintedDataAsSink() {
       this = API::builtin("print").getACall().getArg(_)
       or

python/ql/test/experimental/dataflow/sensitive-data/test.py

@@ -78,3 +78,37 @@ def my_func(password): # $ SensitiveDataSource=password
 
 from not_found import password2 as foo # $ SensitiveDataSource=password
 print(foo) # $ SensitiveUse=password
+
+# ------------------------------------------------------------------------------
+# cross-talk between different calls
+# ------------------------------------------------------------------------------
+
+# Case 1: providing name as argument
+
+_configuration = {"sleep_timer": 5, "mysql_password": "1234"}
+
+def get_config(key):
+    # Treating this as a SensitiveDataSource is questionable, since that will result in
+    # _all_ calls to `get_config` being treated as giving sensitive data
+    return _configuration[key]
+
+foo = get_config("mysql_password")
+print(foo) # $ MISSING: SensitiveUse=password
+
+bar = get_config("sleep_timer")
+print(bar)
+
+# Case 2: Providing function as argument
+
+def call_wrapper(func):
+    print("Will call", func)
+    # Treating this as a SensitiveDataSource is questionable, since that will result in
+    # _all_ calls to `call_wrapper` being treated as giving sensitive data
+    return func() # $ SensitiveDataSource=password
+
+foo = call_wrapper(get_password)
+print(foo) # $ SensitiveUse=password
+
+harmless = lambda: "bar"
+bar = call_wrapper(harmless)
+print(bar) # $ SPURIOUS: SensitiveUse=password