JavaScript escaping in loadUrl can be bypassed using percent-encoding

[marazmarci]

If you pass a malicious crafted URL containing percent-encoded characters to loadUrl(String url, boolean escapeJavascript), injecting JavaScript code becomes possible, despite the protective measures implemented in the library.

Since WebView::loadUrl expects a URI, it uses percent-encoding (URL encoding) to decode the URI, which makes it possible to add characters to the payload that would normally be escaped by StringEscapeUtils::escapeEcmaScript. The " character becomes %22 after URL encoding. If you use this instead of quotes, you can bypass the escaping.

See my PoC (branch):

JavaScriptInjectionFragment.kt:

private const val MALICIOUS_PARAMETER = "alert(%22Hello World%22)" // instead of "alert(\"Hello World\")"
private const val INJECTED_JAVASCRIPT = "javascript:void($MALICIOUS_PARAMETER)"

For this specific case, a possible fix could be to replace all occurrences of the % character with %25, but I think that would break lots of other legitimate use cases, like when the URI doesn't start with javascript:, for example: https://www.google.com/search?q=I%27m+blue

Escaping the whole javascript:method("param") string is also problematic, as it may not even contain user input (example: loadUrl("""javascript:alert("hello"))"""). Only the user input should be escaped, but currently, that could only be the caller's responsibility, since the SecureWebView library can't know which parts of the JS string are untrusted user inputs, and which were written by the developer.

Let's see some examples:

• This is obviously vulnerable to JS injection: loadUrl("""javascript:alert("hello" + "$name")""")
• This is still vulnerable to JS injection, because name can contain percent-encoded chars: loadUrl("""javascript:alert("hello" + "${StringEscapeUtils.escapeEcmaScript(name)}")""")
• This is the current implementation, which is still vulnerable to JS injection, because name can contain percent-encoded chars:
  • loadUrl(StringEscapeUtils.escapeEcmaScript("""javascript:alert("hello" + "$name")""")
  • But it breaks legitimate* JavaScript calls: loadUrl(StringEscapeUtils.escapeEcmaScript("""javascript:alert("hello")"""), which will result in this exact String passed to loadUrl: javascript:alert(\"hello\"). It won't run, because \" causes a JS syntax error.

*: I think legitimate JavaScript calls should always happen through WebView::evaluateJavascript. I'm not aware of any reason why should loadUrl("javascript:...") be used instead of evaluateJavascript, besides API 19 minSdk support, but fortunately the library's minSdk version is 21. Based on these, I suggest prohibiting passing URIs starting with javascript: to loadUrl, and instead developing protective measures around the evaluateJavascript method.

I think the library could provide two things to prevent misusages of evaluateJavascript: 1.) Education in documentation or doc. comments :smile: 2.) Idea: a JavaScript method call builder, where the caller provides the method's name and the list of parameters to be passed, which all will be automatically escaped one by one. We could modify the evaluateJavascript method to receive a JavaScriptCall instance built by this builder. Also, we could add an evaluateJavascriptUnsafe method which replicates the behavior of the original evaluateJavascript method.

WDYT? :slightly_smiling_face:

:potato:

[balazsgerlei]

Thanks for the detailed issue, it's really awesome. And yeah, I knew about this issue and to be frank the "JavaScript Injection" example in the sample app and the accompanying feature really needs work, it suffers from me wanting to demonstrate and solve different things: I wanted to highlight the importance of sanitizing inputs in JavaScript and also disallowing running JavaScript from URI in loadUrl. BTW disallowing javascript: prefix would not be enough for the latter, data URLs can also be used to run JavaScript via loadUrl, for example:

data:text/html,<script>alert('hi');</script>

They should be also either blocked entirely, or at least the usage of the <script> tag.

Ultimately I think I'll rework the example and the library to just disallow running JavaScript via loadUrl by blocking the javascript: prefix and also disallow loading data URLs (by default) as there is the loadData method for that, and I won't go into dealing with injection via parameters for this library (at least for now).