Merge pull request #10000 from geoffw0/defaulttaint

Swift: Taint flow improvements

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

@@ -108,12 +108,20 @@ private module Cached {
       localFlowSsaInput(nodeFrom, def, nodeTo.asDefinition())
     )
     or
+    // flow through writes to inout parameters
     exists(ParamReturnKind kind, ExprCfgNode arg |
       arg = nodeFrom.(InOutUpdateNode).getCall(kind).asCall().getArgument(kind.getIndex()) and
       nodeTo.asDefinition().(Ssa::WriteDefinition).isInoutDef(arg)
     )
     or
+    // flow through `&` (inout argument)
     nodeFrom.asExpr() = nodeTo.asExpr().(InOutExpr).getSubExpr()
+    or
+    // flow through `try!` and similar constructs
+    nodeFrom.asExpr() = nodeTo.asExpr().(AnyTryExpr).getSubExpr()
+    or
+    // flow through `!`
+    nodeFrom.asExpr() = nodeTo.asExpr().(ForceValueExpr).getSubExpr()
   }
 
   /**

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

@@ -41,6 +41,23 @@ private module Cached {
       nodeTo.asExpr() = interpolated and
       nodeFrom.asExpr() = interpolated.getAppendingExpr()
     )
+    or
+    // allow flow through string concatenation.
+    exists(AddExpr ae |
+      ae.getAnOperand() = nodeFrom.asExpr() and
+      ae = nodeTo.asExpr() and
+      ae.getType().getName() = "String"
+    )
+    or
+    // allow flow through `URL.init`.
+    exists(CallExpr call, ClassDecl c, AbstractFunctionDecl f |
+      c.getName() = "URL" and
+      c.getAMember() = f and
+      f.getName() = ["init(string:)", "init(string:relativeTo:)"] and
+      call.getFunction().(ApplyExpr).getStaticTarget() = f and
+      nodeFrom.asExpr() = call.getAnArgument().getExpr() and
+      nodeTo.asExpr() = call
+    )
   }
 
   /**

swift/ql/src/queries/Security/CWE-079/UnsafeWebViewFetch.ql

@@ -81,30 +81,6 @@ class UnsafeWebViewFetchConfig extends TaintTracking::Configuration {
     node instanceof Sink or
     node.asExpr() = any(Sink s).getBaseUrl()
   }
-
-  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    // allow flow through `try!` and similar constructs
-    // TODO: this should probably be part of DataFlow / TaintTracking.
-    node1.asExpr() = node2.asExpr().(AnyTryExpr).getSubExpr()
-    or
-    // allow flow through `!`
-    // TODO: this should probably be part of DataFlow / TaintTracking.
-    node1.asExpr() = node2.asExpr().(ForceValueExpr).getSubExpr()
-    or
-    // allow flow through string concatenation.
-    // TODO: this should probably be part of TaintTracking.
-    node2.asExpr().(AddExpr).getAnOperand() = node1.asExpr()
-    or
-    // allow flow through `URL.init`.
-    exists(CallExpr call, ClassDecl c, AbstractFunctionDecl f |
-      c.getName() = "URL" and
-      c.getAMember() = f and
-      f.getName() = ["init(string:)", "init(string:relativeTo:)"] and
-      call.getFunction().(ApplyExpr).getStaticTarget() = f and
-      node1.asExpr() = call.getAnArgument().getExpr() and
-      node2.asExpr() = call
-    )
-  }
 }
 
 from

swift/ql/test/library-tests/dataflow/taint/Taint.expected

@@ -1,20 +1,61 @@
 edges
-| test.swift:5:11:5:18 | call to source() :  | test.swift:7:13:7:13 | "..." |
-| test.swift:5:11:5:18 | call to source() :  | test.swift:9:13:9:13 | "..." |
-| test.swift:5:11:5:18 | call to source() :  | test.swift:11:13:11:13 | "..." |
-| test.swift:5:11:5:18 | call to source() :  | test.swift:16:13:16:13 | "..." |
-| test.swift:5:11:5:18 | call to source() :  | test.swift:18:13:18:13 | "..." |
+| string.swift:5:11:5:18 | call to source() :  | string.swift:7:13:7:13 | "..." |
+| string.swift:5:11:5:18 | call to source() :  | string.swift:9:13:9:13 | "..." |
+| string.swift:5:11:5:18 | call to source() :  | string.swift:11:13:11:13 | "..." |
+| string.swift:5:11:5:18 | call to source() :  | string.swift:16:13:16:13 | "..." |
+| string.swift:5:11:5:18 | call to source() :  | string.swift:18:13:18:13 | "..." |
+| string.swift:28:17:28:25 | call to source2() :  | string.swift:31:13:31:13 | tainted |
+| string.swift:28:17:28:25 | call to source2() :  | string.swift:34:13:34:21 | ... call to +(_:_:) ... |
+| string.swift:28:17:28:25 | call to source2() :  | string.swift:35:13:35:23 | ... call to +(_:_:) ... |
+| string.swift:28:17:28:25 | call to source2() :  | string.swift:36:13:36:23 | ... call to +(_:_:) ... |
+| string.swift:28:17:28:25 | call to source2() :  | string.swift:39:13:39:29 | ... call to +(_:_:) ... |
+| try.swift:9:17:9:24 | call to source() :  | try.swift:9:13:9:24 | try ... |
+| try.swift:15:17:15:24 | call to source() :  | try.swift:15:12:15:24 | try! ... |
+| try.swift:18:18:18:25 | call to source() :  | try.swift:18:12:18:27 | ...! |
+| url.swift:13:16:13:23 | call to source() :  | url.swift:18:12:18:12 | urlTainted |
+| url.swift:13:16:13:23 | call to source() :  | url.swift:21:12:21:49 | ...! |
+| url.swift:13:16:13:23 | call to source() :  | url.swift:23:12:23:54 | ...! |
+| url.swift:13:16:13:23 | call to source() :  | url.swift:39:12:39:12 | ...! |
 nodes
-| test.swift:5:11:5:18 | call to source() :  | semmle.label | call to source() :  |
-| test.swift:7:13:7:13 | "..." | semmle.label | "..." |
-| test.swift:9:13:9:13 | "..." | semmle.label | "..." |
-| test.swift:11:13:11:13 | "..." | semmle.label | "..." |
-| test.swift:16:13:16:13 | "..." | semmle.label | "..." |
-| test.swift:18:13:18:13 | "..." | semmle.label | "..." |
+| string.swift:5:11:5:18 | call to source() :  | semmle.label | call to source() :  |
+| string.swift:7:13:7:13 | "..." | semmle.label | "..." |
+| string.swift:9:13:9:13 | "..." | semmle.label | "..." |
+| string.swift:11:13:11:13 | "..." | semmle.label | "..." |
+| string.swift:16:13:16:13 | "..." | semmle.label | "..." |
+| string.swift:18:13:18:13 | "..." | semmle.label | "..." |
+| string.swift:28:17:28:25 | call to source2() :  | semmle.label | call to source2() :  |
+| string.swift:31:13:31:13 | tainted | semmle.label | tainted |
+| string.swift:34:13:34:21 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
+| string.swift:35:13:35:23 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
+| string.swift:36:13:36:23 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
+| string.swift:39:13:39:29 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
+| try.swift:9:13:9:24 | try ... | semmle.label | try ... |
+| try.swift:9:17:9:24 | call to source() :  | semmle.label | call to source() :  |
+| try.swift:15:12:15:24 | try! ... | semmle.label | try! ... |
+| try.swift:15:17:15:24 | call to source() :  | semmle.label | call to source() :  |
+| try.swift:18:12:18:27 | ...! | semmle.label | ...! |
+| try.swift:18:18:18:25 | call to source() :  | semmle.label | call to source() :  |
+| url.swift:13:16:13:23 | call to source() :  | semmle.label | call to source() :  |
+| url.swift:18:12:18:12 | urlTainted | semmle.label | urlTainted |
+| url.swift:21:12:21:49 | ...! | semmle.label | ...! |
+| url.swift:23:12:23:54 | ...! | semmle.label | ...! |
+| url.swift:39:12:39:12 | ...! | semmle.label | ...! |
 subpaths
 #select
-| test.swift:7:13:7:13 | "..." | test.swift:5:11:5:18 | call to source() :  | test.swift:7:13:7:13 | "..." | result |
-| test.swift:9:13:9:13 | "..." | test.swift:5:11:5:18 | call to source() :  | test.swift:9:13:9:13 | "..." | result |
-| test.swift:11:13:11:13 | "..." | test.swift:5:11:5:18 | call to source() :  | test.swift:11:13:11:13 | "..." | result |
-| test.swift:16:13:16:13 | "..." | test.swift:5:11:5:18 | call to source() :  | test.swift:16:13:16:13 | "..." | result |
-| test.swift:18:13:18:13 | "..." | test.swift:5:11:5:18 | call to source() :  | test.swift:18:13:18:13 | "..." | result |
+| string.swift:7:13:7:13 | "..." | string.swift:5:11:5:18 | call to source() :  | string.swift:7:13:7:13 | "..." | result |
+| string.swift:9:13:9:13 | "..." | string.swift:5:11:5:18 | call to source() :  | string.swift:9:13:9:13 | "..." | result |
+| string.swift:11:13:11:13 | "..." | string.swift:5:11:5:18 | call to source() :  | string.swift:11:13:11:13 | "..." | result |
+| string.swift:16:13:16:13 | "..." | string.swift:5:11:5:18 | call to source() :  | string.swift:16:13:16:13 | "..." | result |
+| string.swift:18:13:18:13 | "..." | string.swift:5:11:5:18 | call to source() :  | string.swift:18:13:18:13 | "..." | result |
+| string.swift:31:13:31:13 | tainted | string.swift:28:17:28:25 | call to source2() :  | string.swift:31:13:31:13 | tainted | result |
+| string.swift:34:13:34:21 | ... call to +(_:_:) ... | string.swift:28:17:28:25 | call to source2() :  | string.swift:34:13:34:21 | ... call to +(_:_:) ... | result |
+| string.swift:35:13:35:23 | ... call to +(_:_:) ... | string.swift:28:17:28:25 | call to source2() :  | string.swift:35:13:35:23 | ... call to +(_:_:) ... | result |
+| string.swift:36:13:36:23 | ... call to +(_:_:) ... | string.swift:28:17:28:25 | call to source2() :  | string.swift:36:13:36:23 | ... call to +(_:_:) ... | result |
+| string.swift:39:13:39:29 | ... call to +(_:_:) ... | string.swift:28:17:28:25 | call to source2() :  | string.swift:39:13:39:29 | ... call to +(_:_:) ... | result |
+| try.swift:9:13:9:24 | try ... | try.swift:9:17:9:24 | call to source() :  | try.swift:9:13:9:24 | try ... | result |
+| try.swift:15:12:15:24 | try! ... | try.swift:15:17:15:24 | call to source() :  | try.swift:15:12:15:24 | try! ... | result |
+| try.swift:18:12:18:27 | ...! | try.swift:18:18:18:25 | call to source() :  | try.swift:18:12:18:27 | ...! | result |
+| url.swift:18:12:18:12 | urlTainted | url.swift:13:16:13:23 | call to source() :  | url.swift:18:12:18:12 | urlTainted | result |
+| url.swift:21:12:21:49 | ...! | url.swift:13:16:13:23 | call to source() :  | url.swift:21:12:21:49 | ...! | result |
+| url.swift:23:12:23:54 | ...! | url.swift:13:16:13:23 | call to source() :  | url.swift:23:12:23:54 | ...! | result |
+| url.swift:39:12:39:12 | ...! | url.swift:13:16:13:23 | call to source() :  | url.swift:39:12:39:12 | ...! | result |

swift/ql/test/library-tests/dataflow/taint/Taint.ql

@@ -11,12 +11,12 @@ class TestConfiguration extends TaintTracking::Configuration {
   TestConfiguration() { this = "TestConfiguration" }
 
   override predicate isSource(Node src) {
-    src.asExpr().(CallExpr).getStaticTarget().getName() = "source()"
+    src.asExpr().(CallExpr).getStaticTarget().getName().matches("source%")
   }
 
   override predicate isSink(Node sink) {
     exists(CallExpr sinkCall |
-      sinkCall.getStaticTarget().getName() = "sink(arg:)" and
+      sinkCall.getStaticTarget().getName().matches("sink%") and
       sinkCall.getAnArgument().getExpr() = sink.asExpr()
     )
   }

swift/ql/test/library-tests/dataflow/taint/data.swift

@@ -0,0 +1,25 @@
+
+class Data
+{
+    init<S>(_ elements: S) {}
+}
+
+func source() -> String { return "" }
+func sink(arg: Data) {}
+func sink2(arg: String) {}
+
+func taintThroughData() {
+	let dataClean = Data("123456".utf8)
+	let dataTainted = Data(source().utf8)
+	let dataTainted2 = Data(dataTainted)
+
+	sink(arg: dataClean)
+	sink(arg: dataTainted) // tainted [NOT DETECTED]
+	sink(arg: dataTainted2) // tainted [NOT DETECTED]
+
+	let stringClean = String(data: dataClean, encoding: String.Encoding.utf8)
+	let stringTainted = String(data: dataTainted, encoding: String.Encoding.utf8)
+
+	sink2(arg: stringClean!) // tainted [NOT DETECTED]
+	sink2(arg: stringTainted!) // tainted [NOT DETECTED]
+}

swift/ql/test/library-tests/dataflow/taint/string.swift

@@ -0,0 +1,89 @@
+func source() -> Int { return 0; }
+func sink(arg: String) {}
+
+func taintThroughInterpolatedStrings() {
+  var x = source()
+
+  sink(arg: "\(x)") // tainted
+
+  sink(arg: "\(x) \(x)") // tainted
+
+  sink(arg: "\(x) \(0) \(x)") // tainted
+
+  var y = 42
+  sink(arg: "\(y)") // clean
+
+  sink(arg: "\(x) hello \(y)") // tainted
+
+  sink(arg: "\(y) world \(x)") // tainted
+
+  x = 0
+  sink(arg: "\(x)") // clean
+}
+
+func source2() -> String { return ""; }
+
+func taintThroughStringConcatenation() {
+  var clean = "abcdef"
+  var tainted = source2()
+
+  sink(arg: clean)
+  sink(arg: tainted) // tainted
+
+  sink(arg: clean + clean)
+  sink(arg: clean + tainted) // tainted
+  sink(arg: tainted + clean) // tainted
+  sink(arg: tainted + tainted) // tainted
+
+  sink(arg: ">" + clean + "<")
+  sink(arg: ">" + tainted + "<") // tainted
+
+  var str = "abc"
+
+  sink(arg: str)
+
+  str += "def"
+  sink(arg: str)
+
+  str += source2()
+  sink(arg: str) // tainted [NOT DETECTED]
+
+  var str2 = "abc"
+
+  sink(arg: str2)
+
+  str2.append("def")
+  sink(arg: str2)
+
+  str2.append(source2())
+  sink(arg: str2) // tainted [NOT DETECTED]
+
+  var str3 = "abc"
+
+  sink(arg: str3)
+
+  str3.append(contentsOf: "def")
+  sink(arg: str3)
+
+  str3.append(contentsOf: source2())
+  sink(arg: str2) // tainted [NOT DETECTED]
+}
+
+func taintThroughStringOperations() {
+  var clean = ""
+  var tainted = source2()
+  var taintedInt = source()
+
+  sink(arg: String(clean))
+  sink(arg: String(tainted)) // tainted [NOT DETECTED]
+  sink(arg: String(taintedInt)) // tainted [NOT DETECTED]
+
+  sink(arg: String(repeating: clean, count: 2))
+  sink(arg: String(repeating: tainted, count: 2)) // tainted [NOT DETECTED]
+
+  sink(arg: clean.description)
+  sink(arg: tainted.description) // tainted [NOT DETECTED]
+
+  sink(arg: clean.debugDescription)
+  sink(arg: tainted.debugDescription) // tainted [NOT DETECTED]
+}

swift/ql/test/library-tests/dataflow/taint/try.swift

@@ -0,0 +1,19 @@
+func clean() throws -> String { return ""; }
+func source() throws -> String { return ""; }
+func sink(arg: String) {}
+
+func taintThroughTry() {
+	do
+	{
+		sink(arg: try clean())
+		sink(arg: try source()) // tainted
+	} catch {
+		// ...
+	}
+
+	sink(arg: try! clean())
+	sink(arg: try! source()) // tainted
+
+	sink(arg: (try? clean())!)
+	sink(arg: (try? source())!) // tainted
+}

swift/ql/test/library-tests/dataflow/taint/url.swift

@@ -0,0 +1,40 @@
+
+class URL
+{
+	init?(string: String) {}
+	init?(string: String, relativeTo: URL?) {}
+}
+
+func source() -> String { return "" }
+func sink(arg: URL) {}
+
+func taintThroughURL() {
+	let clean = "http://example.com/"
+	let tainted = source()
+	let urlClean = URL(string: clean)!
+	let urlTainted = URL(string: tainted)!
+
+	sink(arg: urlClean)
+	sink(arg: urlTainted) // tainted
+
+	sink(arg: URL(string: clean, relativeTo: nil)!)
+	sink(arg: URL(string: tainted, relativeTo: nil)!) // tainted
+	sink(arg: URL(string: clean, relativeTo: urlClean)!)
+	sink(arg: URL(string: clean, relativeTo: urlTainted)!) // tainted
+
+	if let x = URL(string: clean) {
+		sink(arg: x)
+	}
+
+	if let y = URL(string: tainted) {
+		sink(arg: y) // tainted [NOT DETECTED]
+	}
+
+	var urlClean2 : URL!
+	urlClean2 = URL(string: clean)
+	sink(arg: urlClean2)
+
+	var urlTainted2 : URL!
+	urlTainted2 = URL(string: tainted)
+	sink(arg: urlTainted2) // tainted
+}