Skip to content

Commit 8f065e9

Browse files
d10cd10c
authored
Merge pull request #11001 from d10c/swift/js-injection
2 parents cb4a7e2 + ef837f7 commit 8f065e9

File tree

11 files changed

+676
-6
lines changed

11 files changed

+676
-6
lines changed

‎.vscode/settings.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
11
{
2-
"omnisharp.autoStart": false
2+
"omnisharp.autoStart": false,
3+
"cmake.sourceDirectory": "${workspaceFolder}/swift",
4+
"cmake.buildDirectory": "${workspaceFolder}/bazel-cmake-build"
35
}

‎swift/CMakeLists.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,10 @@ set(CMAKE_CXX_EXTENSIONS OFF)
99
set(CMAKE_C_COMPILER clang)
1010
set(CMAKE_CXX_COMPILER clang++)
1111

12+
if(APPLE)
13+
set(CMAKE_OSX_ARCHITECTURES x86_64) # temporary until we can build a Universal Binary
14+
endif()
15+
1216
project(codeql)
1317

1418
include(../misc/bazel/cmake/setup.cmake)

‎swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,6 +1015,14 @@ module Decls {
10151015

10161016
module Exprs {
10171017
module AssignExprs {
1018+
/**
1019+
* The control-flow of a `DiscardAssignmentExpr`, which represents the
1020+
* `_` leaf expression that may appear on the left-hand side of an `AssignExpr`.
1021+
*/
1022+
private class DiscardAssignmentExprTree extends AstLeafTree {
1023+
override DiscardAssignmentExpr ast;
1024+
}
1025+
10181026
/**
10191027
* The control-flow of an assignment operation.
10201028
*

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Evaluating JavaScript that contains a substring from a remote origin may lead to remote code execution. Code written by an attacker can execute unauthorized actions, including exfiltration of local data through a third party web service.</p>
7+
8+
</overview>
9+
<recommendation>
10+
11+
<p>When loading JavaScript into a web view, evaluate only known, locally-defined source code. If part of the input comes from a remote source, do not inject it into the JavaScript code to be evaluated. Instead, send it to the web view as data using an API such as <code>WKWebView.callAsyncJavaScript</code> with the <code>arguments</code> dictionary to pass remote data objects.</p>
12+
13+
</recommendation>
14+
<example>
15+
16+
<p>In the following (bad) example, a call to <code>WKWebView.evaluateJavaScript</code> evaluates JavaScript source code that is tainted with remote data, potentially introducing a code injection vulnerability.</p>
17+
18+
<sample src="UnsafeJsEvalBad.swift" />
19+
20+
<p>In the following (good) example, we sanitize the remote data by passing it using the <code>arguments</code> dictionary of <code>WKWebView.callAsyncJavaScript</code>. This ensures that untrusted data cannot be evaluated as JavaScript source code.</p>
21+
22+
<sample src="UnsafeJsEvalGood.swift" />
23+
24+
</example>
25+
<references>
26+
27+
<li>
28+
Apple Developer Documentation: <a href="https://developer.apple.com/documentation/webkit/wkwebview/3824703-callasyncjavascript">WKWebView.callAsyncJavaScript(_:arguments:in:contentWorld:)</a>
29+
</li>
30+
31+
</references>
32+
</qhelp>

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

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
/**
2+
* @name JavaScript Injection
3+
* @description Evaluating JavaScript code containing a substring from a remote source may lead to remote code execution.
4+
* @kind path-problem
5+
* @problem.severity warning
6+
* @security-severity 9.3
7+
* @precision high
8+
* @id swift/unsafe-js-eval
9+
* @tags security
10+
* external/cwe/cwe-094
11+
* external/cwe/cwe-095
12+
* external/cwe/cwe-749
13+
*/
14+
15+
import swift
16+
import codeql.swift.dataflow.DataFlow
17+
import codeql.swift.dataflow.TaintTracking
18+
import codeql.swift.dataflow.FlowSources
19+
import DataFlow::PathGraph
20+
21+
/**
22+
* A source of untrusted, user-controlled data.
23+
* TODO: Extend to more (non-remote) sources in the future.
24+
*/
25+
class Source = RemoteFlowSource;
26+
27+
/**
28+
* A sink that evaluates a string of JavaScript code.
29+
*/
30+
abstract class Sink extends DataFlow::Node { }
31+
32+
class WKWebView extends Sink {
33+
WKWebView() {
34+
any(CallExpr ce |
35+
ce.getStaticTarget()
36+
.(MethodDecl)
37+
.hasQualifiedName("WKWebView",
38+
[
39+
"evaluateJavaScript(_:)", "evaluateJavaScript(_:completionHandler:)",
40+
"evaluateJavaScript(_:in:in:completionHandler:)",
41+
"evaluateJavaScript(_:in:contentWorld:)",
42+
"callAsyncJavaScript(_:arguments:in:in:completionHandler:)",
43+
"callAsyncJavaScript(_:arguments:in:contentWorld:)"
44+
])
45+
).getArgument(0).getExpr() = this.asExpr()
46+
}
47+
}
48+
49+
class WKUserContentController extends Sink {
50+
WKUserContentController() {
51+
any(CallExpr ce |
52+
ce.getStaticTarget()
53+
.(MethodDecl)
54+
.hasQualifiedName("WKUserContentController", "addUserScript(_:)")
55+
).getArgument(0).getExpr() = this.asExpr()
56+
}
57+
}
58+
59+
class UIWebView extends Sink {
60+
UIWebView() {
61+
any(CallExpr ce |
62+
ce.getStaticTarget()
63+
.(MethodDecl)
64+
.hasQualifiedName(["UIWebView", "WebView"], "stringByEvaluatingJavaScript(from:)")
65+
).getArgument(0).getExpr() = this.asExpr()
66+
}
67+
}
68+
69+
class JSContext extends Sink {
70+
JSContext() {
71+
any(CallExpr ce |
72+
ce.getStaticTarget()
73+
.(MethodDecl)
74+
.hasQualifiedName("JSContext", ["evaluateScript(_:)", "evaluateScript(_:withSourceURL:)"])
75+
).getArgument(0).getExpr() = this.asExpr()
76+
}
77+
}
78+
79+
class JSEvaluateScript extends Sink {
80+
JSEvaluateScript() {
81+
any(CallExpr ce |
82+
ce.getStaticTarget().(FreeFunctionDecl).hasName("JSEvaluateScript(_:_:_:_:_:_:)")
83+
).getArgument(1).getExpr() = this.asExpr()
84+
}
85+
}
86+
87+
/**
88+
* A taint configuration from taint sources to sinks for this query.
89+
*/
90+
class UnsafeJsEvalConfig extends TaintTracking::Configuration {
91+
UnsafeJsEvalConfig() { this = "UnsafeJsEvalConfig" }
92+
93+
override predicate isSource(DataFlow::Node node) { node instanceof Source }
94+
95+
override predicate isSink(DataFlow::Node node) { node instanceof Sink }
96+
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 | ce.getStaticTarget().(MethodDecl).hasQualifiedName("Data", "init(_:)"))
113+
.getArgument(0)
114+
or
115+
arg =
116+
any(CallExpr ce |
117+
ce.getStaticTarget().(MethodDecl).hasQualifiedName("String", "init(decoding:as:)")
118+
).getArgument(0)
119+
or
120+
arg =
121+
any(CallExpr ce |
122+
ce.getStaticTarget()
123+
.(FreeFunctionDecl)
124+
.hasName([
125+
"JSStringCreateWithUTF8CString(_:)", "JSStringCreateWithCharacters(_:_:)",
126+
"JSStringRetain(_:)"
127+
])
128+
).getArgument(0)
129+
|
130+
nodeFrom.asExpr() = arg.getExpr() and
131+
nodeTo.asExpr() = arg.getApplyExpr()
132+
)
133+
or
134+
exists(CallExpr ce, Expr self, AbstractClosureExpr closure |
135+
ce.getStaticTarget()
136+
.getName()
137+
.matches(["withContiguousStorageIfAvailable(%)", "withUnsafeBufferPointer(%)"]) and
138+
self = ce.getQualifier() and
139+
ce.getArgument(0).getExpr() = closure
140+
|
141+
nodeFrom.asExpr() = self and
142+
nodeTo.(DataFlow::ParameterNode).getParameter() = closure.getParam(0)
143+
)
144+
or
145+
exists(MemberRefExpr e, Expr self, VarDecl member |
146+
self.getType().getName() = "String" and
147+
member.getName() = ["utf8", "utf16", "utf8CString"]
148+
or
149+
self.getType().getName().matches(["Unsafe%Buffer%", "Unsafe%Pointer%"]) and
150+
member.getName() = "baseAddress"
151+
|
152+
e.getBase() = self and
153+
e.getMember() = member and
154+
nodeFrom.asExpr() = self and
155+
nodeTo.asExpr() = e
156+
)
157+
}
158+
}
159+
160+
from
161+
UnsafeJsEvalConfig config, DataFlow::PathNode sourceNode, DataFlow::PathNode sinkNode, Sink sink
162+
where
163+
config.hasFlowPath(sourceNode, sinkNode) and
164+
sink = sinkNode.getNode()
165+
select sink, sourceNode, sinkNode, "Evaluation of uncontrolled JavaScript from a remote source."

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
let webview: WKWebView
2+
let remoteData = try String(contentsOf: URL(string: "http://example.com/evil.json")!)
3+
4+
...
5+
6+
_ = try await webview.evaluateJavaScript("console.log(" + remoteData + ")") // BAD

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
let webview: WKWebView
2+
let remoteData = try String(contentsOf: URL(string: "http://example.com/evil.json")!)
3+
4+
...
5+
6+
_ = try await webview.callAsyncJavaScript(
7+
"console.log(data)",
8+
arguments: ["data": remoteData], // GOOD
9+
contentWorld: .page
10+
)

0 commit comments

Comments
 (0)