Merge pull request #14357 from geoffw0/commandinject3

Swift: Replace two additional taint steps with implicit reads
github · Oct 3, 2023 · c518f39 · c518f39
2 parents 8224f17 + bbd3c66
commit c518f39
Show file tree

Hide file tree

Showing 6 changed files with 296 additions and 177 deletions.
diff --git a/swift/ql/lib/codeql/swift/security/CleartextStorageDatabaseExtensions.qll b/swift/ql/lib/codeql/swift/security/CleartextStorageDatabaseExtensions.qll
@@ -128,15 +128,9 @@ private class CleartextStorageDatabaseDefaultBarrier extends CleartextStorageDat
 /**
  * An additional taint step for cleartext database storage vulnerabilities.
  */
-private class CleartextStorageDatabaseArrayAdditionalFlowStep extends CleartextStorageDatabaseAdditionalFlowStep
+private class CleartextStorageDatabaseFieldsAdditionalFlowStep extends CleartextStorageDatabaseAdditionalFlowStep
 {
   override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-    // needed until we have proper content flow through arrays.
-    exists(ArrayExpr arr |
-      nodeFrom.asExpr() = arr.getAnElement() and
-      nodeTo.asExpr() = arr
-    )
-    or
     // if an object is sensitive, its fields are always sensitive
     // (this is needed because the sensitive data sources are in a sense
     //  approximate; for example we might identify `passwordBox` as a source,

diff --git a/swift/ql/lib/codeql/swift/security/CleartextStorageDatabaseQuery.qll b/swift/ql/lib/codeql/swift/security/CleartextStorageDatabaseQuery.qll
@@ -39,6 +39,11 @@ module CleartextStorageDatabaseConfig implements DataFlow::ConfigSig {
       cx.asNominalTypeDecl() = d and
       c.getAReadContent().(DataFlow::Content::FieldContent).getField() = cx.getAMember()
     )
+    or
+    // flow out from array elements of at the sink,
+    // for example in `database.allStatements(sql: "", arguments: [sensitive])`.
+    isSink(node) and
+    c.getAReadContent() instanceof DataFlow::Content::CollectionContent
   }
 }
 

diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionExtensions.qll
@@ -29,19 +29,6 @@ class CommandInjectionAdditionalFlowStep extends Unit {
   abstract predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo);
 }
 
-/**
- * An additional taint step for command injection vulnerabilities.
- */
-private class CommandInjectionArrayAdditionalFlowStep extends CommandInjectionAdditionalFlowStep {
-  override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-    // needed until we have proper content flow through arrays.
-    exists(ArrayExpr arr |
-      nodeFrom.asExpr() = arr.getAnElement() and
-      nodeTo.asExpr() = arr
-    )
-  }
-}
-
 /**
  * A sink defined in a CSV model.
  */

diff --git a/swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll b/swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll
@@ -23,6 +23,12 @@ module CommandInjectionConfig implements DataFlow::ConfigSig {
   predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
     any(CommandInjectionAdditionalFlowStep s).step(nodeFrom, nodeTo)
   }
+
+  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
+    // flow out from array elements of at the sink, for example in `task.arguments = [tainted]`.
+    isSink(node) and
+    c.getAReadContent() instanceof DataFlow::Content::CollectionContent
+  }
 }
 
 /**