Skip to content

Commit 66291d3

Browse files
d10cd10c
committed
Swift: sync tests pass with additional flow steps
TODO: Convert those flow steps to taint flow models in the library.
1 parent 7c515bb commit 66291d3

File tree

3 files changed

+176
-18
lines changed

3 files changed

+176
-18
lines changed

‎swift/ql/src/queries/Security/CWE-094/UnsafeJsEval.ql

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,57 @@ class UnsafeJsEvalConfig extends TaintTracking::Configuration {
9494

9595
override predicate isSink(DataFlow::Node node) { node instanceof Sink }
9696

97-
override predicate isSanitizer(DataFlow::Node node) {
98-
none() // TODO: A conversion to a primitive type or an enum
97+
// TODO: convert to new taint flow models
98+
override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
99+
exists(Argument arg |
100+
arg =
101+
any(CallExpr ce |
102+
ce.getStaticTarget()
103+
.(MethodDecl)
104+
.hasQualifiedName("WKUserScript",
105+
[
106+
"init(source:injectionTime:forMainFrameOnly:)",
107+
"init(source:injectionTime:forMainFrameOnly:in:)"
108+
])
109+
).getArgument(0)
110+
or
111+
arg =
112+
any(CallExpr ce |
113+
ce.getStaticTarget()
114+
.(FreeFunctionDecl)
115+
.hasName([
116+
"JSStringCreateWithUTF8CString(_:)", "JSStringCreateWithCharacters(_:_:)",
117+
"JSStringRetain(_:)"
118+
])
119+
).getArgument(0)
120+
|
121+
nodeFrom.asExpr() = arg.getExpr() and
122+
nodeTo.asExpr() = arg.getApplyExpr()
123+
)
124+
or
125+
exists(CallExpr ce, Expr self, AbstractClosureExpr closure |
126+
ce.getStaticTarget()
127+
.getName()
128+
.matches(["withContiguousStorageIfAvailable(%)", "withUnsafeBufferPointer(%)"]) and
129+
self = ce.getQualifier() and
130+
ce.getArgument(0).getExpr() = closure
131+
|
132+
nodeFrom.asExpr() = self and
133+
nodeTo.(DataFlow::ParameterNode).getParameter() = closure.getParam(0)
134+
)
135+
or
136+
exists(MemberRefExpr e, Expr self, VarDecl member |
137+
self.getType().getName() = "String" and
138+
member.getName() = ["utf16", "utf8CString"]
139+
or
140+
self.getType().getName().matches(["Unsafe%Buffer%", "Unsafe%Pointer%"]) and
141+
member.getName() = ["baseAddress"]
142+
|
143+
e.getBase() = self and
144+
e.getMember() = member and
145+
nodeFrom.asExpr() = self and
146+
nodeTo.asExpr() = e
147+
)
99148
}
100149
}
101150

