Skip to content

Commit c7750fd

Browse files
luchua-bcsmowton
luchua-bc
authored and
smowton
committed
Fine tune the query
1 parent 5338332 commit c7750fd

File tree

5 files changed

+70
-44
lines changed

5 files changed

+70
-44
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ public void onCreate(Bundle savedInstanceState) {
33
super.onCreate(savedInstanceState);
44
setContentView(R.layout.webview);
55

6-
// BAD: Have both JavaScript and universal resource access enabled in webview while
6+
// BAD: Have both JavaScript and cross-origin resource access enabled in webview while
77
// taking remote user inputs
88
{
99
WebView wv = (WebView) findViewById(R.id.my_webview);
@@ -24,7 +24,7 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
2424
wv.loadUrl(thisUrl);
2525
}
2626

27-
// BAD: Have both JavaScript and universal resource access enabled in webview while
27+
// BAD: Have both JavaScript and cross-origin resource access enabled in webview while
2828
// taking remote user inputs
2929
{
3030
WebView wv = (WebView) findViewById(R.id.my_webview);
@@ -45,7 +45,7 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
4545
wv.loadUrl(thisUrl);
4646
}
4747

48-
// GOOD: Have JavaScript and universal resource access disabled by default on modern Android (Jellybean+) while taking remote user inputs
48+
// GOOD: Have JavaScript and cross-origin resource access disabled by default on modern Android (Jellybean+) while taking remote user inputs
4949
{
5050
WebView wv = (WebView) findViewById(-1);
5151
WebSettings webSettings = wv.getSettings();

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@
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>
11-
<li>High precision vulnerability when allowing universal resource access is also enabled. The setting was just deprecated in API level 30 (Android 11) thus most devices are still affected given that Android phones don't get timely version updates like iPhones.</li>
11+
<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>
1212
</ol>
1313
</overview>
1414

1515
<recommendation>
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>
16+
<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>
1717
</recommendation>
1818

1919
<example>
@@ -28,4 +28,4 @@
2828
<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>
2929
</li>
3030
</references>
31-
</qhelp>
31+
</qhelp>

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

Lines changed: 35 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import semmle.code.java.frameworks.android.WebView
1313
import semmle.code.java.dataflow.FlowSources
1414

1515
/**
16-
* Methods allowing any-local-file and universal access in the WebSettings class
16+
* Methods allowing any-local-file and cross-origin access in the WebSettings class
1717
*/
1818
class CrossOriginAccessMethod extends Method {
1919
CrossOriginAccessMethod() {
@@ -36,12 +36,11 @@ class AllowJavaScriptMethod extends Method {
3636
}
3737

3838
/**
39-
* Holds if `ma` is a method invocation against `va` and `va.setJavaScriptEnabled(true)` occurs elsewhere in the program
39+
* Holds if a call to `v.setJavaScriptEnabled(true)` exists
4040
*/
4141
predicate isJSEnabled(Variable v) {
42-
exists(VarAccess va, MethodAccess jsa |
43-
v.getAnAccess() = va and
44-
jsa.getQualifier() = v.getAnAccess() and
42+
exists(MethodAccess jsa |
43+
v.getAnAccess() = jsa.getQualifier() and
4544
jsa.getMethod() instanceof AllowJavaScriptMethod and
4645
jsa.getArgument(0).(BooleanLiteral).getBooleanValue() = true
4746
)
@@ -53,10 +52,7 @@ predicate isJSEnabled(Variable v) {
5352
class FetchResourceMethodAccess extends MethodAccess {
5453
FetchResourceMethodAccess() {
5554
this.getMethod().getDeclaringType() instanceof TypeWebView and
56-
(
57-
this.getMethod().hasName("loadUrl") or
58-
this.getMethod().hasName("postUrl")
59-
)
55+
this.getMethod().hasName(["loadUrl", "postUrl"])
6056
}
6157
}
6258

@@ -84,19 +80,7 @@ class UntrustedResourceSource extends RemoteFlowSource {
8480
)
8581
}
8682

87-
override string getSourceType() {
88-
result = "user input vulnerable to XSS and sensitive resource disclosure attacks" and
89-
exists(MethodAccess ma |
90-
ma.getMethod() instanceof CrossOriginAccessMethod and //High precision match of unsafe resource fetching
91-
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true
92-
)
93-
or
94-
result = "user input potentially vulnerable to XSS and sensitive resource disclosure attacks" and
95-
not exists(MethodAccess ma |
96-
ma.getMethod() instanceof CrossOriginAccessMethod and //High precision match of unsafe resource fetching
97-
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true
98-
)
99-
}
83+
override string getSourceType() { result = "UntrustedIntentExtraSource" }
10084
}
10185

10286
/**
@@ -111,25 +95,44 @@ class UrlResourceSink extends DataFlow::ExprNode {
11195
UrlResourceSink() { fetchResource(_, this.getExpr()) }
11296

11397
FetchResourceMethodAccess getMethodAccess() { fetchResource(result, this.getExpr()) }
98+
99+
predicate crossOriginAccessEnabled() {
100+
exists(MethodAccess ma, MethodAccess getSettingsMa |
101+
ma.getMethod() instanceof CrossOriginAccessMethod and // Unsafe resource fetching of more severe vulnerabilities
102+
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and
103+
ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and
104+
getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and
105+
getSettingsMa.getQualifier().(VarAccess).getVariable().getAnAccess() =
106+
getMethodAccess().getQualifier()
107+
)
108+
}
109+
110+
string getSinkType() {
111+
if crossOriginAccessEnabled()
112+
then result = "user input vulnerable to cross-origin and sensitive resource disclosure attacks"
113+
else result = "user input vulnerable to XSS attacks"
114+
}
114115
}
115116

116117
class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration {
117118
FetchUntrustedResourceConfiguration() { this = "FetchUntrustedResourceConfiguration" }
118119

119120
override predicate isSource(DataFlow::Node source) { source instanceof UntrustedResourceSource }
120121

121-
override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink }
122+
override predicate isSink(DataFlow::Node sink) {
123+
sink instanceof UrlResourceSink and
124+
exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v |
125+
sink.(UrlResourceSink).getMethodAccess().getQualifier() = webviewVa and
126+
getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and
127+
webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and
128+
v.getAnAssignedValue() = getSettingsMa and
129+
isJSEnabled(v)
130+
)
131+
}
122132
}
123133

124134
from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf
125-
where
126-
exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v |
127-
conf.hasFlowPath(source, sink) and
128-
sink.getNode().(UrlResourceSink).getMethodAccess().getQualifier() = webviewVa and
129-
webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and
130-
v.getAnAssignedValue() = getSettingsMa and
131-
isJSEnabled(v)
132-
)
135+
where conf.hasFlowPath(source, sink)
133136
select sink.getNode().(UrlResourceSink).getMethodAccess(), source, sink,
134137
"Unsafe resource fetching in Android webview due to $@.", source.getNode(),
135-
source.getNode().(UntrustedResourceSource).getSourceType()
138+
sink.getNode().(UrlResourceSink).getSinkType()

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
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 |
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 cross-origin 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 cross-origin and sensitive resource disclosure attacks |
3+
| UnsafeAndroidAccess.java:95:3:95:21 | loadUrl(...) | UnsafeAndroidAccess.java:95:14:95:20 | thisUrl | UnsafeAndroidAccess.java:95:14:95:20 | thisUrl | Unsafe resource fetching in Android webview due to $@. | UnsafeAndroidAccess.java:95:14:95:20 | thisUrl | user input vulnerable to XSS attacks |

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

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import android.webkit.WebViewClient;
88

99
public class UnsafeAndroidAccess extends Activity {
10-
//Test onCreate with both JavaScript and universal resource access enabled while taking remote user inputs from bundle extras
10+
//Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from bundle extras
1111
public void testOnCreate1(Bundle savedInstanceState) {
1212
super.onCreate(savedInstanceState);
1313
setContentView(-1);
@@ -30,7 +30,7 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
3030
wv.loadUrl(thisUrl);
3131
}
3232

33-
//Test onCreate with both JavaScript and universal resource access enabled while taking remote user inputs from string extra
33+
//Test onCreate with both JavaScript and cross-origin resource access enabled while taking remote user inputs from string extra
3434
public void testOnCreate2(Bundle savedInstanceState) {
3535
super.onCreate(savedInstanceState);
3636
setContentView(-1);
@@ -53,7 +53,7 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
5353
wv.loadUrl(thisUrl);
5454
}
5555

56-
//Test onCreate with both JavaScript and universal resource access disabled by default while taking remote user inputs
56+
//Test onCreate with both JavaScript and cross-origin resource access disabled by default while taking remote user inputs
5757
public void testOnCreate3(Bundle savedInstanceState) {
5858
super.onCreate(savedInstanceState);
5959
setContentView(-1);
@@ -73,14 +73,36 @@ public boolean shouldOverrideUrlLoading(WebView view, String url) {
7373
wv.loadUrl(thisUrl);
7474
}
7575

76-
//Test onCreate with both JavaScript and universal resource access enabled while not taking remote user inputs
76+
//Test onCreate with JavaScript enabled but cross-origin resource access disabled while taking remote user inputs
7777
public void testOnCreate4(Bundle savedInstanceState) {
7878
super.onCreate(savedInstanceState);
7979
setContentView(-1);
8080

8181
WebView wv = (WebView) findViewById(-1);
8282
WebSettings webSettings = wv.getSettings();
8383

84+
webSettings.setJavaScriptEnabled(true);
85+
86+
wv.setWebViewClient(new WebViewClient() {
87+
@Override
88+
public boolean shouldOverrideUrlLoading(WebView view, String url) {
89+
view.loadUrl(url);
90+
return true;
91+
}
92+
});
93+
94+
String thisUrl = getIntent().getStringExtra("url");
95+
wv.loadUrl(thisUrl);
96+
}
97+
98+
//Test onCreate with both JavaScript and cross-origin resource access enabled while not taking remote user inputs
99+
public void testOnCreate5(Bundle savedInstanceState) {
100+
super.onCreate(savedInstanceState);
101+
setContentView(-1);
102+
103+
WebView wv = (WebView) findViewById(-1);
104+
WebSettings webSettings = wv.getSettings();
105+
84106
webSettings.setJavaScriptEnabled(true);
85107
webSettings.setAllowFileAccessFromFileURLs(true);
86108

0 commit comments

Comments
 (0)