Skip to content

Commit c0d76da

Browse files
aschackmullaschackmull
authored
Merge pull request github#5846 from atorralba/atorralba/promote-unsafe-android-webview-fetch
Java: Promote Unsafe resource loading in Android WebView from experimental
2 parents fb9feab + 4435853 commit c0d76da

35 files changed

+1203
-233
lines changed

‎java/change-notes/2021-05-06-unsafe-android-access-query.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* The query "Unsafe resource fetching in Android WebView" (`java/android/unsafe-android-webview-fetch`) has been promoted from experimental to the main query pack. Its results will now appear by default. This query was originally [submitted as an experimental query by @luchua-bc](https://github.com/github/codeql/pull/3706).

‎java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.java renamed to ‎java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.java

File renamed without changes.

‎java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
2+
<qhelp>
3+
4+
<overview>
5+
<p>Android WebViews that allow externally controlled URLs to be loaded, and whose JavaScript interface is enabled, are potentially vulnerable to cross-site scripting and sensitive resource disclosure attacks.</p>
6+
<p>A <code>WebView</code> whose <code>WebSettings</code> object has called <code>setAllowFileAccessFromFileURLs(true)</code> or <code>setAllowUniversalAccessFromFileURLs(true)</code> must not load any untrusted web content.</p>
7+
<p>Enabling these settings allows malicious scripts loaded in a <code>file://</code> context to launch cross-site scripting attacks, accessing arbitrary local files including WebView cookies, session tokens, private app data or even credentials used on arbitrary web sites.</p>
8+
<p>This query detects the following two scenarios:</p>
9+
<ol>
10+
<li>A vulnerability introduced by WebViews when JavaScript is enabled and remote inputs are allowed.</li>
11+
<li>A more severe vulnerability when "allow cross-origin resource access" is also enabled. This setting was deprecated in API level 30 (Android 11), but most devices are still affected, especially since some Android phones are updated slowly or no longer updated at all.</li>
12+
</ol>
13+
</overview>
14+
15+
<recommendation>
16+
<p>Only allow trusted web content to be displayed in WebViews when JavaScript is enabled. Disallow cross-origin resource access in WebSettings to reduce the attack surface.</p>
17+
</recommendation>
18+
19+
<example>
20+
<p>The following example shows both 'BAD' and 'GOOD' configurations. In the 'BAD' configuration, JavaScript and the allow access setting are enabled and 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>
21+
<sample src="UnsafeAndroidAccess.java" />
22+
</example>
23+
24+
<references>
25+
<li>
26+
Google Help: <a href="https://support.google.com/faqs/answer/7668153?hl=en">Fixing a File-based XSS Vulnerability</a>
27+
</li>
28+
<li>
29+
OWASP: <a href="https://github.com/OWASP/owasp-mstg/blob/master/Document/0x05h-Testing-Platform-Interaction.md#testing-javascript-execution-in-webviews-mstg-platform-5">Testing JavaScript Execution in WebViews (MSTG-PLATFORM-5)</a>
30+
</li>
31+
<li>
32+
OWASP: <a href="https://github.com/OWASP/owasp-mstg/blob/master/Document/0x05h-Testing-Platform-Interaction.md#testing-webview-protocol-handlers-mstg-platform-6">Testing WebView Protocol Handlers (MSTG-PLATFORM-6)</a>
33+
</li>
34+
</references>
35+
</qhelp>

‎java/ql/src/Security/CWE/CWE-749/UnsafeAndroidAccess.ql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* @name Unsafe resource fetching in Android WebView
3+
* @description JavaScript rendered inside WebViews can access protected
4+
* application files and web resources from any origin exposing them to attack.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @precision medium
8+
* @id java/android/unsafe-android-webview-fetch
9+
* @tags security
10+
* external/cwe/cwe-749
11+
* external/cwe/cwe-079
12+
*/
13+
14+
import java
15+
import semmle.code.java.security.UnsafeAndroidAccessQuery
16+
import DataFlow::PathGraph
17+
18+
from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf
19+
where conf.hasFlowPath(source, sink)
20+
select sink.getNode(), source, sink, "Unsafe resource fetching in Android WebView due to $@.",
21+
source.getNode(), sink.getNode().(UrlResourceSink).getSinkType()

‎java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.qhelp

Lines changed: 0 additions & 31 deletions
This file was deleted.

‎java/ql/src/experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql

Lines changed: 0 additions & 130 deletions
This file was deleted.

‎java/ql/src/semmle/code/java/frameworks/android/XssSinks.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@ private class DefaultXssSinkModel extends SinkModelCsv {
99
row =
1010
[
1111
"android.webkit;WebView;false;loadData;;;Argument[0];xss",
12-
"android.webkit;WebView;false;loadUrl;;;Argument[0];xss",
13-
"android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss"
12+
"android.webkit;WebView;false;loadDataWithBaseURL;;;Argument[1];xss",
13+
"android.webkit;WebView;false;evaluateJavascript;;;Argument[0];xss"
1414
]
1515
}
1616
}