‎swift/ql/test/query-tests/Security/CWE-094/UnsafeJsEval.expected

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
edges
2+
| UnsafeJsEval.swift:123:21:123:42 | string : | UnsafeJsEval.swift:123:70:123:70 | string : |
3+
| UnsafeJsEval.swift:164:10:164:37 | try ... : | UnsafeJsEval.swift:200:21:200:35 | call to getRemoteData() : |
4+
| UnsafeJsEval.swift:164:10:164:37 | try ... : | UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : |
5+
| UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | UnsafeJsEval.swift:164:10:164:37 | try ... : |
6+
| UnsafeJsEval.swift:200:21:200:35 | call to getRemoteData() : | UnsafeJsEval.swift:204:7:204:7 | remoteString : |
7+
| UnsafeJsEval.swift:200:21:200:35 | call to getRemoteData() : | UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : |
8+
| UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : | UnsafeJsEval.swift:264:13:264:13 | string : |
9+
| UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : | UnsafeJsEval.swift:267:13:267:13 | string : |
10+
| UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : | UnsafeJsEval.swift:275:13:275:13 | string : |
11+
| UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : | UnsafeJsEval.swift:278:13:278:13 | string : |
12+
| UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : | UnsafeJsEval.swift:284:13:284:13 | string : |
13+
| UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : | UnsafeJsEval.swift:298:13:298:13 | string : |
14+
| UnsafeJsEval.swift:204:7:204:7 | remoteString : | UnsafeJsEval.swift:264:13:264:13 | string : |
15+
| UnsafeJsEval.swift:204:7:204:7 | remoteString : | UnsafeJsEval.swift:267:13:267:13 | string : |
16+
| UnsafeJsEval.swift:204:7:204:7 | remoteString : | UnsafeJsEval.swift:275:13:275:13 | string : |
17+
| UnsafeJsEval.swift:204:7:204:7 | remoteString : | UnsafeJsEval.swift:278:13:278:13 | string : |
18+
| UnsafeJsEval.swift:204:7:204:7 | remoteString : | UnsafeJsEval.swift:284:13:284:13 | string : |
19+
| UnsafeJsEval.swift:204:7:204:7 | remoteString : | UnsafeJsEval.swift:298:13:298:13 | string : |
20+
| UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : | UnsafeJsEval.swift:264:13:264:13 | string : |
21+
| UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : | UnsafeJsEval.swift:267:13:267:13 | string : |
22+
| UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : | UnsafeJsEval.swift:275:13:275:13 | string : |
23+
| UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : | UnsafeJsEval.swift:278:13:278:13 | string : |
24+
| UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : | UnsafeJsEval.swift:284:13:284:13 | string : |
25+
| UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : | UnsafeJsEval.swift:298:13:298:13 | string : |
26+
| UnsafeJsEval.swift:264:13:264:13 | string : | UnsafeJsEval.swift:265:22:265:107 | call to init(source:injectionTime:forMainFrameOnly:) |
27+
| UnsafeJsEval.swift:267:13:267:13 | string : | UnsafeJsEval.swift:268:22:268:124 | call to init(source:injectionTime:forMainFrameOnly:in:) |
28+
| UnsafeJsEval.swift:275:13:275:13 | string : | UnsafeJsEval.swift:276:26:276:26 | string |
29+
| UnsafeJsEval.swift:278:13:278:13 | string : | UnsafeJsEval.swift:279:26:279:26 | string |
30+
| UnsafeJsEval.swift:284:13:284:13 | string : | UnsafeJsEval.swift:285:3:285:10 | .utf16 : |
31+
| UnsafeJsEval.swift:285:3:285:10 | .utf16 : | UnsafeJsEval.swift:285:51:285:51 | stringBytes : |
32+
| UnsafeJsEval.swift:285:51:285:51 | stringBytes : | UnsafeJsEval.swift:286:31:286:97 | call to JSStringCreateWithCharacters(_:_:) : |
33+
| UnsafeJsEval.swift:285:51:285:51 | stringBytes : | UnsafeJsEval.swift:290:17:290:17 | jsstr |
34+
| UnsafeJsEval.swift:286:16:286:98 | call to JSStringRetain(_:) : | UnsafeJsEval.swift:290:17:290:17 | jsstr |
35+
| UnsafeJsEval.swift:286:31:286:97 | call to JSStringCreateWithCharacters(_:_:) : | UnsafeJsEval.swift:123:21:123:42 | string : |
36+
| UnsafeJsEval.swift:286:31:286:97 | call to JSStringCreateWithCharacters(_:_:) : | UnsafeJsEval.swift:286:16:286:98 | call to JSStringRetain(_:) : |
37+
| UnsafeJsEval.swift:286:31:286:97 | call to JSStringCreateWithCharacters(_:_:) : | UnsafeJsEval.swift:290:17:290:17 | jsstr |
38+
| UnsafeJsEval.swift:298:13:298:13 | string : | UnsafeJsEval.swift:299:3:299:10 | .utf8CString : |
39+
| UnsafeJsEval.swift:299:3:299:10 | .utf8CString : | UnsafeJsEval.swift:299:48:299:48 | stringBytes : |
40+
| UnsafeJsEval.swift:299:48:299:48 | stringBytes : | UnsafeJsEval.swift:300:31:300:84 | call to JSStringCreateWithUTF8CString(_:) : |
41+
| UnsafeJsEval.swift:299:48:299:48 | stringBytes : | UnsafeJsEval.swift:304:17:304:17 | jsstr |
42+
| UnsafeJsEval.swift:300:16:300:85 | call to JSStringRetain(_:) : | UnsafeJsEval.swift:304:17:304:17 | jsstr |
43+
| UnsafeJsEval.swift:300:31:300:84 | call to JSStringCreateWithUTF8CString(_:) : | UnsafeJsEval.swift:123:21:123:42 | string : |
44+
| UnsafeJsEval.swift:300:31:300:84 | call to JSStringCreateWithUTF8CString(_:) : | UnsafeJsEval.swift:300:16:300:85 | call to JSStringRetain(_:) : |
45+
| UnsafeJsEval.swift:300:31:300:84 | call to JSStringCreateWithUTF8CString(_:) : | UnsafeJsEval.swift:304:17:304:17 | jsstr |
46+
nodes
47+
| UnsafeJsEval.swift:123:21:123:42 | string : | semmle.label | string : |
48+
| UnsafeJsEval.swift:123:70:123:70 | string : | semmle.label | string : |
49+
| UnsafeJsEval.swift:164:10:164:37 | try ... : | semmle.label | try ... : |
50+
| UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | semmle.label | call to init(contentsOf:) : |
51+
| UnsafeJsEval.swift:200:21:200:35 | call to getRemoteData() : | semmle.label | call to getRemoteData() : |
52+
| UnsafeJsEval.swift:203:7:203:21 | call to getRemoteData() : | semmle.label | call to getRemoteData() : |
53+
| UnsafeJsEval.swift:204:7:204:7 | remoteString : | semmle.label | remoteString : |
54+
| UnsafeJsEval.swift:207:7:207:39 | ... .+(_:_:) ... : | semmle.label | ... .+(_:_:) ... : |
55+
| UnsafeJsEval.swift:264:13:264:13 | string : | semmle.label | string : |
56+
| UnsafeJsEval.swift:265:22:265:107 | call to init(source:injectionTime:forMainFrameOnly:) | semmle.label | call to init(source:injectionTime:forMainFrameOnly:) |
57+
| UnsafeJsEval.swift:267:13:267:13 | string : | semmle.label | string : |
58+
| UnsafeJsEval.swift:268:22:268:124 | call to init(source:injectionTime:forMainFrameOnly:in:) | semmle.label | call to init(source:injectionTime:forMainFrameOnly:in:) |
59+
| UnsafeJsEval.swift:275:13:275:13 | string : | semmle.label | string : |
60+
| UnsafeJsEval.swift:276:26:276:26 | string | semmle.label | string |
61+
| UnsafeJsEval.swift:278:13:278:13 | string : | semmle.label | string : |
62+
| UnsafeJsEval.swift:279:26:279:26 | string | semmle.label | string |
63+
| UnsafeJsEval.swift:284:13:284:13 | string : | semmle.label | string : |
64+
| UnsafeJsEval.swift:285:3:285:10 | .utf16 : | semmle.label | .utf16 : |
65+
| UnsafeJsEval.swift:285:51:285:51 | stringBytes : | semmle.label | stringBytes : |
66+
| UnsafeJsEval.swift:286:16:286:98 | call to JSStringRetain(_:) : | semmle.label | call to JSStringRetain(_:) : |
67+
| UnsafeJsEval.swift:286:31:286:97 | call to JSStringCreateWithCharacters(_:_:) : | semmle.label | call to JSStringCreateWithCharacters(_:_:) : |
68+
| UnsafeJsEval.swift:290:17:290:17 | jsstr | semmle.label | jsstr |
69+
| UnsafeJsEval.swift:298:13:298:13 | string : | semmle.label | string : |
70+
| UnsafeJsEval.swift:299:3:299:10 | .utf8CString : | semmle.label | .utf8CString : |
71+
| UnsafeJsEval.swift:299:48:299:48 | stringBytes : | semmle.label | stringBytes : |
72+
| UnsafeJsEval.swift:300:16:300:85 | call to JSStringRetain(_:) : | semmle.label | call to JSStringRetain(_:) : |
73+
| UnsafeJsEval.swift:300:31:300:84 | call to JSStringCreateWithUTF8CString(_:) : | semmle.label | call to JSStringCreateWithUTF8CString(_:) : |
74+
| UnsafeJsEval.swift:304:17:304:17 | jsstr | semmle.label | jsstr |
75+
subpaths
76+
| UnsafeJsEval.swift:286:31:286:97 | call to JSStringCreateWithCharacters(_:_:) : | UnsafeJsEval.swift:123:21:123:42 | string : | UnsafeJsEval.swift:123:70:123:70 | string : | UnsafeJsEval.swift:286:16:286:98 | call to JSStringRetain(_:) : |
77+
| UnsafeJsEval.swift:300:31:300:84 | call to JSStringCreateWithUTF8CString(_:) : | UnsafeJsEval.swift:123:21:123:42 | string : | UnsafeJsEval.swift:123:70:123:70 | string : | UnsafeJsEval.swift:300:16:300:85 | call to JSStringRetain(_:) : |
78+
#select
79+
| UnsafeJsEval.swift:265:22:265:107 | call to init(source:injectionTime:forMainFrameOnly:) | UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | UnsafeJsEval.swift:265:22:265:107 | call to init(source:injectionTime:forMainFrameOnly:) | Evaluation of uncontrolled JavaScript from a remote source. |
80+
| UnsafeJsEval.swift:268:22:268:124 | call to init(source:injectionTime:forMainFrameOnly:in:) | UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | UnsafeJsEval.swift:268:22:268:124 | call to init(source:injectionTime:forMainFrameOnly:in:) | Evaluation of uncontrolled JavaScript from a remote source. |
81+
| UnsafeJsEval.swift:276:26:276:26 | string | UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | UnsafeJsEval.swift:276:26:276:26 | string | Evaluation of uncontrolled JavaScript from a remote source. |
82+
| UnsafeJsEval.swift:279:26:279:26 | string | UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | UnsafeJsEval.swift:279:26:279:26 | string | Evaluation of uncontrolled JavaScript from a remote source. |
83+
| UnsafeJsEval.swift:290:17:290:17 | jsstr | UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | UnsafeJsEval.swift:290:17:290:17 | jsstr | Evaluation of uncontrolled JavaScript from a remote source. |
84+
| UnsafeJsEval.swift:304:17:304:17 | jsstr | UnsafeJsEval.swift:164:14:164:37 | call to init(contentsOf:) : | UnsafeJsEval.swift:304:17:304:17 | jsstr | Evaluation of uncontrolled JavaScript from a remote source. |

