Swift: WebView JS-native bridge sources #11027
Conversation
This is what contains externally-provided data in Webview JS-native bridges
5557744
to
5ef8f9c
Compare
Better model the JSExport protocol logic
5ef8f9c
to
baf7986
Compare
| [ | ||
| ";JSContext;true;globalObject;;;;remote", | ||
| ";JSContext;true;objectAtIndexedSubscript(_:);;;ReturnValue;remote", | ||
| ";JSContext;true;objectForKeyedSubscript(_:);;;ReturnValue;remote" |
There was a problem hiding this comment.
Should these things also be taint sources?
- the value of
JSContext.exception - the second input to
JSContext.exceptionHandler? - the return value from both variants of
evaluateScript?
There was a problem hiding this comment.
the value of
JSContext.exception
JSContext.exception is an exception that gets thrown on the JavaScript side once the native code execution has finished. So I don't think JavaScript can use that field to execute native code.
the second input to
JSContext.exceptionHandler?
The exceptionHandler is a block that executes native code indeed, but I think it can't be set from the JavaScript side. The JSValue that is the second argument is the exception that got thrown in a native code callback, which again doesn't seem to be JS-provided nor should affect the native code — it's targeted to JavaScript so that the exception can be handled there (if I understood all this correctly).
the return value from both variants of
evaluateScript?
That indeed comes for JavaScript. The thing is that the JavaScript being executed in this case comes from the app itself, not the website visited in the WebView. So unless the existing JavaScript could somehow tamper the code passed to evaluateScript, its return value wouldn't be under the malicious site's control. Of course controlling the script of the evaluateScript call itself would be a problem, but that's more akin to a XSS vulnerability.
| @@ -574,6 +574,14 @@ private module PostUpdateNodes { | |||
|
|
|||
| override DataFlowCallable getEnclosingCallable() { result = TDataFlowFunc(n.getScope()) } | |||
| } | |||
|
|
|||
| class SummaryPostUpdateNode extends SummaryNode, PostUpdateNodeImpl { | |||
There was a problem hiding this comment.
Please could you explain the purpose of this class?
There was a problem hiding this comment.
This was a missing piece in the data flow library to catch cases like the this: https://github.com/github/codeql/pull/11027/files#diff-7e1f3d77d83b67247f8f21045e82539532beff203bfccf85a0080314064f3ffaR89
Basically, if a summary introduced a step involving updating the taint of a node (like arg -> qualifier flow), we weren't propagating flow correctly through it because of this missing node.
| var tainted: Any { get } | ||
| func tainted(arg1: Any, arg2: Any) | ||
| } | ||
| class ExportedImpl : Exported { | ||
| var tainted: Any { get { return "" } } // SOURCE |
There was a problem hiding this comment.
We don't appear to catch this one.
There was a problem hiding this comment.
Good catch, there was an issue in how the source was modeled around fields. Fixed in b62ede1, thanks!
| } | ||
| /** A protocol inheriting `JSExport`. */ | ||
| private class JsExportedProto extends ProtocolDecl { | ||
| JsExportedProto() { this.getABaseTypeDecl+() instanceof JsExport } |
There was a problem hiding this comment.
Why is this not getABaseTypeDecl*()? Can JSExport not be applied directly?
There was a problem hiding this comment.
Applying JSExport directly would theoretically do nothing. The expected use is extending the protocol in another protocol which declares the members you want to export to JavaScript. Then classes adopting the protocol will export those members only (by default, if JSExport isn't used, no members are exported).
| adopter.getEnclosingDecl() instanceof JsExportedType | ||
| | | ||
| this.(DataFlow::ParameterNode).getParameter().getDeclaringFunction() = adopter and | ||
| adopter.getName() = base.getName() |
There was a problem hiding this comment.
What is the purpose of this check? Is this because some things in the class can't be accessed by JS?
There was a problem hiding this comment.
Yes, this is to reflect that only the members declared in the protocol are exported to JavaScript, not all members of the adopting class/struct. I couldn't find a better way to model member inheritance, happy to improve this if there's a better alternative.
| class JSContext { | ||
| var globalObject: JSValue { get { return JSValue() } } | ||
| func objectForKeyedSubscript(_: Any!) -> JSValue! { return JSValue() } | ||
| func setObject(_: Any, forKeyedSubscript: (NSCopying & NSObjectProtocol) ) {} |
There was a problem hiding this comment.
So to make sure I understand: in an early version you were looking for types that are handed to setObject, because JS can potentially taint them or anything contained in them. But now you look for all types with the JSExport prototype because presumably that's required to do this, and is more accurate?
There was a problem hiding this comment.
Yes, I first thought that an object exposed through setObject would automatically export all its members to JS, but turns out it doesn't. You must declare explicitly, through a protocol that extends JSExport, the members that need to be exported. All the other members remain unaccessible from JavaScript.
And I thought that modeling the JSExport logic was enough. Theoretically the object would still need to be added to the JSContext, but I imagine that applications rarely would declare a type adopting a JSExport-inherited protocol and then not use it in the context. And still if they do, we probably want to alert if there's a dangerous path starting on them anyway, since it would be a ticking bomb waiting to explode when the object finally got added to the context.
But let me know if you think we should add the setObject constraint to the existing logic to be 100% accurate.
Model the source as an access to the tainted field, instead of the field itself (which didn't work)
Adds taint sources for JS-native bridges in WebViews. These are objects where JavaScript running in the WebView can execute native code, which can be exploited by malicious websites, or external attackers through XSS vulnerabilities, if it performs sensitive actions.