Merge pull request github#14521 from geoffw0/defaultstep

Swift: Add CollectionContent to defaultImplicitTaintRead

Author: MathiasVP

swift/ql/lib/change-notes/2023-10-16-collection-content.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+
+* Collection content is now automatically read at taint flow sinks. This removes the need to define an `allowImplicitRead` predicate on data flow configurations where the sink might be an array, set or similar type with tainted contents. Where that step had not been defined, taint may find additional results now.

swift/ql/lib/codeql/swift/dataflow/internal/TaintTrackingPublic.qll

@@ -37,4 +37,9 @@ predicate defaultImplicitTaintRead(DataFlow::Node node, DataFlow::ContentSet cs)
     cx.asNominalTypeDecl() = d and
     cs.getAReadContent().(DataFlow::Content::FieldContent).getField() = cx.getAMember()
   )
+  or
+  // We often expect taint to reach a sink inside `CollectionContent`, for example an array element
+  // or pointer contents. It is convenient to have a default implicit read step for these cases rather
+  // than implementing this step in a lot of separate `allowImplicitRead`s.
+  cs.getAReadContent() instanceof DataFlow::Content::CollectionContent
 }

swift/ql/lib/codeql/swift/security/CleartextLoggingQuery.qll

@@ -25,12 +25,6 @@ module CleartextLoggingConfig implements DataFlow::ConfigSig {
   predicate isAdditionalFlowStep(DataFlow::Node n1, DataFlow::Node n2) {
     any(CleartextLoggingAdditionalFlowStep s).step(n1, n2)
   }
-
-  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
-    // flow out from collection content at the sink.
-    isSink(node) and
-    c.getAReadContent() instanceof DataFlow::Content::CollectionContent
-  }
 }
 
 /**

swift/ql/lib/codeql/swift/security/CleartextStorageDatabaseQuery.qll

@@ -45,11 +45,6 @@ module CleartextStorageDatabaseConfig implements DataFlow::ConfigSig {
     isSink(node) and
     node.asExpr().getType().getUnderlyingType() instanceof DictionaryType and
     c.getAReadContent().(DataFlow::Content::TupleContent).getIndex() = 1
-    or
-    // flow out from array elements (and other collection content) at the sink,
-    // for example in `database.allStatements(sql: "", arguments: [sensitive])`.
-    isSink(node) and
-    c.getAReadContent() instanceof DataFlow::Content::CollectionContent
   }
 }
 

swift/ql/lib/codeql/swift/security/CommandInjectionQuery.qll

@@ -23,12 +23,6 @@ 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
-  }
 }
 
 /**

swift/ql/lib/codeql/swift/security/ConstantPasswordQuery.qll

@@ -30,6 +30,11 @@ module ConstantPasswordConfig implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) { node instanceof ConstantPasswordBarrier }
 
+  predicate isBarrierIn(DataFlow::Node node) {
+    // make sources barriers so that we only report the closest instance
+    isSource(node)
+  }
+
   predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
     any(ConstantPasswordAdditionalFlowStep s).step(nodeFrom, nodeTo)
   }

swift/ql/lib/codeql/swift/security/ConstantSaltQuery.qll

@@ -31,6 +31,11 @@ module ConstantSaltConfig implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) { node instanceof ConstantSaltBarrier }
 
+  predicate isBarrierIn(DataFlow::Node node) {
+    // make sources barriers so that we only report the closest instance
+    isSource(node)
+  }
+
   predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
     any(ConstantSaltAdditionalFlowStep s).step(nodeFrom, nodeTo)
   }

swift/ql/lib/codeql/swift/security/HardcodedEncryptionKeyQuery.qll

@@ -38,14 +38,13 @@ module HardcodedKeyConfig implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) { node instanceof HardcodedEncryptionKeyBarrier }
 
-  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-    any(HardcodedEncryptionKeyAdditionalFlowStep s).step(nodeFrom, nodeTo)
+  predicate isBarrierIn(DataFlow::Node node) {
+    // make sources barriers so that we only report the closest instance
+    isSource(node)
   }
 
-  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
-    // flow out of collections at the sink
-    isSink(node) and
-    c.getAReadContent() instanceof DataFlow::Content::CollectionContent
+  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    any(HardcodedEncryptionKeyAdditionalFlowStep s).step(nodeFrom, nodeTo)
   }
 }
 

swift/ql/lib/codeql/swift/security/StaticInitializationVectorQuery.qll

@@ -32,6 +32,11 @@ module StaticInitializationVectorConfig implements DataFlow::ConfigSig {
 
   predicate isBarrier(DataFlow::Node node) { node instanceof StaticInitializationVectorBarrier }
 
+  predicate isBarrierIn(DataFlow::Node node) {
+    // make sources barriers so that we only report the closest instance
+    isSource(node)
+  }
+
   predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
     any(StaticInitializationVectorAdditionalFlowStep s).step(nodeFrom, nodeTo)
   }

swift/ql/lib/codeql/swift/security/UnsafeJsEvalQuery.qll

@@ -22,16 +22,6 @@ module UnsafeJsEvalConfig implements DataFlow::ConfigSig {
   predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
     any(UnsafeJsEvalAdditionalFlowStep s).step(nodeFrom, nodeTo)
   }
-
-  predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
-    // flow out from content a the sink
-    (
-      isSink(node)
-      or
-      isAdditionalFlowStep(node, _)
-    ) and
-    c.getAReadContent() instanceof DataFlow::Content::CollectionContent
-  }
 }
 
 /**