‎swift/ql/test/query-tests/Security/CWE-094/UnsafeJsEval.swift

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ func getRemoteData() -> String {
167167
}
168168
}
169169

170-
func testUsage(_ sink: @escaping (String) async throws -> ()) {
170+
func testAsync(_ sink: @escaping (String) async throws -> ()) {
171171
Task {
172172
let localString = "console.log('localString')"
173173
let localStringFragment = "'localStringFragment'"
@@ -184,7 +184,7 @@ func testUsage(_ sink: @escaping (String) async throws -> ()) {
184184
let remoteData = Data(remoteString.utf8)
185185

186186
try! await sink(String(decoding: localData, as: UTF8.self)) // GOOD: the data is local
187-
try! await sink(String(decoding: remoteData, as: UTF8.self)) // BAD: the data is remote
187+
try! await sink(String(decoding: remoteData, as: UTF8.self)) // BAD [NOT DETECTED]: the data is remote (TODO: model Data taint sources)
188188

189189
try! await sink("console.log(" + String(Int(localStringFragment) ?? 0) + ")") // GOOD: Primitive conversion
190190
try! await sink("console.log(" + String(Int(remoteString) ?? 0) + ")") // GOOD: Primitive conversion
@@ -194,69 +194,94 @@ func testUsage(_ sink: @escaping (String) async throws -> ()) {
194194
}
195195
}
196196

197+
func testSync(_ sink: @escaping (String) -> ()) {
198+
let localString = "console.log('localString')"
199+
let localStringFragment = "'localStringFragment'"
200+
let remoteString = getRemoteData()
201+
202+
sink(localString) // GOOD: the HTML data is local
203+
sink(getRemoteData()) // BAD: HTML contains remote input, may access local secrets
204+
sink(remoteString) // BAD
205+
206+
sink("console.log(" + localStringFragment + ")") // GOOD: the HTML data is local
207+
sink("console.log(" + remoteString + ")") // BAD
208+
209+
let localData = Data(localString.utf8)
210+
let remoteData = Data(remoteString.utf8)
211+
212+
sink(String(decoding: localData, as: UTF8.self)) // GOOD: the data is local
213+
sink(String(decoding: remoteData, as: UTF8.self)) // BAD [NOT DETECTED]: the data is remote (TODO: model Data taint sources)
214+
215+
sink("console.log(" + String(Int(localStringFragment) ?? 0) + ")") // GOOD: Primitive conversion
216+
sink("console.log(" + String(Int(remoteString) ?? 0) + ")") // GOOD: Primitive conversion
217+
218+
sink("console.log(" + (localStringFragment.count != 0 ? "1" : "0") + ")") // GOOD: Primitive conversion
219+
sink("console.log(" + (remoteString.count != 0 ? "1" : "0") + ")") // GOOD: Primitive conversion
220+
}
221+
197222
func testUIWebView() {
198223
let webview = UIWebView()
199224

200-
testUsage { string in
225+
testAsync { string in
201226
_ = await webview.stringByEvaluatingJavaScript(from: string)
202227
}
203228
}
204229

205230
func testWebView() {
206231
let webview = WebView()
207232

208-
testUsage { string in
233+
testAsync { string in
209234
_ = await webview.stringByEvaluatingJavaScript(from: string)
210235
}
211236
}
212237

213238
func testWKWebView() {
214239
let webview = WKWebView()
215240

216-
testUsage { string in
241+
testAsync { string in
217242
_ = try await webview.evaluateJavaScript(string)
218243
}
219-
testUsage { string in
244+
testAsync { string in
220245
await webview.evaluateJavaScript(string) { _, _ in }
221246
}
222-
testUsage { string in
247+
testAsync { string in
223248
await webview.evaluateJavaScript(string, in: nil, in: WKContentWorld.defaultClient) { _ in }
224249
}
225-
testUsage { string in
250+
testAsync { string in
226251
_ = try await webview.evaluateJavaScript(string, contentWorld: .defaultClient)
227252
}
228-
testUsage { string in
253+
testAsync { string in
229254
await webview.callAsyncJavaScript(string, in: nil, in: .defaultClient) { _ in () }
230255
}
231-
testUsage { string in
256+
testAsync { string in
232257
_ = try await webview.callAsyncJavaScript(string, contentWorld: WKContentWorld.defaultClient)
233258
}
234259
}
235260

236261
func testWKUserContentController() {
237262
let ctrl = WKUserContentController()
238263

239-
testUsage { string in
264+
testSync { string in
240265
ctrl.addUserScript(WKUserScript(source: string, injectionTime: .atDocumentStart, forMainFrameOnly: false))
241266
}
242-
testUsage { string in
267+
testSync { string in
243268
ctrl.addUserScript(WKUserScript(source: string, injectionTime: .atDocumentEnd, forMainFrameOnly: true, in: .defaultClient))
244269
}
245270
}
246271

247272
func testJSContext() {
248273
let ctx = JSContext()
249274

250-
testUsage { string in
275+
testSync { string in
251276
_ = ctx.evaluateScript(string)
252277
}
253-
testUsage { string in
278+
testSync { string in
254279
_ = ctx.evaluateScript(string, withSourceURL: URL(string: "https://example.com"))
255280
}
256281
}
257282

258283
func testJSEvaluateScript() {
259-
testUsage { string in
284+
testSync { string in
260285
string.utf16.withContiguousStorageIfAvailable { stringBytes in
261286
let jsstr = JSStringRetain(JSStringCreateWithCharacters(stringBytes.baseAddress, string.count))
262287
defer { JSStringRelease(jsstr) }
@@ -270,7 +295,7 @@ func testJSEvaluateScript() {
270295
)
271296
}
272297
}
273-
testUsage { string in
298+
testSync { string in
274299
string.utf8CString.withUnsafeBufferPointer { stringBytes in
275300
let jsstr = JSStringRetain(JSStringCreateWithUTF8CString(stringBytes.baseAddress))
276301
defer { JSStringRelease(jsstr) }

0 commit comments

Comments
 (0)