Swift: Unsafe JS Eval Query by d10c · Pull Request #11001 · github/codeql

d10c · 2022-10-26T18:29:57Z

No description provided.

geoffw0 · 2022-11-02T17:28:39Z

+		let remoteString = getRemoteData()
+
+		try! await sink(localString) // GOOD: the HTML data is local
+		try! await sink(getRemoteData()) // BAD: HTML contains remote input, may access local secrets


How will we tell which sink types each case is working for? Might we be better off with thorough tests for just one sink type, then an additional simple test of each other sink type to verify coverage?

True, it would be simpler if I inlined (most of) the tests. But we can eye-ball the correctness of the test by running the query in VSCode and checking that each of the sinks has the same number of paths leading to it.

See concurrency note here: https://developer.apple.com/documentation/webkit/wkwebview/1415017-evaluatejavascript See also https://developer.apple.com/documentation/swift/calling-objective-c-apis-asynchronously

The `-arch=x86_64` from `swift/rules.bzl` turns out to be unnecessary, even on Arm-based Macs.

Instead of hand-rolled predicates.

This enables DataFlow to skip over it and not get stuck.

TODO: Convert those flow steps to taint flow models in the library.

Still TODO: a more comprehensive taint flow model for Data in the libs.

geoffw0

Looks great. We have a lot of follow-up work to do on models!

Also thanks for updating the QL to use the new predicates (MethodDecl).hasQualifiedName and FreeFunctionDecl.

geoffw0 · 2022-11-08T12:36:17Z


