Merge pull request #7652 from RasmusWL/cleartext-remove-fps

Python: Remove usernames as sensitive source for cleartext queries

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

@@ -40,6 +40,10 @@ module CleartextLogging {
    * A source of sensitive data, considered as a flow source.
    */
   class SensitiveDataSourceAsSource extends Source, SensitiveDataSource {
+    SensitiveDataSourceAsSource() {
+      not SensitiveDataSource.super.getClassification() = SensitiveDataClassification::id()
+    }
+
     override SensitiveDataClassification getClassification() {
       result = SensitiveDataSource.super.getClassification()
     }

python/ql/lib/semmle/python/security/dataflow/CleartextStorageCustomizations.qll

@@ -39,6 +39,10 @@ module CleartextStorage {
    * A source of sensitive data, considered as a flow source.
    */
   class SensitiveDataSourceAsSource extends Source, SensitiveDataSource {
+    SensitiveDataSourceAsSource() {
+      not SensitiveDataSource.super.getClassification() = SensitiveDataClassification::id()
+    }
+
     override SensitiveDataClassification getClassification() {
       result = SensitiveDataSource.super.getClassification()
     }

python/ql/src/change-notes/2021-01-19-remove-cleartext-fps.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* User names and other account information is no longer considered to be sensitive data for the queries `py/clear-text-logging-sensitive-data` and `py/clear-text-storage-sensitive-data`, since this lead to many false positives.

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

@@ -112,3 +112,16 @@ print(foo) # $ SensitiveUse=password
 harmless = lambda: "bar"
 bar = call_wrapper(harmless)
 print(bar) # $ SPURIOUS: SensitiveUse=password
+
+# ------------------------------------------------------------------------------
+# cross-talk in dictionary.
+# ------------------------------------------------------------------------------
+
+from unknown_settings import password # $ SensitiveDataSource=password
+
+print(password) # $ SensitiveUse=password
+_config = {"sleep_timer": 5, "mysql_password": password}
+
+# since we have taint-step from store of `password`, we will consider any item in the
+# dictionary to be a password :(
+print(_config["sleep_timer"]) # $ SPURIOUS: SensitiveUse=password

python/ql/test/query-tests/Security/CWE-312-CleartextLogging/CleartextLogging.expected

@@ -4,6 +4,8 @@ edges
 | test.py:19:16:19:29 | ControlFlowNode for get_password() | test.py:23:58:23:65 | ControlFlowNode for password |
 | test.py:19:16:19:29 | ControlFlowNode for get_password() | test.py:27:40:27:47 | ControlFlowNode for password |
 | test.py:19:16:19:29 | ControlFlowNode for get_password() | test.py:30:58:30:65 | ControlFlowNode for password |
+| test.py:65:14:68:5 | ControlFlowNode for Dict | test.py:69:11:69:31 | ControlFlowNode for Subscript |
+| test.py:67:21:67:37 | ControlFlowNode for Attribute | test.py:65:14:68:5 | ControlFlowNode for Dict |
 nodes
 | test.py:19:16:19:29 | ControlFlowNode for get_password() | semmle.label | ControlFlowNode for get_password() |
 | test.py:20:48:20:55 | ControlFlowNode for password | semmle.label | ControlFlowNode for password |
@@ -15,6 +17,9 @@ nodes
 | test.py:37:11:37:24 | ControlFlowNode for get_password() | semmle.label | ControlFlowNode for get_password() |
 | test.py:39:22:39:35 | ControlFlowNode for get_password() | semmle.label | ControlFlowNode for get_password() |
 | test.py:40:22:40:35 | ControlFlowNode for get_password() | semmle.label | ControlFlowNode for get_password() |
+| test.py:65:14:68:5 | ControlFlowNode for Dict | semmle.label | ControlFlowNode for Dict |
+| test.py:67:21:67:37 | ControlFlowNode for Attribute | semmle.label | ControlFlowNode for Attribute |
+| test.py:69:11:69:31 | ControlFlowNode for Subscript | semmle.label | ControlFlowNode for Subscript |
 subpaths
 #select
 | test.py:20:48:20:55 | ControlFlowNode for password | test.py:19:16:19:29 | ControlFlowNode for get_password() | test.py:20:48:20:55 | ControlFlowNode for password | $@ is logged here. | test.py:19:16:19:29 | ControlFlowNode for get_password() | Sensitive data (password) |
@@ -26,3 +31,4 @@ subpaths
 | test.py:37:11:37:24 | ControlFlowNode for get_password() | test.py:37:11:37:24 | ControlFlowNode for get_password() | test.py:37:11:37:24 | ControlFlowNode for get_password() | $@ is logged here. | test.py:37:11:37:24 | ControlFlowNode for get_password() | Sensitive data (password) |
 | test.py:39:22:39:35 | ControlFlowNode for get_password() | test.py:39:22:39:35 | ControlFlowNode for get_password() | test.py:39:22:39:35 | ControlFlowNode for get_password() | $@ is logged here. | test.py:39:22:39:35 | ControlFlowNode for get_password() | Sensitive data (password) |
 | test.py:40:22:40:35 | ControlFlowNode for get_password() | test.py:40:22:40:35 | ControlFlowNode for get_password() | test.py:40:22:40:35 | ControlFlowNode for get_password() | $@ is logged here. | test.py:40:22:40:35 | ControlFlowNode for get_password() | Sensitive data (password) |
+| test.py:69:11:69:31 | ControlFlowNode for Subscript | test.py:67:21:67:37 | ControlFlowNode for Attribute | test.py:69:11:69:31 | ControlFlowNode for Subscript | $@ is logged here. | test.py:67:21:67:37 | ControlFlowNode for Attribute | Sensitive data (password) |

python/ql/test/query-tests/Security/CWE-312-CleartextLogging/test.py

@@ -39,6 +39,36 @@ def print_password():
     sys.stdout.write(get_password()) # NOT OK
     sys.stderr.write(get_password()) # NOT OK
 
+
+def FPs(account, account_id):
+    # we assume that any account parameter is sensitive (id/username)
+    # https://github.com/github/codeql/issues/6363
+    print(account) # OK
+
+    # https://github.com/github/codeql/issues/6927
+    arn = f"arn:aws:iam::{account_id}:role/cfripper-access"
+    logging.info(f"Preparing to assume role: {arn}") # OK
+
+    # Harmless UUIDs
+    # https://github.com/github/codeql/issues/6726
+    # https://github.com/github/codeql/issues/7497
+    x = generate_uuid4()
+    print(x) # OK
+
+    # username not considered sensitive
+    # https://github.com/github/codeql/issues/7116
+    logging.error("Misc Exception. User %s: %s", request.user.username)
+
+    # dictionary taint-flow cross-talk
+    # https://github.com/github/codeql/issues/6380
+    import settings
+    config = {
+        "sleep_timer": 5,
+        "password": settings.password
+    }
+    print(config["sleep_timer"]) # OK
+
+
 if __name__ == "__main__":
     logging.basicConfig(level=logging.DEBUG)
     log_password()

python/ql/test/query-tests/Security/CWE-312-CleartextStorage/test.py

@@ -8,3 +8,14 @@ def write_cert(filename):
         file.write(cert) # NOT OK
         lines = [cert + "\n"]
         file.writelines(lines) # NOT OK
+
+def FPs():
+    # just like for cleartext-logging see that file for more elaborate tests
+    #
+    # this part is just to make sure the two queries are in line with what is considered
+    # sensitive information.
+
+    with open(filename, "w") as file:
+        # Harmless UUIDs
+        x = generate_uuid4()
+        file.write(x) # OK