-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Java: Unsafe resource loading in Android webview #3706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
9ca1c3c
Unsafe resource loading in Android webview
luchua-bc 9030834
Text changes to the help file
luchua-bc 3b99083
Text changes to the help file
luchua-bc ce17360
Enhance the query and add more test cases
luchua-bc c3e5e0f
Fine tune the query
luchua-bc b0bf0db
Update the doc comments
luchua-bc 2d43c60
Change the source class type and simplify the data-flow step
luchua-bc bd451fe
Replace non-ASCII apostrophe in Java stub classes
luchua-bc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
83 changes: 83 additions & 0 deletions
83
java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| public class UnsafeAndroidAccess extends Activity { | ||
| public void onCreate(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(R.layout.webview); | ||
|
|
||
| // BAD: Have both JavaScript and cross-origin resource access enabled in webview while | ||
| // taking remote user inputs | ||
| { | ||
| WebView wv = (WebView) findViewById(R.id.my_webview); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| webSettings.setJavaScriptEnabled(true); | ||
| webSettings.setAllowUniversalAccessFromFileURLs(true); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| String thisUrl = getIntent().getExtras().getString("url"); // dangerous remote input from the intent's Bundle of extras | ||
| wv.loadUrl(thisUrl); | ||
| } | ||
|
|
||
| // BAD: Have both JavaScript and cross-origin resource access enabled in webview while | ||
| // taking remote user inputs | ||
| { | ||
| WebView wv = (WebView) findViewById(R.id.my_webview); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| webSettings.setJavaScriptEnabled(true); | ||
| webSettings.setAllowUniversalAccessFromFileURLs(true); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| String thisUrl = getIntent().getStringExtra("url"); //dangerous remote input from intent extra | ||
| wv.loadUrl(thisUrl); | ||
| } | ||
|
|
||
| // GOOD: Have JavaScript and cross-origin resource access disabled by default on modern Android (Jellybean+) while taking remote user inputs | ||
| { | ||
| WebView wv = (WebView) findViewById(-1); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| String thisUrl = getIntent().getExtras().getString("url"); // remote input | ||
| wv.loadUrl(thisUrl); | ||
| } | ||
|
|
||
| // GOOD: Have JavaScript enabled in webview but remote user input is not allowed | ||
| { | ||
| WebView wv = (WebView) findViewById(-1); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| webSettings.setJavaScriptEnabled(true); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| wv.loadUrl("https://www.mycorp.com"); | ||
| } | ||
| } | ||
| } | ||
31 changes: 31 additions & 0 deletions
31
java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| <!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd"> | ||
| <qhelp> | ||
|
|
||
| <overview> | ||
| <p>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.</p> | ||
| <p>A <code>WebView</code> whose <code>WebSettings</code> object has <code>setAllowFileAccessFromFileURLs(true)</code> or <code>setAllowUniversalAccessFromFileURLs(true)</code> called must not load any untrusted web content.</p> | ||
| <p>Enabling these settings allows malicious scripts loaded in a <code>file://</code> context to launch cross-site scripting attacks, either accessing arbitrary local files including WebView cookies, session tokens, private app data or even credentials used on arbitrary web sites.</p> | ||
| <p>This query detects the following two scenarios:</p> | ||
| <ol> | ||
| <li>Vulnerability introduced by WebViews with JavaScript enabled and remote inputs allowed.</li> | ||
| <li>A more severe vulnerability when allowing cross-origin resource access is also enabled. The setting was deprecated in API level 30 (Android 11), but most devices are still affected, especially given that some Android phones are updated slowly or no longer updated at all.</li> | ||
| </ol> | ||
| </overview> | ||
|
|
||
| <recommendation> | ||
| <p>Only allow trusted web content to be displayed in WebViews when JavaScript is enabled. Disallow cross-origin resource access in WebSetting to reduce the attack surface.</p> | ||
| </recommendation> | ||
|
|
||
| <example> | ||
| <p>The following example shows both 'BAD' and 'GOOD' configurations. In the 'BAD' configuration, setting is enabled and JavaScript is enabled while URLs are loaded from externally controlled inputs. In the 'GOOD' configuration, JavaScript is disabled or only trusted web content is allowed to be loaded.</p> | ||
| <sample src="UnsafeAndroidAccess.java" /> | ||
| </example> | ||
|
|
||
| <references> | ||
| <li> | ||
| <a href="https://cwe.mitre.org/data/definitions/749.html">CWE-749</a> | ||
| <a href="https://support.google.com/faqs/answer/7668153?hl=en">Fixing a File-based XSS Vulnerability</a> | ||
| <a href="https://github.com/OWASP/owasp-mstg/blob/master/Document/0x05h-Testing-Platform-Interaction.md">OWASP - Testing WebView Protocol Handlers (MSTG-PLATFORM-5 and MSTG-PLATFORM-6)</a> | ||
| </li> | ||
| </references> | ||
| </qhelp> |
146 changes: 146 additions & 0 deletions
146
java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| /** | ||
| * @name Unsafe resource fetching in Android webview | ||
| * @description JavaScript rendered inside WebViews can access any protected application file and web resource from any origin | ||
| * @kind path-problem | ||
| * @tags security | ||
| * external/cwe/cwe-749 | ||
| * external/cwe/cwe-079 | ||
| */ | ||
|
|
||
| import java | ||
| import semmle.code.java.frameworks.android.Intent | ||
| import semmle.code.java.frameworks.android.WebView | ||
| import semmle.code.java.dataflow.FlowSources | ||
|
|
||
| /** | ||
| * Methods allowing any-local-file and cross-origin access in the WebSettings class | ||
| */ | ||
| class CrossOriginAccessMethod extends Method { | ||
| CrossOriginAccessMethod() { | ||
| this.getDeclaringType() instanceof TypeWebSettings and | ||
| ( | ||
| this.hasName("setAllowUniversalAccessFromFileURLs") or | ||
| this.hasName("setAllowFileAccessFromFileURLs") | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * `setJavaScriptEnabled` method for the webview | ||
| */ | ||
| class AllowJavaScriptMethod extends Method { | ||
| AllowJavaScriptMethod() { | ||
| this.getDeclaringType() instanceof TypeWebSettings and | ||
| this.hasName("setJavaScriptEnabled") | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Holds if a call to `v.setJavaScriptEnabled(true)` exists | ||
| */ | ||
| predicate isJSEnabled(Variable v) { | ||
| exists(MethodAccess jsa | | ||
| v.getAnAccess() = jsa.getQualifier() and | ||
| jsa.getMethod() instanceof AllowJavaScriptMethod and | ||
| jsa.getArgument(0).(BooleanLiteral).getBooleanValue() = true | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Fetch URL method call on the `android.webkit.WebView` object | ||
| */ | ||
| class FetchResourceMethodAccess extends MethodAccess { | ||
| FetchResourceMethodAccess() { | ||
| this.getMethod().getDeclaringType() instanceof TypeWebView and | ||
| this.getMethod().hasName(["loadUrl", "postUrl"]) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Method access to external inputs of `android.content.Intent` object | ||
| */ | ||
| class IntentGetExtraMethodAccess extends MethodAccess { | ||
| IntentGetExtraMethodAccess() { | ||
| this.getMethod().getName().regexpMatch("get\\w+Extra") and | ||
| this.getMethod().getDeclaringType() instanceof TypeIntent | ||
| or | ||
| this.getMethod().getName().regexpMatch("get\\w+") and | ||
| this.getQualifier().(MethodAccess).getMethod().hasName("getExtras") and | ||
| this.getQualifier().(MethodAccess).getMethod().getDeclaringType() instanceof TypeIntent | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Source of fetching URLs from intent extras | ||
| */ | ||
| class UntrustedResourceSource extends DataFlow::ExprNode { | ||
| UntrustedResourceSource() { this.asExpr() instanceof IntentGetExtraMethodAccess } | ||
| } | ||
|
|
||
| /** | ||
| * Holds if `ma` loads URL `sink` | ||
| */ | ||
| predicate fetchResource(FetchResourceMethodAccess ma, Expr sink) { sink = ma.getArgument(0) } | ||
|
|
||
| /** | ||
| * A URL argument to a `loadUrl` or `postUrl` call, considered as a sink. | ||
| */ | ||
| class UrlResourceSink extends DataFlow::ExprNode { | ||
| UrlResourceSink() { fetchResource(_, this.getExpr()) } | ||
|
|
||
| /** Gets the fetch method that fetches this sink URL. */ | ||
| FetchResourceMethodAccess getMethodAccess() { fetchResource(result, this.getExpr()) } | ||
luchua-bc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /** | ||
| * Holds if cross-origin access is enabled for this resource fetch. | ||
| * | ||
| * Specifically this looks for code like | ||
| * `webView.getSettings().setAllow[File|Universal]AccessFromFileURLs(true);` | ||
| */ | ||
| predicate crossOriginAccessEnabled() { | ||
luchua-bc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| exists(MethodAccess ma, MethodAccess getSettingsMa | | ||
| ma.getMethod() instanceof CrossOriginAccessMethod and | ||
| ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and | ||
| ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and | ||
| getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and | ||
| getSettingsMa.getQualifier().(VarAccess).getVariable().getAnAccess() = | ||
| getMethodAccess().getQualifier() | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Returns a description of this vulnerability, assuming Javascript is enabled and | ||
| * the fetched URL is attacker-controlled. | ||
| */ | ||
| string getSinkType() { | ||
luchua-bc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if crossOriginAccessEnabled() | ||
| then result = "user input vulnerable to cross-origin and sensitive resource disclosure attacks" | ||
| else result = "user input vulnerable to XSS attacks" | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Taint configuration tracking flow from untrusted inputs to `loadUrl` or `postUrl` calls. | ||
| */ | ||
| class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration { | ||
luchua-bc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" } | ||
|
|
||
| override predicate isSource(DataFlow::Node source) { source instanceof UntrustedResourceSource } | ||
luchua-bc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| override predicate isSink(DataFlow::Node sink) { | ||
| sink instanceof UrlResourceSink and | ||
| exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v | | ||
| sink.(UrlResourceSink).getMethodAccess().getQualifier() = webviewVa and | ||
| getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and | ||
| webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and | ||
| v.getAnAssignedValue() = getSettingsMa and | ||
| isJSEnabled(v) | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf | ||
| where conf.hasFlowPath(source, sink) | ||
| select sink.getNode().(UrlResourceSink).getMethodAccess(), source, sink, | ||
| "Unsafe resource fetching in Android webview due to $@.", source.getNode(), | ||
| sink.getNode().(UrlResourceSink).getSinkType() | ||
3 changes: 3 additions & 0 deletions
3
java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.expected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| | UnsafeAndroidAccess.java:30:3:30:21 | loadUrl(...) | UnsafeAndroidAccess.java:29:20:29:59 | getString(...) : String | UnsafeAndroidAccess.java:30:14:30:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:29:20:29:59 | getString(...) | user input vulnerable to cross-origin and sensitive resource disclosure attacks | | ||
| | UnsafeAndroidAccess.java:53:3:53:21 | loadUrl(...) | UnsafeAndroidAccess.java:52:20:52:52 | getStringExtra(...) : String | UnsafeAndroidAccess.java:53:14:53:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:52:20:52:52 | getStringExtra(...) | user input vulnerable to cross-origin and sensitive resource disclosure attacks | | ||
| | UnsafeAndroidAccess.java:95:3:95:21 | loadUrl(...) | UnsafeAndroidAccess.java:94:20:94:52 | getStringExtra(...) : String | UnsafeAndroidAccess.java:95:14:95:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:94:20:94:52 | getStringExtra(...) | user input vulnerable to XSS attacks | |
119 changes: 119 additions & 0 deletions
119
java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| import android.app.Activity; | ||
|
|
||
| import android.os.Bundle; | ||
|
|
||
| import android.webkit.WebSettings; | ||
| import android.webkit.WebView; | ||
| import android.webkit.WebViewClient; | ||
|
|
||
| public class UnsafeAndroidAccess extends Activity { | ||
| //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras | ||
| public void testOnCreate1(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(-1); | ||
|
|
||
| WebView wv = (WebView) findViewById(-1); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| webSettings.setJavaScriptEnabled(true); | ||
| webSettings.setAllowFileAccessFromFileURLs(true); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| String thisUrl = getIntent().getExtras().getString("url"); | ||
| wv.loadUrl(thisUrl); | ||
| } | ||
|
|
||
| //Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from string extra | ||
| public void testOnCreate2(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(-1); | ||
|
|
||
| WebView wv = (WebView) findViewById(-1); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| webSettings.setJavaScriptEnabled(true); | ||
| webSettings.setAllowFileAccessFromFileURLs(true); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| String thisUrl = getIntent().getStringExtra("url"); | ||
| wv.loadUrl(thisUrl); | ||
| } | ||
|
|
||
| //Test onCreate with both JavaScript and cross-origin resource access disabled by default while taking remote user inputs | ||
| public void testOnCreate3(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(-1); | ||
|
|
||
| WebView wv = (WebView) findViewById(-1); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| String thisUrl = getIntent().getStringExtra("url"); | ||
| wv.loadUrl(thisUrl); | ||
| } | ||
|
|
||
| //Test onCreate with JavaScript enabled but cross-origin resource access disabled while taking remote user inputs | ||
| public void testOnCreate4(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(-1); | ||
|
|
||
| WebView wv = (WebView) findViewById(-1); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| webSettings.setJavaScriptEnabled(true); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| String thisUrl = getIntent().getStringExtra("url"); | ||
| wv.loadUrl(thisUrl); | ||
| } | ||
|
|
||
| //Test onCreate with both JavaScript and cross-origin resource access enabled while not taking remote user inputs | ||
| public void testOnCreate5(Bundle savedInstanceState) { | ||
| super.onCreate(savedInstanceState); | ||
| setContentView(-1); | ||
|
|
||
| WebView wv = (WebView) findViewById(-1); | ||
| WebSettings webSettings = wv.getSettings(); | ||
|
|
||
| webSettings.setJavaScriptEnabled(true); | ||
| webSettings.setAllowFileAccessFromFileURLs(true); | ||
|
|
||
| wv.setWebViewClient(new WebViewClient() { | ||
| @Override | ||
| public boolean shouldOverrideUrlLoading(WebView view, String url) { | ||
| view.loadUrl(url); | ||
| return true; | ||
| } | ||
| }); | ||
|
|
||
| wv.loadUrl("https://www.mycorp.com"); | ||
| } | ||
| } |
1 change: 1 addition & 0 deletions
1
java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.qlref
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| // semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/google-android-9.0.0 |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.