-  override predicate isSanitizer(DataFlow::Node node) {
-    none() // TODO: A conversion to a primitive type or an enum
+  // TODO: convert to new taint flow models


I think we should aim to create follow-up tasks for most of these things rather than addressing them all now. Many are planned already. And I don't want this PR just growing and growing.

Makes sense. Though it's a tradeoff between a growing PR and shipping more tech debt. An alternative is that I move these additional taint steps into the library, and create issues to flesh the library out to handle other cases that I didn't handle here.

That sounds fine as long as its straightforward.

I have a preference for smaller, faster PRs where possible as they're easier for everyone to review and manage.

github-actions · 2022-11-09T12:12:41Z

QHelp previews:

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

JavaScript Injection

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.

Recommendation

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 WKWebView.callAsyncJavaScript with the arguments dictionary to pass remote data objects.

Example

In the following (bad) example, a call to WKWebView.evaluateJavaScript evaluates JavaScript source code that is tainted with remote data, potentially introducing a code injection vulnerability.

let webview: WKWebView
let remoteData = try String(contentsOf: URL(string: "http://example.com/evil.json")!)

...

_ = try await webview.evaluateJavaScript("console.log(" + remoteData + ")") // BAD

In the following (good) example, we sanitize the remote data by passing it using the arguments dictionary of WKWebView.callAsyncJavaScript. This ensures that untrusted data cannot be evaluated as JavaScript source code.

let webview: WKWebView
let remoteData = try String(contentsOf: URL(string: "http://example.com/evil.json")!)

...

_ = try await webview.callAsyncJavaScript(
    "console.log(data)",
    arguments: ["data": remoteData], // GOOD
    contentWorld: .page
)

References

Apple Developer Documentation: WKWebView.callAsyncJavaScript(_:arguments:in:contentWorld:)
Common Weakness Enumeration: CWE-94.
Common Weakness Enumeration: CWE-95.
Common Weakness Enumeration: CWE-749.

geoffw0

Otherwise I think we're just waiting for docs review. The DCA run looks fine I think.

geoffw0 · 2022-11-15T18:44:01Z

+		let remoteString = getRemoteData()
+
+		try! await sink(localString) // GOOD: the HTML data is local
+		try! await sink(getRemoteData()) // BAD [NOT DETECTED - TODO: extract Callables of @MainActor method calls]: HTML contains remote input, may access local secrets


I wonder if the results of this test might be easier to interpret if we replace the calls to getRemoteData with an in-place taint source such as:

try String(contentsOf: URL(string: "http://example.com/")!)

Then the sources + sinks on the result lines should show us all of the combinations are working?

Weirdly enough, they are harder to interpret (see the latest commit). Now there is a separate result for this new in-place source (as expected). But unexpectedly, for the JSEvaluateScript sinks, there are two separate paths for this result: one passing through the JSString constructor function, one omitting this taint step.

for the JSEvaluateScript sinks, there are two separate paths for this result: one passing through the JSString constructor function, one omitting this taint step.

I think I see what you mean, there's an unnecessary edge:

| UnsafeJsEval.swift:287:31:287:97 | call to JSStringCreateWithCharacters(_:_:) : | UnsafeJsEval.swift:291:17:291:17 | jsstr |

and also:

| UnsafeJsEval.swift:286:51:286:51 | stringBytes : | UnsafeJsEval.swift:291:17:291:17 | jsstr |

But they don't appear to be new in this commit.

The new results under #select look good to me. You will need to replace the other calls to getRemoteData with inline sinks to see the complete grid of results there.

Notably both unnecessary edges are the transitive closure of more than one isAdditionalTaintStep step, ending at the sink. I wonder if this is a bug in taint flow itself?

The dataflow library will "compress" both dataflow and taint-flow steps before reporting them as edges in the edges relation. So we can't really conclude that there's a bug based on the edges data in the .expected file.

If you want to disable this compression, you can replace none() with any() in the charpred for CastNode here. Then each local step will be visible in the edges relation.

Are you saying the additional edges are expected / harmless?

Not quite. I'm saying that the transitive closure of more than one isAdditionalTaintStep is expected (this happens all the time in dataflow/taintflow). But this shouldn't produce additional spurious edges. The presence of additional edges might indicate an unnecessary taintflow/dataflow rule somewhere.

I checked over the rules in isAdditionalTaintStep for this query again and they all seem reasonable. I'm not sure what else could be generating edges through this - JSString* isn't modelled yet so it should be nothing.

Feel free to create an issue for it. I don't think this should block this PR.

sabrowning1

@d10c hello from Docs! 👋🏼 Thanks for your work on the documentation; I made some small suggestions to bring things in line with our style guide. Let me know if you have any questions!

Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com>

Strangely, there are two separate paths to each of the JSEvaluateScript sinks: one passing through the JSString constructor, one omitting this step.

sabrowning1

Looks good for Docs, thanks for making those changes @d10c! 🎉

geoffw0 · 2022-11-18T16:26:51Z

+	let remoteString = getRemoteData()
+
+	sink(localString) // GOOD: the HTML data is local
+	sink(try! String(contentsOf: URL(string: "http://example.com/")!)) // BAD: HTML contains remote input, may access local secrets


Do you plan to change all of the cases in testSync / testAsync work this way? I still think it will be easier to read the results like that - though if you feel otherwise I don't want us to get stuck on this.

d10c

Last commit LGTM 👍🏻

github-actions Bot added the Swift label Oct 26, 2022

geoffw0 reviewed Oct 31, 2022

View reviewed changes

Comment thread .vscode/settings.json Outdated

Comment thread swift/ql/src/queries/Security/CWE-094/UnsafeJsEval.ql

Comment thread swift/ql/src/queries/Security/CWE-094/UnsafeJsEval.ql

Comment thread swift/ql/src/queries/Security/CWE-094/UnsafeJsEval.ql Outdated

geoffw0 reviewed Nov 2, 2022

View reviewed changes

d10c added 4 commits November 3, 2022 11:16

Swift: Initial UnsafeJsEval query

5c905c4

Swift: Add async varant of WKWebView evaluateJavaScript(_:)

7b599f5

See concurrency note here: https://developer.apple.com/documentation/webkit/wkwebview/1415017-evaluatejavascript See also https://developer.apple.com/documentation/swift/calling-objective-c-apis-asynchronously

Swift: UnsafeJsEval test finally compiles

28b7f08

Swift: enable VSCode to build extractor via CMake

3d24e0a

The `-arch=x86_64` from `swift/rules.bzl` turns out to be unnecessary, even on Arm-based Macs.

d10c force-pushed the swift/js-injection branch from c3cd871 to 3d24e0a Compare November 3, 2022 10:19

d10c added 3 commits November 3, 2022 16:14

Swift: use FreeFunctionDecl/.has(Qualified)Name

fdd7d76

Instead of hand-rolled predicates.

Swift: _ as in _ = ... is a CFG leaf node.

7c515bb

This enables DataFlow to skip over it and not get stuck.

Swift: sync tests pass with additional flow steps

66291d3

TODO: Convert those flow steps to taint flow models in the library.

github-advanced-security AI found potential problems Nov 8, 2022

View reviewed changes

Comment thread swift/ql/src/queries/Security/CWE-094/UnsafeJsEval.ql Fixed

d10c added 2 commits November 8, 2022 11:24

Swift: basic Data-related taint flow in query

d37ed02

Still TODO: a more comprehensive taint flow model for Data in the libs.

Merge branch 'main' into swift/js-injection

7585541

geoffw0 reviewed Nov 8, 2022

View reviewed changes

Swift: Docs + doctests

5940f17

github-actions Bot added the documentation label Nov 9, 2022

d10c marked this pull request as ready for review November 9, 2022 12:21

d10c requested review from a team as code owners November 9, 2022 12:21

d10c added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Nov 9, 2022

geoffw0 reviewed Nov 9, 2022

View reviewed changes

Comment thread swift/ql/src/queries/Security/CWE-094/UnsafeJsEvalGood.swift Outdated

d10c added 2 commits November 11, 2022 12:23

Merge branch 'main' into swift/js-injection

4b7a89e

Swift: update doctests

16ba5b1

geoffw0 reviewed Nov 15, 2022

View reviewed changes

sabrowning1 reviewed Nov 15, 2022

View reviewed changes

d10c and others added 3 commits November 15, 2022 21:14

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

b42482c

Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com>

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

8db8f14

Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com>

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

cb7d9d5

Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com>

d10c and others added 3 commits November 15, 2022 21:14

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

fccb581

Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com>

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

52e5d54

Co-authored-by: Sam Browning <106113886+sabrowning1@users.noreply.github.com>

Swift: Add direct call to remote source to a test

09b669a

Strangely, there are two separate paths to each of the JSEvaluateScript sinks: one passing through the JSString constructor, one omitting this step.

sabrowning1 previously approved these changes Nov 16, 2022

View reviewed changes

Swift: update @security-severity

8b33277

d10c dismissed sabrowning1’s stale review via 8b33277 November 17, 2022 17:08

Merge branch 'main' into swift/js-injection

8f5af3f

geoffw0 reviewed Nov 18, 2022

View reviewed changes

d10c and others added 2 commits November 21, 2022 12:15

Merge branch 'main' into swift/js-injection

61de07e

Swift: Test .expected changes resulting from merge.

ef837f7

geoffw0 requested a review from a team as a code owner November 23, 2022 14:58

d10c commented Nov 23, 2022

View reviewed changes

geoffw0 approved these changes Nov 23, 2022

View reviewed changes

d10c merged commit 8f065e9 into github:main Nov 24, 2022

atorralba mentioned this pull request Nov 24, 2022

Swift: Add taint models for the Data class #11345

Merged

Conversation

d10c commented Oct 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JavaScript Injection

Recommendation

Example

References

Uh oh!

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sabrowning1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sabrowning1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d10c left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

github-actions Bot commented Nov 9, 2022 •

edited

Loading

geoffw0 Nov 16, 2022 •

edited

Loading

d10c left a comment •

edited

Loading