Java: Unsafe resource loading in Android webview

[luchua-bc]

Android WebViews that allow loading URLs controlled by external inputs and whose JavaScript interface is enabled are potentially vulnerable to cross-site scripting and sensitive resource disclosure attacks.

In addition, WebViews whose settings have setAllowFileAccessFromFileURLs or setAllowUniversalAccessFromFileURLs enabled must not load any untrusted web content since malicious scripts can launch cross-site scripting attacks, either accessing arbitrary local files including WebView cookies, session tokens, and private app data or even credentials used on arbitrary web sites.

This query detects the following two scenarios:

• Vulnerability introduced by WebViews with JavaScript enabled and remote inputs allowed
• Severe vulnerabilities when allowing cross-origin resource access is also enabled

Please consider to merge the PR. Thanks.

[smowton]

Hello, sorry for the delay! Quite a few comments I'm afraid but they should all be quite quick and easy to apply.

[smowton]

Thanks for your fast response -- a few more comments to look at, but this is looking good

[smowton]

Regarding the text about two vulnerabilities, let me check my understanding:

"Vulnerability introduced by WebViews with JavaScript enabled and remote inputs allowed" I think refers to the situation where (a) the URL might be controlled by an attacker and (b) Javascript is switched on, but neither of setAllow{File,Universal}AccessFromFileURLs are set?

Then "High precision vulnerability when allowing universal resource access is also enabled" refers to the situation where additionally one of setAllow{File,Universal}AccessFromFileURLs is set?

If so then I suggest "universal" -> "cross-origin" since "Universal" is just one of the two access settings we care about, and "High precision" -> "A more severe", since we have the same imprecision level but the consequences are worse.

[smowton]

This incorporates my suggested reword (please apply it elsewhere if this text is duplicated), plus avoids the CodeQL help weighing in on Droid vs. iPhone :)

[smowton]

Looks good! Let's sort out the doc comments, then good to merge

[smowton]

BTW don't worry about the failing code scanning job, there was an outage recently and it should clear on your next push. Rebase on latest main to be sure we're running the latest version of the tests.

[luchua-bc]

Thanks a lot for all the help from @smowton and @aschackmull with this PR.

@luchua-bc

[smowton]

@luchua-bc please rebase the branch on latest main to (probably) fix CI

[smowton]

This still needs a rebase:

git fetch https://github.com/github/codeql.git main
git rebase FETCH_HEAD

[luchua-bc]

Thanks @smowton. Probably the issue was caused by that I haven't merged with master for a long time. I will refresh my copy.

[smowton]

1. master on this (and other Github repos) has been renamed to main
2. A rebase is usually preferred to a merge for a PR because it produces a better Git history -- the merge will create a merge commit in the PR showing main's changes being brought in, whereas a rebase replays your commits on top of main as it stands today, similar to git cherry-pick if you've used that.

In any case, you can learn about that in your own time -- this will land in #4483.

[smowton]

Merged as #4483