Skip to content

Commit 56fddd7

Browse files
MathiasVPMathiasVP
authored
Merge pull request #10000 from geoffw0/defaulttaint
Swift: Taint flow improvements
2 parents cce39fb + 6ffe5fc commit 56fddd7

File tree

11 files changed

+479
-185
lines changed

11 files changed

+479
-185
lines changed

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,20 @@ private module Cached {
108108
localFlowSsaInput(nodeFrom, def, nodeTo.asDefinition())
109109
)
110110
or
111+
// flow through writes to inout parameters
111112
exists(ParamReturnKind kind, ExprCfgNode arg |
112113
arg = nodeFrom.(InOutUpdateNode).getCall(kind).asCall().getArgument(kind.getIndex()) and
113114
nodeTo.asDefinition().(Ssa::WriteDefinition).isInoutDef(arg)
114115
)
115116
or
117+
// flow through `&` (inout argument)
116118
nodeFrom.asExpr() = nodeTo.asExpr().(InOutExpr).getSubExpr()
119+
or
120+
// flow through `try!` and similar constructs
121+
nodeFrom.asExpr() = nodeTo.asExpr().(AnyTryExpr).getSubExpr()
122+
or
123+
// flow through `!`
124+
nodeFrom.asExpr() = nodeTo.asExpr().(ForceValueExpr).getSubExpr()
117125
}
118126