‎java/ql/src/semmle/code/java/security/UnsafeAndroidAccess.qll

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
/**
2+
* Provides classes to reason about Unsafe Resource Fetching vulnerabilities in Android.
3+
*/
4+
5+
import java
6+
private import semmle.code.java.frameworks.android.WebView
7+
private import semmle.code.java.dataflow.DataFlow
8+
private import semmle.code.java.dataflow.ExternalFlow
9+
10+
/**
11+
* A sink that represents a method that fetches a web resource in Android.
12+
*
13+
* Extend this class to add your own Unsafe Resource Fetching sinks.
14+
*/
15+
abstract class UrlResourceSink extends DataFlow::Node {
16+
/**
17+
* Gets a description of this vulnerability.
18+
*/
19+
abstract string getSinkType();
20+
}
21+
22+
/**
23+
* A cross-origin access enabled resource fetch.
24+
*
25+
* Only considered a valid sink when JavaScript is also enabled.
26+
*/
27+
private class CrossOriginUrlResourceSink extends JavaScriptEnabledUrlResourceSink {
28+
CrossOriginUrlResourceSink() {
29+
exists(Variable settings, MethodAccess ma |
30+
webViewLoadUrl(this.asExpr(), settings) and
31+
ma.getMethod() instanceof CrossOriginAccessMethod and
32+
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and
33+
ma.getQualifier() = settings.getAnAccess()
34+
)
35+
}
36+
37+
override string getSinkType() {
38+
result = "user input vulnerable to cross-origin and sensitive resource disclosure attacks"
39+
}
40+
}
41+
42+
/**
43+
* JavaScript enabled resource fetch.
44+
*/
45+
private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink {
46+
JavaScriptEnabledUrlResourceSink() {
47+
exists(Variable settings |
48+
webViewLoadUrl(this.asExpr(), settings) and
49+
isJSEnabled(settings)
50+
)
51+
}
52+
53+
override string getSinkType() { result = "user input vulnerable to XSS attacks" }
54+
}
55+
56+
/**
57+
* Holds if a `WebViewLoadUrlMethod` method is called with the given `urlArg` on a
58+
* WebView with settings stored in `settings`.
59+
*/
60+
private predicate webViewLoadUrl(Expr urlArg, Variable settings) {
61+
exists(MethodAccess loadUrl, Variable webview, MethodAccess getSettings |
62+
loadUrl.getArgument(0) = urlArg and
63+
loadUrl.getMethod() instanceof WebViewLoadUrlMethod and
64+
loadUrl.getQualifier() = webview.getAnAccess() and
65+
getSettings.getMethod() instanceof WebViewGetSettingsMethod and
66+
webview.getAnAccess() = getSettings.getQualifier() and
67+
settings.getAnAssignedValue() = getSettings
68+
)
69+
}
70+
71+
/**
72+
* A method allowing any-local-file and cross-origin access in the WebSettings class.
73+
*/
74+
private class CrossOriginAccessMethod extends Method {
75+
CrossOriginAccessMethod() {
76+
this.getDeclaringType() instanceof TypeWebSettings and
77+
this.hasName(["setAllowUniversalAccessFromFileURLs", "setAllowFileAccessFromFileURLs"])
78+
}
79+
}
80+
81+
/**
82+
* The `setJavaScriptEnabled` method for the webview.
83+
*/
84+
private class AllowJavaScriptMethod extends Method {
85+
AllowJavaScriptMethod() {
86+
this.getDeclaringType() instanceof TypeWebSettings and
87+
this.hasName("setJavaScriptEnabled")
88+
}
89+
}
90+
91+
/**
92+
* Holds if a call to `v.setJavaScriptEnabled(true)` exists.
93+
*/
94+
private predicate isJSEnabled(Variable v) {
95+
exists(MethodAccess jsa |
96+
v.getAnAccess() = jsa.getQualifier() and
97+
jsa.getMethod() instanceof AllowJavaScriptMethod and
98+
jsa.getArgument(0).(BooleanLiteral).getBooleanValue() = true
99+
)
100+
}

‎java/ql/src/semmle/code/java/security/UnsafeAndroidAccessQuery.qll

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/** Provides taint tracking configurations to be used in Unsafe Resource Fetching queries. */
2+
3+
import java
4+
import semmle.code.java.dataflow.FlowSources
5+
import semmle.code.java.dataflow.TaintTracking
6+
import semmle.code.java.security.RequestForgery
7+
import semmle.code.java.security.UnsafeAndroidAccess
8+
9+
/**
10+
* A taint configuration tracking flow from untrusted inputs to a resource fetching call.
11+
*/
12+
class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration {
13+
FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" }
14+
15+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
16+
17+
override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink }
18+
19+
override predicate isSanitizer(DataFlow::Node sanitizer) {
20+
sanitizer instanceof RequestForgerySanitizer
21+
}
22+
}

‎java/ql/test/experimental/query-tests/security/CWE-749/UnsafeAndroidAccess.expected

Lines changed: 0 additions & 31 deletions
This file was deleted.

0 commit comments

Comments
 (0)