Merge pull request #11670 from atorralba/atorralba/swift/predicate-injection

Swift: Add predicate injection query

swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll

@@ -90,6 +90,7 @@ private module Frameworks {
   private import codeql.swift.frameworks.StandardLibrary.WebView
   private import codeql.swift.frameworks.Alamofire.Alamofire
   private import codeql.swift.security.PathInjection
+  private import codeql.swift.security.PredicateInjection
 }
 
 /**

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

@@ -0,0 +1,41 @@
+/** Provides classes and predicates to reason about predicate injection vulnerabilities. */
+
+import swift
+private import codeql.swift.dataflow.DataFlow
+private import codeql.swift.dataflow.ExternalFlow
+
+/** A data flow sink for predicate injection vulnerabilities. */
+abstract class PredicateInjectionSink extends DataFlow::Node { }
+
+/** A sanitizer for predicate injection vulnerabilities. */
+abstract class PredicateInjectionSanitizer extends DataFlow::Node { }
+
+/**
+ * A unit class for adding additional taint steps.
+ *
+ * Extend this class to add additional taint steps that should apply to paths related to
+ * predicate injection vulnerabilities.
+ */
+class PredicateInjectionAdditionalTaintStep extends Unit {
+  /**
+   * Holds if the step from `node1` to `node2` should be considered a taint
+   * step for paths related to predicate injection vulnerabilities.
+   */
+  abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
+}
+
+private class DefaultPredicateInjectionSink extends PredicateInjectionSink {
+  DefaultPredicateInjectionSink() { sinkNode(this, "predicate-injection") }
+}
+
+private class PredicateInjectionSinkCsv extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        ";NSPredicate;true;init(format:argumentArray:);;;Argument[0];predicate-injection",
+        ";NSPredicate;true;init(format:arguments:);;;Argument[0];predicate-injection",
+        ";NSPredicate;true;init(format:_:);;;Argument[0];predicate-injection",
+        ";NSPredicate;true;init(fromMetadataQueryString:);;;Argument[0];predicate-injection"
+      ]
+  }
+}

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

@@ -0,0 +1,29 @@
+/**
+ * Provides a taint-tracking configuration for reasoning about predicate injection
+ * vulnerabilities.
+ */
+
+import swift
+private import codeql.swift.dataflow.DataFlow
+private import codeql.swift.dataflow.FlowSources
+private import codeql.swift.dataflow.TaintTracking
+private import codeql.swift.security.PredicateInjection
+
+/**
+ * A taint-tracking configuration for predicate injection vulnerabilities.
+ */
+class PredicateInjectionConf extends TaintTracking::Configuration {
+  PredicateInjectionConf() { this = "PredicateInjectionConf" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof PredicateInjectionSink }
+
+  override predicate isSanitizer(DataFlow::Node sanitizer) {
+    sanitizer instanceof PredicateInjectionSanitizer
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
+    any(PredicateInjectionAdditionalTaintStep s).step(n1, n2)
+  }
+}

swift/ql/src/queries/Security/CWE-943/PredicateInjection.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+<overview>
+<p>
+Predicates represent logical conditions that can be used to check whether an object matches them.
+If a predicate is built from user-provided data without sufficient sanitization, an attacker may
+be able to change the overall meaning of the predicate.
+</p>
+</overview>
+<recommendation>
+<p>
+When building a predicate from untrusted data, you should either pass it to the appropriate <code>arguments</code> parameter during initialization, or as an array of substitution variables before evaluation. You should not append or concatenate it to the body of the predicate.
+</p>
+</recommendation>
+<example>
+<p>
+In the following insecure example, <code>NSPredicate</code> is built directly from data obtained from an HTTP request. This is untrusted, and can be arbitrarily set by an attacker to alter the meaning of the predicate:
+</p>
+<sample src="PredicateInjectionBad.swift" />
+<p>
+A better way to do this is to use the <code>arguments</code> parameter of <code>NSPredicate</code>'s constructor. This prevents attackers from altering the meaning of the predicate, even if they control the externally obtained data, as seen in the following secure example:
+</p>
+<sample src="PredicateInjectionGood.swift" />
+</example>
+<references>
+<li>Apple Developer Documentation: <a href="https://developer.apple.com/documentation/foundation/nspredicate">NSPredicate</a> </li>
+</references>
+</qhelp>

swift/ql/src/queries/Security/CWE-943/PredicateInjection.ql