119127
/**

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,23 @@ private module Cached {
4141
nodeTo.asExpr() = interpolated and
4242
nodeFrom.asExpr() = interpolated.getAppendingExpr()
4343
)
44+
or
45+
// allow flow through string concatenation.
46+
exists(AddExpr ae |
47+
ae.getAnOperand() = nodeFrom.asExpr() and
48+
ae = nodeTo.asExpr() and
49+
ae.getType().getName() = "String"
50+
)
51+
or
52+
// allow flow through `URL.init`.
53+
exists(CallExpr call, ClassDecl c, AbstractFunctionDecl f |
54+
c.getName() = "URL" and
55+
c.getAMember() = f and
56+
f.getName() = ["init(string:)", "init(string:relativeTo:)"] and
57+
call.getFunction().(ApplyExpr).getStaticTarget() = f and
58+
nodeFrom.asExpr() = call.getAnArgument().getExpr() and
59+
nodeTo.asExpr() = call
60+
)
4461
}
4562

4663
/**

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

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -81,30 +81,6 @@ class UnsafeWebViewFetchConfig extends TaintTracking::Configuration {
8181
node instanceof Sink or
8282
node.asExpr() = any(Sink s).getBaseUrl()
8383
}
84-
85-
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
86-
// allow flow through `try!` and similar constructs
87-
// TODO: this should probably be part of DataFlow / TaintTracking.
88-
node1.asExpr() = node2.asExpr().(AnyTryExpr).getSubExpr()
89-
or
90-
// allow flow through `!`
91-
// TODO: this should probably be part of DataFlow / TaintTracking.
92-
node1.asExpr() = node2.asExpr().(ForceValueExpr).getSubExpr()
93-
or
94-
// allow flow through string concatenation.
95-
// TODO: this should probably be part of TaintTracking.
96-
node2.asExpr().(AddExpr).getAnOperand() = node1.asExpr()
97-
or
98-
// allow flow through `URL.init`.
99-
exists(CallExpr call, ClassDecl c, AbstractFunctionDecl f |
100-
c.getName() = "URL" and
101-
c.getAMember() = f and
102-
f.getName() = ["init(string:)", "init(string:relativeTo:)"] and
103-
call.getFunction().(ApplyExpr).getStaticTarget() = f and
104-
node1.asExpr() = call.getAnArgument().getExpr() and
105-
node2.asExpr() = call
106-
)
107-
}
10884
}
10985

11086
from

‎swift/ql/test/library-tests/dataflow/taint/LocalTaint.expected

Lines changed: 222 additions & 121 deletions
Large diffs are not rendered by default.

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

Lines changed: 57 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,61 @@
11
edges
2-
| test.swift:5:11:5:18 | call to source() : | test.swift:7:13:7:13 | "..." |
3-
| test.swift:5:11:5:18 | call to source() : | test.swift:9:13:9:13 | "..." |
4-
| test.swift:5:11:5:18 | call to source() : | test.swift:11:13:11:13 | "..." |
5-
| test.swift:5:11:5:18 | call to source() : | test.swift:16:13:16:13 | "..." |
6-
| test.swift:5:11:5:18 | call to source() : | test.swift:18:13:18:13 | "..." |
2+
| string.swift:5:11:5:18 | call to source() : | string.swift:7:13:7:13 | "..." |
3+
| string.swift:5:11:5:18 | call to source() : | string.swift:9:13:9:13 | "..." |
4+
| string.swift:5:11:5:18 | call to source() : | string.swift:11:13:11:13 | "..." |
5+
| string.swift:5:11:5:18 | call to source() : | string.swift:16:13:16:13 | "..." |
6+
| string.swift:5:11:5:18 | call to source() : | string.swift:18:13:18:13 | "..." |
7+
| string.swift:28:17:28:25 | call to source2() : | string.swift:31:13:31:13 | tainted |
8+
| string.swift:28:17:28:25 | call to source2() : | string.swift:34:13:34:21 | ... call to +(_:_:) ... |
9+
| string.swift:28:17:28:25 | call to source2() : | string.swift:35:13:35:23 | ... call to +(_:_:) ... |
10+
| string.swift:28:17:28:25 | call to source2() : | string.swift:36:13:36:23 | ... call to +(_:_:) ... |
11+
| string.swift:28:17:28:25 | call to source2() : | string.swift:39:13:39:29 | ... call to +(_:_:) ... |
12+
| try.swift:9:17:9:24 | call to source() : | try.swift:9:13:9:24 | try ... |
13+
| try.swift:15:17:15:24 | call to source() : | try.swift:15:12:15:24 | try! ... |
14+
| try.swift:18:18:18:25 | call to source() : | try.swift:18:12:18:27 | ...! |
15+
| url.swift:13:16:13:23 | call to source() : | url.swift:18:12:18:12 | urlTainted |
16+
| url.swift:13:16:13:23 | call to source() : | url.swift:21:12:21:49 | ...! |
17+
| url.swift:13:16:13:23 | call to source() : | url.swift:23:12:23:54 | ...! |
18+
| url.swift:13:16:13:23 | call to source() : | url.swift:39:12:39:12 | ...! |
719
nodes
8-
| test.swift:5:11:5:18 | call to source() : | semmle.label | call to source() : |
9-
| test.swift:7:13:7:13 | "..." | semmle.label | "..." |
10-
| test.swift:9:13:9:13 | "..." | semmle.label | "..." |
11-
| test.swift:11:13:11:13 | "..." | semmle.label | "..." |
12-
| test.swift:16:13:16:13 | "..." | semmle.label | "..." |
13-
| test.swift:18:13:18:13 | "..." | semmle.label | "..." |
20+
| string.swift:5:11:5:18 | call to source() : | semmle.label | call to source() : |
21+
| string.swift:7:13:7:13 | "..." | semmle.label | "..." |
22+
| string.swift:9:13:9:13 | "..." | semmle.label | "..." |
23+
| string.swift:11:13:11:13 | "..." | semmle.label | "..." |
24+
| string.swift:16:13:16:13 | "..." | semmle.label | "..." |
25+
| string.swift:18:13:18:13 | "..." | semmle.label | "..." |
26+
| string.swift:28:17:28:25 | call to source2() : | semmle.label | call to source2() : |
27+
| string.swift:31:13:31:13 | tainted | semmle.label | tainted |
28+
| string.swift:34:13:34:21 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
29+
| string.swift:35:13:35:23 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
30+
| string.swift:36:13:36:23 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
31+
| string.swift:39:13:39:29 | ... call to +(_:_:) ... | semmle.label | ... call to +(_:_:) ... |
32+
| try.swift:9:13:9:24 | try ... | semmle.label | try ... |
33+
| try.swift:9:17:9:24 | call to source() : | semmle.label | call to source() : |
34+
| try.swift:15:12:15:24 | try! ... | semmle.label | try! ... |
35+
| try.swift:15:17:15:24 | call to source() : | semmle.label | call to source() : |
36+
| try.swift:18:12:18:27 | ...! | semmle.label | ...! |
37+
| try.swift:18:18:18:25 | call to source() : | semmle.label | call to source() : |
38+
| url.swift:13:16:13:23 | call to source() : | semmle.label | call to source() : |
39+
| url.swift:18:12:18:12 | urlTainted | semmle.label | urlTainted |
40+
| url.swift:21:12:21:49 | ...! | semmle.label | ...! |
41+
| url.swift:23:12:23:54 | ...! | semmle.label | ...! |
42+
| url.swift:39:12:39:12 | ...! | semmle.label | ...! |
1443
subpaths
1544
#select
16-
| test.swift:7:13:7:13 | "..." | test.swift:5:11:5:18 | call to source() : | test.swift:7:13:7:13 | "..." | result |
17-
| test.swift:9:13:9:13 | "..." | test.swift:5:11:5:18 | call to source() : | test.swift:9:13:9:13 | "..." | result |
18-
| test.swift:11:13:11:13 | "..." | test.swift:5:11:5:18 | call to source() : | test.swift:11:13:11:13 | "..." | result |
19-
| test.swift:16:13:16:13 | "..." | test.swift:5:11:5:18 | call to source() : | test.swift:16:13:16:13 | "..." | result |
20-
| test.swift:18:13:18:13 | "..." | test.swift:5:11:5:18 | call to source() : | test.swift:18:13:18:13 | "..." | result |
45+
| string.swift:7:13:7:13 | "..." | string.swift:5:11:5:18 | call to source() : | string.swift:7:13:7:13 | "..." | result |
46+
| string.swift:9:13:9:13 | "..." | string.swift:5:11:5:18 | call to source() : | string.swift:9:13:9:13 | "..." | result |
47+
| string.swift:11:13:11:13 | "..." | string.swift:5:11:5:18 | call to source() : | string.swift:11:13:11:13 | "..." | result |
48+
| string.swift:16:13:16:13 | "..." | string.swift:5:11:5:18 | call to source() : | string.swift:16:13:16:13 | "..." | result |
49+
| string.swift:18:13:18:13 | "..." | string.swift:5:11:5:18 | call to source() : | string.swift:18:13:18:13 | "..." | result |
50+
| string.swift:31:13:31:13 | tainted | string.swift:28:17:28:25 | call to source2() : | string.swift:31:13:31:13 | tainted | result |
51+
| 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 |
52+
| 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 |
53+
| 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 |
54+
| 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 |
55+
| try.swift:9:13:9:24 | try ... | try.swift:9:17:9:24 | call to source() : | try.swift:9:13:9:24 | try ... | result |
56+
| try.swift:15:12:15:24 | try! ... | try.swift:15:17:15:24 | call to source() : | try.swift:15:12:15:24 | try! ... | result |
57+
| try.swift:18:12:18:27 | ...! | try.swift:18:18:18:25 | call to source() : | try.swift:18:12:18:27 | ...! | result |
58+
| url.swift:18:12:18:12 | urlTainted | url.swift:13:16:13:23 | call to source() : | url.swift:18:12:18:12 | urlTainted | result |
59+
| url.swift:21:12:21:49 | ...! | url.swift:13:16:13:23 | call to source() : | url.swift:21:12:21:49 | ...! | result |
60+
| url.swift:23:12:23:54 | ...! | url.swift:13:16:13:23 | call to source() : | url.swift:23:12:23:54 | ...! | result |
61+
| 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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,12 @@ class TestConfiguration extends TaintTracking::Configuration {
1111
TestConfiguration() { this = "TestConfiguration" }
1212

1313
override predicate isSource(Node src) {
14-
src.asExpr().(CallExpr).getStaticTarget().getName() = "source()"
14+
src.asExpr().(CallExpr).getStaticTarget().getName().matches("source%")
1515
}
1616

1717
override predicate isSink(Node sink) {
1818
exists(CallExpr sinkCall |
19-
sinkCall.getStaticTarget().getName() = "sink(arg:)" and
19+
sinkCall.getStaticTarget().getName().matches("sink%") and
2020
sinkCall.getAnArgument().getExpr() = sink.asExpr()
2121
)
2222
}

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

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
2+
class Data
3+
{
4+
init<S>(_ elements: S) {}
5+
}
6+
7+
func source() -> String { return "" }
8+
func sink(arg: Data) {}
9+
func sink2(arg: String) {}
10+
11+
func taintThroughData() {
12+
let dataClean = Data("123456".utf8)
13+
let dataTainted = Data(source().utf8)
14+
let dataTainted2 = Data(dataTainted)
15+
16+
sink(arg: dataClean)
17+
sink(arg: dataTainted) // tainted [NOT DETECTED]
18+
sink(arg: dataTainted2) // tainted [NOT DETECTED]
19+
20+
let stringClean = String(data: dataClean, encoding: String.Encoding.utf8)
21+
let stringTainted = String(data: dataTainted, encoding: String.Encoding.utf8)
22+
23+
sink2(arg: stringClean!) // tainted [NOT DETECTED]
24+
sink2(arg: stringTainted!) // tainted [NOT DETECTED]
25+
}

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

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
func source() -> Int { return 0; }
2+
func sink(arg: String) {}
3+
4+
func taintThroughInterpolatedStrings() {
5+
var x = source()
6+
7+
sink(arg: "\(x)") // tainted
8+
9+
sink(arg: "\(x) \(x)") // tainted
10+
11+
sink(arg: "\(x) \(0) \(x)") // tainted
12+
13+
var y = 42
14+
sink(arg: "\(y)") // clean
15+
16+
sink(arg: "\(x) hello \(y)") // tainted
17+
18+
sink(arg: "\(y) world \(x)") // tainted
19+
20+
x = 0
21+
sink(arg: "\(x)") // clean
22+
}
23+
24+
func source2() -> String { return ""; }
25+
26+
func taintThroughStringConcatenation() {
27+
var clean = "abcdef"
28+
var tainted = source2()
29+
30+
sink(arg: clean)
31+
sink(arg: tainted) // tainted
32+
33+
sink(arg: clean + clean)
34+
sink(arg: clean + tainted) // tainted
35+
sink(arg: tainted + clean) // tainted
36+
sink(arg: tainted + tainted) // tainted
37+
38+
sink(arg: ">" + clean + "<")
39+
sink(arg: ">" + tainted + "<") // tainted
40+
41+
var str = "abc"
42+
43+
sink(arg: str)
44+
45+
str += "def"
46+
sink(arg: str)
47+
48+
str += source2()
49+
sink(arg: str) // tainted [NOT DETECTED]
50+
51+
var str2 = "abc"
52+
53+
sink(arg: str2)
54+
55+
str2.append("def")
56+
sink(arg: str2)
57+
58+
str2.append(source2())
59+
sink(arg: str2) // tainted [NOT DETECTED]
60+
61+
var str3 = "abc"
62+
63+
sink(arg: str3)
64+
65+
str3.append(contentsOf: "def")
66+
sink(arg: str3)
67+
68+
str3.append(contentsOf: source2())
69+
sink(arg: str2) // tainted [NOT DETECTED]
70+
}
71+
72+
func taintThroughStringOperations() {
73+
var clean = ""
74+
var tainted = source2()
75+
var taintedInt = source()
76+
77+
sink(arg: String(clean))
78+
sink(arg: String(tainted)) // tainted [NOT DETECTED]
79+
sink(arg: String(taintedInt)) // tainted [NOT DETECTED]
80+
81+
sink(arg: String(repeating: clean, count: 2))
82+
sink(arg: String(repeating: tainted, count: 2)) // tainted [NOT DETECTED]
83+
84+
sink(arg: clean.description)
85+
sink(arg: tainted.description) // tainted [NOT DETECTED]
86+
87+
sink(arg: clean.debugDescription)
88+
sink(arg: tainted.debugDescription) // tainted [NOT DETECTED]
89+
}

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

Lines changed: 0 additions & 22 deletions
This file was deleted.

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
func clean() throws -> String { return ""; }
2+
func source() throws -> String { return ""; }
3+
func sink(arg: String) {}
4+
5+
func taintThroughTry() {
6+
do
7+
{
8+
sink(arg: try clean())
9+
sink(arg: try source()) // tainted
10+
} catch {
11+
// ...
12+
}
13+
14+
sink(arg: try! clean())
15+
sink(arg: try! source()) // tainted
16+
17+
sink(arg: (try? clean())!)
18+
sink(arg: (try? source())!) // tainted
19+
}

0 commit comments

Comments
 (0)