Skip to content

Commit 5338332

Browse files
luchua-bcsmowton
luchua-bc
authored and
smowton
committed
Enhance the query and add more test cases
1 parent 55af373 commit 5338332

File tree

6 files changed

+129
-49
lines changed

6 files changed

+129
-49
lines changed

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

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,18 +20,35 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
2020
}
2121
});
2222

23-
String thisUrl = getIntent().getExtras().getString("url"); // dangerous remote input
24-
//String thisUrl = getIntent().getStringExtra("url"); //An alternative way to load dangerous remote input
23+
String thisUrl = getIntent().getExtras().getString("url"); // dangerous remote input from the intent's Bundle of extras
2524
wv.loadUrl(thisUrl);
2625
}
2726

28-
// GOOD: Have JavaScript and universal resource access disabled while taking
29-
// remote user inputs
27+
// BAD: Have both JavaScript and universal resource access enabled in webview while
28+
// taking remote user inputs
3029
{
31-
WebView wv = (WebView) findViewById(-1);
30+
WebView wv = (WebView) findViewById(R.id.my_webview);
3231
WebSettings webSettings = wv.getSettings();
3332

34-
webSettings.setJavaScriptEnabled(false);
33+
webSettings.setJavaScriptEnabled(true);
34+
webSettings.setAllowUniversalAccessFromFileURLs(true);
35+
36+
wv.setWebViewClient(new WebViewClient() {
37+
@Override
38+
public boolean shouldOverrideUrlLoading(WebView view, String url) {
39+
view.loadUrl(url);
40+
return true;
41+
}
42+
});
43+
44+
String thisUrl = getIntent().getStringExtra("url"); //dangerous remote input from intent extra
45+
wv.loadUrl(thisUrl);
46+
}
47+
48+
// GOOD: Have JavaScript and universal resource access disabled by default on modern Android (Jellybean+) while taking remote user inputs
49+
{
50+
WebView wv = (WebView) findViewById(-1);
51+
WebSettings webSettings = wv.getSettings();
3552

3653
wv.setWebViewClient(new WebViewClient() {
3754
@Override

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
<qhelp>
33

44
<overview>
5-
<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>
6-
<p>WebSettings of WebViews with setAllowFileAccessFromFileURLs or setAllowUniversalAccessFromFileURLs enabled must not load any untrusted web content.</p>
7-
<p>Enabling this setting allows malicious scripts loaded in a file:// 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>
5+
<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>
6+
<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>
7+
<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>
88
<p>This query detects the following two scenarios:</p>
99
<ol>
1010
<li>Vulnerability introduced by WebViews with JavaScript enabled and remote inputs allowed.</li>
@@ -13,11 +13,11 @@
1313
</overview>
1414

1515
<recommendation>
16-
<p>Only allow trusted web contents to be displayed in WebViews when JavaScript is enabled. And disallow universal resource access in WebSettings to reduce the attack surface.</p>
16+
<p>Only allow trusted web content to be displayed in WebViews when JavaScript is enabled. Disallow universal resource access in WebSetting to reduce the attack surface .</p>
1717
</recommendation>
1818

1919
<example>
20-
<p>The following example shows both 'BAD' and 'GOOD' configurations. In the 'BAD' configuration, universal resource access 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 contents are allowed to be loaded.</p>
20+
<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>
2121
<sample src="UnsafeAndroidAccess.java" />
2222
</example>
2323

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

Lines changed: 31 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* @name Unsafe resource loading in Android webview
2+
* @name Unsafe resource fetching in Android webview
33
* @description JavaScript rendered inside WebViews can access any protected application file and web resource from any origin
44
* @kind path-problem
55
* @tags security
@@ -13,10 +13,10 @@ import semmle.code.java.frameworks.android.WebView
1313
import semmle.code.java.dataflow.FlowSources
1414

1515
/**
16-
* Allow universal access methods in the WebSettings class
16+
* Methods allowing any-local-file and universal access in the WebSettings class
1717
*/
18-
class AllowUniversalAccessMethod extends Method {
19-
AllowUniversalAccessMethod() {
18+
class CrossOriginAccessMethod extends Method {
19+
CrossOriginAccessMethod() {
2020
this.getDeclaringType() instanceof TypeWebSettings and
2121
(
2222
this.hasName("setAllowUniversalAccessFromFileURLs") or
@@ -36,22 +36,22 @@ class AllowJavaScriptMethod extends Method {
3636
}
3737

3838
/**
39-
* Check whether JavaScript is enabled in the webview with universal resource access
39+
* Holds if `ma` is a method invocation against `va` and `va.setJavaScriptEnabled(true)` occurs elsewhere in the program
4040
*/
41-
predicate isJSEnabled(MethodAccess ma) {
41+
predicate isJSEnabled(Variable v) {
4242
exists(VarAccess va, MethodAccess jsa |
43-
ma.getQualifier() = va and
44-
jsa.getQualifier() = va.getVariable().getAnAccess() and
43+
v.getAnAccess() = va and
44+
jsa.getQualifier() = v.getAnAccess() and
4545
jsa.getMethod() instanceof AllowJavaScriptMethod and
4646
jsa.getArgument(0).(BooleanLiteral).getBooleanValue() = true
4747
)
4848
}
4949

5050
/**
51-
* Load URL method call on the `android.webkit.WebView` object
51+
* Fetch URL method call on the `android.webkit.WebView` object
5252
*/
53-
class LoadResourceMethodAccess extends MethodAccess {
54-
LoadResourceMethodAccess() {
53+
class FetchResourceMethodAccess extends MethodAccess {
54+
FetchResourceMethodAccess() {
5555
this.getMethod().getDeclaringType() instanceof TypeWebView and
5656
(
5757
this.getMethod().hasName("loadUrl") or
@@ -75,65 +75,61 @@ class IntentGetExtraMethodAccess extends MethodAccess {
7575
}
7676

7777
/**
78-
* Source of loading urls
78+
* Source of fetching urls
7979
*/
8080
class UntrustedResourceSource extends RemoteFlowSource {
8181
UntrustedResourceSource() {
82-
exists(MethodAccess ma |
83-
ma instanceof IntentGetExtraMethodAccess and
82+
exists(IntentGetExtraMethodAccess ma |
8483
this.asExpr().(VarAccess).getVariable().getAnAssignedValue() = ma
8584
)
8685
}
8786

8887
override string getSourceType() {
8988
result = "user input vulnerable to XSS and sensitive resource disclosure attacks" and
9089
exists(MethodAccess ma |
91-
ma.getMethod() instanceof AllowUniversalAccessMethod and //High precision match of unsafe resource loading
90+
ma.getMethod() instanceof CrossOriginAccessMethod and //High precision match of unsafe resource fetching
9291
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true
9392
)
9493
or
9594
result = "user input potentially vulnerable to XSS and sensitive resource disclosure attacks" and
9695
not exists(MethodAccess ma |
97-
ma.getMethod() instanceof AllowUniversalAccessMethod and //High precision match of unsafe resource loading
96+
ma.getMethod() instanceof CrossOriginAccessMethod and //High precision match of unsafe resource fetching
9897
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true
9998
)
10099
}
101100
}
102101

103102
/**
104-
* load externally controlled data from loadUntrustedResource
103+
* Holds if `ma` loads url `sink`
105104
*/
106-
predicate loadUntrustedResource(MethodAccess ma, Expr sink) {
107-
ma instanceof LoadResourceMethodAccess and
108-
sink = ma.getArgument(0)
109-
}
105+
predicate fetchResource(FetchResourceMethodAccess ma, Expr sink) { sink = ma.getArgument(0) }
110106

111107
/**
112-
* Sink of loading urls
108+
* Sink of fetching urls
113109
*/
114-
class UntrustedResourceSink extends DataFlow::ExprNode {
115-
UntrustedResourceSink() { loadUntrustedResource(_, this.getExpr()) }
110+
class UrlResourceSink extends DataFlow::ExprNode {
111+
UrlResourceSink() { fetchResource(_, this.getExpr()) }
116112

117-
MethodAccess getMethodAccess() { loadUntrustedResource(result, this.getExpr()) }
113+
FetchResourceMethodAccess getMethodAccess() { fetchResource(result, this.getExpr()) }
118114
}
119115

120-
class LoadUntrustedResourceConfiguration extends TaintTracking::Configuration {
121-
LoadUntrustedResourceConfiguration() { this = "LoadUntrustedResourceConfiguration" }
116+
class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration {
117+
FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" }
122118

123119
override predicate isSource(DataFlow::Node source) { source instanceof UntrustedResourceSource }
124120

125-
override predicate isSink(DataFlow::Node sink) { sink instanceof UntrustedResourceSink }
121+
override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink }
126122
}
127123

128-
from DataFlow::PathNode source, DataFlow::PathNode sink, LoadUntrustedResourceConfiguration conf
124+
from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf
129125
where
130-
exists(VarAccess webviewVa, MethodAccess getSettingsMa, MethodAccess ma |
126+
exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v |
131127
conf.hasFlowPath(source, sink) and
132-
sink.getNode().(UntrustedResourceSink).getMethodAccess().getQualifier() = webviewVa and
128+
sink.getNode().(UrlResourceSink).getMethodAccess().getQualifier() = webviewVa and
133129
webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and
134-
ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and
135-
isJSEnabled(ma)
130+
v.getAnAssignedValue() = getSettingsMa and
131+
isJSEnabled(v)
136132
)
137-
select sink.getNode().(UntrustedResourceSink).getMethodAccess(), source, sink,
138-
"Unsafe resource loading in Android webview due to $@.", source.getNode(),
133+
select sink.getNode().(UrlResourceSink).getMethodAccess(), source, sink,
134+
"Unsafe resource fetching in Android webview due to $@.", source.getNode(),
139135
source.getNode().(UntrustedResourceSource).getSourceType()

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,2 @@
1-
| UnsafeAndroidAccess.java:29:3:29:21 | loadUrl(...) | UnsafeAndroidAccess.java:29:14:29:20 | thisUrl | UnsafeAndroidAccess.java:29:14:29:20 | thisUrl | Unsafe resource loading in Android webview due to $@. | UnsafeAndroidAccess.java:29:14:29:20 | thisUrl | user input vulnerable to XSS and sensitive resource disclosure attacks |
1+
| UnsafeAndroidAccess.java:30:3:30:21 | loadUrl(...) | UnsafeAndroidAccess.java:30:14:30:20 | thisUrl | UnsafeAndroidAccess.java:30:14:30:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:30:14:30:20 | thisUrl | user input vulnerable to XSS and sensitive resource disclosure attacks |
2+
| UnsafeAndroidAccess.java:53:3:53:21 | loadUrl(...) | UnsafeAndroidAccess.java:53:14:53:20 | thisUrl | UnsafeAndroidAccess.java:53:14:53:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:53:14:53:20 | thisUrl | user input vulnerable to XSS and sensitive resource disclosure attacks |

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

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,8 @@
77
import android.webkit.WebViewClient;
88

99
public class UnsafeAndroidAccess extends Activity {
10-
public void onCreate(Bundle savedInstanceState) {
10+
//Test onCreate with both JavaScript and universal resource access enabled while taking remote user inputs from bundle extras
11+
public void testOnCreate1(Bundle savedInstanceState) {
1112
super.onCreate(savedInstanceState);
1213
setContentView(-1);
1314

@@ -28,4 +29,69 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
2829
String thisUrl = getIntent().getExtras().getString("url");
2930
wv.loadUrl(thisUrl);
3031
}
32+
33+
//Test onCreate with both JavaScript and universal resource access enabled while taking remote user inputs from string extra
34+
public void testOnCreate2(Bundle savedInstanceState) {
35+
super.onCreate(savedInstanceState);
36+
setContentView(-1);
37+
38+
WebView wv = (WebView) findViewById(-1);
39+
WebSettings webSettings = wv.getSettings();
40+
41+
webSettings.setJavaScriptEnabled(true);
42+
webSettings.setAllowFileAccessFromFileURLs(true);
43+
44+
wv.setWebViewClient(new WebViewClient() {
45+
@Override
46+
public boolean shouldOverrideUrlLoading(WebView view, String url) {
47+
view.loadUrl(url);
48+
return true;
49+
}
50+
});
51+
52+
String thisUrl = getIntent().getStringExtra("url");
53+
wv.loadUrl(thisUrl);
54+
}
55+
56+
//Test onCreate with both JavaScript and universal resource access disabled by default while taking remote user inputs
57+
public void testOnCreate3(Bundle savedInstanceState) {
58+
super.onCreate(savedInstanceState);
59+
setContentView(-1);
60+
61+
WebView wv = (WebView) findViewById(-1);
62+
WebSettings webSettings = wv.getSettings();
63+
64+
wv.setWebViewClient(new WebViewClient() {
65+
@Override
66+
public boolean shouldOverrideUrlLoading(WebView view, String url) {
67+
view.loadUrl(url);
68+
return true;
69+
}
70+
});
71+
72+
String thisUrl = getIntent().getStringExtra("url");
73+
wv.loadUrl(thisUrl);
74+
}
75+
76+
//Test onCreate with both JavaScript and universal resource access enabled while not taking remote user inputs
77+
public void testOnCreate4(Bundle savedInstanceState) {
78+
super.onCreate(savedInstanceState);
79+
setContentView(-1);
80+
81+
WebView wv = (WebView) findViewById(-1);
82+
WebSettings webSettings = wv.getSettings();
83+
84+
webSettings.setJavaScriptEnabled(true);
85+
webSettings.setAllowFileAccessFromFileURLs(true);
86+
87+
wv.setWebViewClient(new WebViewClient() {
88+
@Override
89+
public boolean shouldOverrideUrlLoading(WebView view, String url) {
90+
view.loadUrl(url);
91+
return true;
92+
}
93+
});
94+
95+
wv.loadUrl("https://www.mycorp.com");
96+
}
3197
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql
1+
experimental/Security/CWE/CWE-749/UnsafeAndroidAccess.ql

0 commit comments

Comments
 (0)