@@ -0,0 +1,22 @@
+/**
+ * @name Predicate built from user-controlled sources
+ * @description Building an NSPredicate from user-controlled sources may lead to attackers
+ *              changing the predicate's intended logic.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 8.8
+ * @precision high
+ * @id swift/predicate-injection
+ * @tags security
+ *       external/cwe/cwe-943
+ */
+
+import swift
+import codeql.swift.dataflow.DataFlow
+import codeql.swift.security.PredicateInjectionQuery
+import DataFlow::PathGraph
+
+from DataFlow::PathNode source, DataFlow::PathNode sink
+where any(PredicateInjectionConf c).hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "This predicate depends on a $@.", source.getNode(),
+  "user-provided value"

swift/ql/src/queries/Security/CWE-943/PredicateInjectionBad.swift

@@ -0,0 +1,9 @@
+let remoteString = try String(contentsOf: URL(string: "https://example.com/")!)
+
+let filenames: [String] = ["img1.png", "img2.png", "img3.png", "img.txt", "img.csv"]
+
+let predicate = NSPredicate(format: "SELF LIKE \(remoteString)")
+let filtered = filenames.filter(){ filename in
+    predicate.evaluate(with: filename)
+}
+print(filtered)

swift/ql/src/queries/Security/CWE-943/PredicateInjectionGood.swift

@@ -0,0 +1,9 @@
+let remoteString = try String(contentsOf: URL(string: "https://example.com/")!)
+
+let filenames: [String] = ["img1.png", "img2.png", "img3.png", "img.txt", "img.csv"]
+
+let predicate = NSPredicate(format: "SELF LIKE %@", remoteString)
+let filtered = filenames.filter(){ filename in
+    predicate.evaluate(with: filename)
+}
+print(filtered)

swift/ql/test/query-tests/Security/CWE-946/PredicateInjectionTest.ql

@@ -0,0 +1,23 @@
+import swift
+import codeql.swift.dataflow.DataFlow
+import codeql.swift.security.PredicateInjectionQuery
+import TestUtilities.InlineExpectationsTest
+
+class PredicateInjectionTest extends InlineExpectationsTest {
+  PredicateInjectionTest() { this = "PredicateInjectionTest" }
+
+  override string getARelevantTag() { result = "hasPredicateInjection" }
+
+  override predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(
+      PredicateInjectionConf config, DataFlow::Node source, DataFlow::Node sink, Expr sinkExpr
+    |
+      config.hasFlow(source, sink) and
+      sinkExpr = sink.asExpr() and
+      location = sinkExpr.getLocation() and
+      element = sinkExpr.toString() and
+      tag = "hasPredicateInjection" and
+      value = source.asExpr().getLocation().getStartLine().toString()
+    )
+  }
+}

swift/ql/test/query-tests/Security/CWE-946/predicateInjection.swift

@@ -0,0 +1,38 @@
+// --- stubs ---
+struct URL {
+	init?(string: String) {}
+}
+
+extension String {
+	init(contentsOf: URL) {
+        let data = ""
+        self.init(data)
+    }
+}
+
+class NSPredicate {
+    init(format: String, argumentArray: [Any]?) {}
+    init(format: String, arguments: CVaListPointer) {}
+    init(format: String, _: CVarArg...) {}
+    init?(fromMetadataQueryString: String) {}
+}
+
+// --- tests ---
+
+func test() {
+    let remoteString = String(contentsOf: URL(string: "http://example.com/")!)
+    let safeString = "safe"
+
+    NSPredicate(format: remoteString, argumentArray: []) // $ hasPredicateInjection=23
+    NSPredicate(format: safeString, argumentArray: []) // Safe
+    NSPredicate(format: safeString, argumentArray: [remoteString]) // Safe
+    NSPredicate(format: remoteString, arguments: CVaListPointer(_fromUnsafeMutablePointer: UnsafeMutablePointer(bitPattern: 0)!)) // $ hasPredicateInjection=23
+    NSPredicate(format: safeString, arguments: CVaListPointer(_fromUnsafeMutablePointer: UnsafeMutablePointer(bitPattern: 0)!)) // Safe
+    NSPredicate(format: remoteString) // $ hasPredicateInjection=23
+    NSPredicate(format: safeString) // Safe
+    NSPredicate(format: remoteString, "" as! CVarArg) // $ hasPredicateInjection=23
+    NSPredicate(format: safeString, "" as! CVarArg) // Safe
+    NSPredicate(format: safeString, remoteString as! CVarArg) // Safe
+    NSPredicate(fromMetadataQueryString: remoteString) // $ hasPredicateInjection=23
+    NSPredicate(fromMetadataQueryString: safeString) // Safe
+}