Skip to content

Commit f7776f8

Browse files
geoffw0geoffw0
committed
Swift: 'good enough' fix for UnsafeJsEval flow.
1 parent 317757b commit f7776f8

File tree

3 files changed

+59
-15
lines changed

3 files changed

+59
-15
lines changed

‎swift/ql/lib/codeql/swift/security/UnsafeJsEvalExtensions.qll

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -102,21 +102,6 @@ private class JSEvaluateScriptDefaultUnsafeJsEvalSink extends UnsafeJsEvalSink {
102102
*/
103103
private class DefaultUnsafeJsEvalAdditionalFlowStep extends UnsafeJsEvalAdditionalFlowStep {
104104
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
105-
exists(Argument arg |
106-
arg =
107-
any(CallExpr ce |
108-
ce.getStaticTarget()
109-
.(FreeFunction)
110-
.hasName([
111-
"JSStringCreateWithUTF8CString(_:)", "JSStringCreateWithCharacters(_:_:)",
112-
"JSStringRetain(_:)"
113-
])
114-
).getArgument(0)
115-
|
116-
nodeFrom.asExpr() = arg.getExpr() and
117-
nodeTo.asExpr() = arg.getApplyExpr()
118-
)
119-
or
120105
exists(MemberRefExpr e, Expr self, VarDecl member |
121106
self.getType().getFullName().matches(["Unsafe%Buffer%", "Unsafe%Pointer%"]) and
122107
member.getName() = "baseAddress"

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,19 @@ module UnsafeJsEvalConfig implements DataFlow::ConfigSig {
2222
predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
2323
any(UnsafeJsEvalAdditionalFlowStep s).step(nodeFrom, nodeTo)
2424
}
25+
26+
predicate allowImplicitRead(DataFlow::Node node, DataFlow::ContentSet c) {
27+
// flow out from content a the sink
28+
(
29+
isSink(node)
30+
or
31+
isAdditionalFlowStep(node, _)
32+
) and
33+
(
34+
c.getAReadContent() instanceof DataFlow::Content::ArrayContent or
35+
c.getAReadContent() instanceof DataFlow::Content::CollectionContent
36+
)
37+
}
2538
}
2639

2740
/**

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

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,28 +8,52 @@ edges
88
| UnsafeJsEval.swift:204:7:204:66 | try! ... | UnsafeJsEval.swift:268:13:268:13 | string |
99
| UnsafeJsEval.swift:204:7:204:66 | try! ... | UnsafeJsEval.swift:276:13:276:13 | string |
1010
| UnsafeJsEval.swift:204:7:204:66 | try! ... | UnsafeJsEval.swift:279:13:279:13 | string |
11+
| UnsafeJsEval.swift:204:7:204:66 | try! ... | UnsafeJsEval.swift:285:13:285:13 | string |
12+
| UnsafeJsEval.swift:204:7:204:66 | try! ... | UnsafeJsEval.swift:299:13:299:13 | string |
1113
| UnsafeJsEval.swift:204:12:204:66 | call to String.init(contentsOf:) | UnsafeJsEval.swift:204:7:204:66 | try! ... |
1214
| UnsafeJsEval.swift:205:7:205:7 | remoteString | UnsafeJsEval.swift:265:13:265:13 | string |
1315
| UnsafeJsEval.swift:205:7:205:7 | remoteString | UnsafeJsEval.swift:268:13:268:13 | string |
1416
| UnsafeJsEval.swift:205:7:205:7 | remoteString | UnsafeJsEval.swift:276:13:276:13 | string |
1517
| UnsafeJsEval.swift:205:7:205:7 | remoteString | UnsafeJsEval.swift:279:13:279:13 | string |
18+
| UnsafeJsEval.swift:205:7:205:7 | remoteString | UnsafeJsEval.swift:285:13:285:13 | string |
19+
| UnsafeJsEval.swift:205:7:205:7 | remoteString | UnsafeJsEval.swift:299:13:299:13 | string |
1620
| UnsafeJsEval.swift:208:7:208:39 | ... .+(_:_:) ... | UnsafeJsEval.swift:265:13:265:13 | string |
1721
| UnsafeJsEval.swift:208:7:208:39 | ... .+(_:_:) ... | UnsafeJsEval.swift:268:13:268:13 | string |
1822
| UnsafeJsEval.swift:208:7:208:39 | ... .+(_:_:) ... | UnsafeJsEval.swift:276:13:276:13 | string |
1923
| UnsafeJsEval.swift:208:7:208:39 | ... .+(_:_:) ... | UnsafeJsEval.swift:279:13:279:13 | string |
24+
| UnsafeJsEval.swift:208:7:208:39 | ... .+(_:_:) ... | UnsafeJsEval.swift:285:13:285:13 | string |
25+
| UnsafeJsEval.swift:208:7:208:39 | ... .+(_:_:) ... | UnsafeJsEval.swift:299:13:299:13 | string |
2026
| UnsafeJsEval.swift:211:19:211:41 | call to Data.init(_:) | UnsafeJsEval.swift:214:24:214:24 | remoteData |
2127
| UnsafeJsEval.swift:211:24:211:37 | .utf8 | UnsafeJsEval.swift:211:19:211:41 | call to Data.init(_:) |
2228
| UnsafeJsEval.swift:214:7:214:49 | call to String.init(decoding:as:) | UnsafeJsEval.swift:265:13:265:13 | string |
2329
| UnsafeJsEval.swift:214:7:214:49 | call to String.init(decoding:as:) | UnsafeJsEval.swift:268:13:268:13 | string |
2430
| UnsafeJsEval.swift:214:7:214:49 | call to String.init(decoding:as:) | UnsafeJsEval.swift:276:13:276:13 | string |
2531
| UnsafeJsEval.swift:214:7:214:49 | call to String.init(decoding:as:) | UnsafeJsEval.swift:279:13:279:13 | string |
32+
| UnsafeJsEval.swift:214:7:214:49 | call to String.init(decoding:as:) | UnsafeJsEval.swift:285:13:285:13 | string |
33+
| UnsafeJsEval.swift:214:7:214:49 | call to String.init(decoding:as:) | UnsafeJsEval.swift:299:13:299:13 | string |
2634
| UnsafeJsEval.swift:214:24:214:24 | remoteData | UnsafeJsEval.swift:214:7:214:49 | call to String.init(decoding:as:) |
2735
| UnsafeJsEval.swift:265:13:265:13 | string | UnsafeJsEval.swift:266:43:266:43 | string |
2836
| UnsafeJsEval.swift:266:43:266:43 | string | UnsafeJsEval.swift:266:22:266:107 | call to WKUserScript.init(source:injectionTime:forMainFrameOnly:) |
2937
| UnsafeJsEval.swift:268:13:268:13 | string | UnsafeJsEval.swift:269:43:269:43 | string |
3038
| UnsafeJsEval.swift:269:43:269:43 | string | UnsafeJsEval.swift:269:22:269:124 | call to WKUserScript.init(source:injectionTime:forMainFrameOnly:in:) |
3139
| UnsafeJsEval.swift:276:13:276:13 | string | UnsafeJsEval.swift:277:26:277:26 | string |
3240
| UnsafeJsEval.swift:279:13:279:13 | string | UnsafeJsEval.swift:280:26:280:26 | string |
41+
| UnsafeJsEval.swift:285:13:285:13 | string | UnsafeJsEval.swift:286:3:286:10 | .utf16 |
42+
| UnsafeJsEval.swift:286:3:286:10 | .utf16 | UnsafeJsEval.swift:286:51:286:51 | stringBytes [Collection element] |
43+
| UnsafeJsEval.swift:286:51:286:51 | stringBytes [Collection element] | UnsafeJsEval.swift:287:60:287:60 | stringBytes [Collection element] |
44+
| UnsafeJsEval.swift:287:16:287:98 | call to JSStringRetain(_:) | UnsafeJsEval.swift:291:17:291:17 | jsstr |
45+
| UnsafeJsEval.swift:287:31:287:97 | call to JSStringCreateWithCharacters(_:_:) | UnsafeJsEval.swift:287:16:287:98 | call to JSStringRetain(_:) |
46+
| UnsafeJsEval.swift:287:60:287:60 | stringBytes | UnsafeJsEval.swift:287:60:287:72 | .baseAddress |
47+
| UnsafeJsEval.swift:287:60:287:60 | stringBytes [Collection element] | UnsafeJsEval.swift:287:60:287:60 | stringBytes |
48+
| UnsafeJsEval.swift:287:60:287:72 | .baseAddress | UnsafeJsEval.swift:287:31:287:97 | call to JSStringCreateWithCharacters(_:_:) |
49+
| UnsafeJsEval.swift:299:13:299:13 | string | UnsafeJsEval.swift:300:3:300:10 | .utf8CString |
50+
| UnsafeJsEval.swift:300:3:300:10 | .utf8CString | UnsafeJsEval.swift:300:48:300:48 | stringBytes [Collection element] |
51+
| UnsafeJsEval.swift:300:48:300:48 | stringBytes [Collection element] | UnsafeJsEval.swift:301:61:301:61 | stringBytes [Collection element] |
52+
| UnsafeJsEval.swift:301:16:301:85 | call to JSStringRetain(_:) | UnsafeJsEval.swift:305:17:305:17 | jsstr |
53+
| UnsafeJsEval.swift:301:31:301:84 | call to JSStringCreateWithUTF8CString(_:) | UnsafeJsEval.swift:301:16:301:85 | call to JSStringRetain(_:) |
54+
| UnsafeJsEval.swift:301:61:301:61 | stringBytes | UnsafeJsEval.swift:301:61:301:73 | .baseAddress |
55+
| UnsafeJsEval.swift:301:61:301:61 | stringBytes [Collection element] | UnsafeJsEval.swift:301:61:301:61 | stringBytes |
56+
| UnsafeJsEval.swift:301:61:301:73 | .baseAddress | UnsafeJsEval.swift:301:31:301:84 | call to JSStringCreateWithUTF8CString(_:) |
3357
| UnsafeJsEval.swift:318:24:318:87 | call to String.init(contentsOf:) | UnsafeJsEval.swift:320:44:320:74 | ... .+(_:_:) ... |
3458
nodes
3559
| UnsafeJsEval.swift:165:10:165:37 | try ... | semmle.label | try ... |
@@ -53,6 +77,24 @@ nodes
5377
| UnsafeJsEval.swift:277:26:277:26 | string | semmle.label | string |
5478
| UnsafeJsEval.swift:279:13:279:13 | string | semmle.label | string |
5579
| UnsafeJsEval.swift:280:26:280:26 | string | semmle.label | string |
80+
| UnsafeJsEval.swift:285:13:285:13 | string | semmle.label | string |
81+
| UnsafeJsEval.swift:286:3:286:10 | .utf16 | semmle.label | .utf16 |
82+
| UnsafeJsEval.swift:286:51:286:51 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
83+
| UnsafeJsEval.swift:287:16:287:98 | call to JSStringRetain(_:) | semmle.label | call to JSStringRetain(_:) |
84+
| UnsafeJsEval.swift:287:31:287:97 | call to JSStringCreateWithCharacters(_:_:) | semmle.label | call to JSStringCreateWithCharacters(_:_:) |
85+
| UnsafeJsEval.swift:287:60:287:60 | stringBytes | semmle.label | stringBytes |
86+
| UnsafeJsEval.swift:287:60:287:60 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
87+
| UnsafeJsEval.swift:287:60:287:72 | .baseAddress | semmle.label | .baseAddress |
88+
| UnsafeJsEval.swift:291:17:291:17 | jsstr | semmle.label | jsstr |
89+
| UnsafeJsEval.swift:299:13:299:13 | string | semmle.label | string |
90+
| UnsafeJsEval.swift:300:3:300:10 | .utf8CString | semmle.label | .utf8CString |
91+
| UnsafeJsEval.swift:300:48:300:48 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
92+
| UnsafeJsEval.swift:301:16:301:85 | call to JSStringRetain(_:) | semmle.label | call to JSStringRetain(_:) |
93+
| UnsafeJsEval.swift:301:31:301:84 | call to JSStringCreateWithUTF8CString(_:) | semmle.label | call to JSStringCreateWithUTF8CString(_:) |
94+
| UnsafeJsEval.swift:301:61:301:61 | stringBytes | semmle.label | stringBytes |
95+
| UnsafeJsEval.swift:301:61:301:61 | stringBytes [Collection element] | semmle.label | stringBytes [Collection element] |
96+
| UnsafeJsEval.swift:301:61:301:73 | .baseAddress | semmle.label | .baseAddress |
97+
| UnsafeJsEval.swift:305:17:305:17 | jsstr | semmle.label | jsstr |
5698
| UnsafeJsEval.swift:318:24:318:87 | call to String.init(contentsOf:) | semmle.label | call to String.init(contentsOf:) |
5799
| UnsafeJsEval.swift:320:44:320:74 | ... .+(_:_:) ... | semmle.label | ... .+(_:_:) ... |
58100
subpaths
@@ -65,4 +107,8 @@ subpaths
65107
| UnsafeJsEval.swift:277:26:277:26 | string | UnsafeJsEval.swift:204:12:204:66 | call to String.init(contentsOf:) | UnsafeJsEval.swift:277:26:277:26 | string | Evaluation of uncontrolled JavaScript from a remote source. |
66108
| UnsafeJsEval.swift:280:26:280:26 | string | UnsafeJsEval.swift:165:14:165:37 | call to String.init(contentsOf:) | UnsafeJsEval.swift:280:26:280:26 | string | Evaluation of uncontrolled JavaScript from a remote source. |
67109
| UnsafeJsEval.swift:280:26:280:26 | string | UnsafeJsEval.swift:204:12:204:66 | call to String.init(contentsOf:) | UnsafeJsEval.swift:280:26:280:26 | string | Evaluation of uncontrolled JavaScript from a remote source. |
110+
| UnsafeJsEval.swift:291:17:291:17 | jsstr | UnsafeJsEval.swift:165:14:165:37 | call to String.init(contentsOf:) | UnsafeJsEval.swift:291:17:291:17 | jsstr | Evaluation of uncontrolled JavaScript from a remote source. |
111+
| UnsafeJsEval.swift:291:17:291:17 | jsstr | UnsafeJsEval.swift:204:12:204:66 | call to String.init(contentsOf:) | UnsafeJsEval.swift:291:17:291:17 | jsstr | Evaluation of uncontrolled JavaScript from a remote source. |
112+
| UnsafeJsEval.swift:305:17:305:17 | jsstr | UnsafeJsEval.swift:165:14:165:37 | call to String.init(contentsOf:) | UnsafeJsEval.swift:305:17:305:17 | jsstr | Evaluation of uncontrolled JavaScript from a remote source. |
113+
| UnsafeJsEval.swift:305:17:305:17 | jsstr | UnsafeJsEval.swift:204:12:204:66 | call to String.init(contentsOf:) | UnsafeJsEval.swift:305:17:305:17 | jsstr | Evaluation of uncontrolled JavaScript from a remote source. |
68114
| UnsafeJsEval.swift:320:44:320:74 | ... .+(_:_:) ... | UnsafeJsEval.swift:318:24:318:87 | call to String.init(contentsOf:) | UnsafeJsEval.swift:320:44:320:74 | ... .+(_:_:) ... | Evaluation of uncontrolled JavaScript from a remote source. |

0 commit comments

Comments
 (0)