Skip to content

Commit 03ce8d6

Browse files
atorralbaatorralba
committed
Refactored to use CSV sink model
1 parent 9b78cee commit 03ce8d6

File tree

3 files changed

+71
-71
lines changed

3 files changed

+71
-71
lines changed

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

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,10 @@ class FetchUntrustedResourceConfiguration extends TaintTracking::Configuration {
2424

2525
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
2626

27-
override predicate isSink(DataFlow::Node sink) { sink instanceof FetchUntrustedResourceSink }
27+
override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink }
2828
}
2929

3030
from DataFlow::PathNode source, DataFlow::PathNode sink, FetchUntrustedResourceConfiguration conf
3131
where conf.hasFlowPath(source, sink)
32-
select sink.getNode().(FetchUntrustedResourceSink).getMethodAccess(), source, sink,
33-
"Unsafe resource fetching in Android webview due to $@.", source.getNode(),
34-
sink.getNode().(FetchUntrustedResourceSink).getSinkType()
32+
select sink.getNode(), source, sink, "Unsafe resource fetching in Android webview due to $@.",
33+
source.getNode(), sink.getNode().(UrlResourceSink).getSinkType()

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

Lines changed: 67 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,75 @@
1+
/**
2+
*/
3+
14
import java
25
import semmle.code.java.frameworks.android.WebView
36
import semmle.code.java.dataflow.DataFlow
47
import semmle.code.java.dataflow.ExternalFlow
58

9+
/**
10+
*/
11+
abstract class UrlResourceSink extends DataFlow::Node {
12+
/**
13+
* Returns a description of this vulnerability,
14+
*/
15+
abstract string getSinkType();
16+
}
17+
18+
/**
19+
* A URL argument to a `loadUrl` or `postUrl` call, considered as a sink.
20+
*/
21+
private class DefaultUrlResourceSinkModel extends SinkModelCsv {
22+
override predicate row(string row) {
23+
row =
24+
[
25+
"android.webkit;WebView;true;loadUrl;;;Argument[0];unsafe-android-access",
26+
"android.webkit;WebView;true;postUrl;;;Argument[0];unsafe-android-access"
27+
]
28+
}
29+
}
30+
31+
/**
32+
* Cross-origin access enabled resource fetch.
33+
*
34+
* Specifically this looks for code like
35+
* `webView.getSettings().setAllow[File|Universal]AccessFromFileURLs(true);`
36+
*/
37+
private class CrossOriginUrlResourceSink extends UrlResourceSink {
38+
CrossOriginUrlResourceSink() {
39+
sinkNode(this, "unsafe-android-access") and
40+
exists(MethodAccess ma, MethodAccess getSettingsMa |
41+
ma.getMethod() instanceof CrossOriginAccessMethod and
42+
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and
43+
ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and
44+
getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and
45+
getSettingsMa.getQualifier().(VarAccess).getVariable().getAnAccess() =
46+
this.asExpr().(Argument).getCall().getQualifier()
47+
)
48+
}
49+
50+
override string getSinkType() {
51+
result = "user input vulnerable to cross-origin and sensitive resource disclosure attacks"
52+
}
53+
}
54+
55+
/**
56+
* JavaScript enabled resource fetch.
57+
*/
58+
private class JavaScriptEnabledUrlResourceSink extends UrlResourceSink {
59+
JavaScriptEnabledUrlResourceSink() {
60+
sinkNode(this, "unsafe-android-access") and
61+
exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v |
62+
this.asExpr().(Argument).getCall().getQualifier() = webviewVa and
63+
getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and
64+
webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and
65+
v.getAnAssignedValue() = getSettingsMa and
66+
isJSEnabled(v)
67+
)
68+
}
69+
70+
override string getSinkType() { result = "user input vulnerable to XSS attacks" }
71+
}
72+
673
/**
774
* Methods allowing any-local-file and cross-origin access in the WebSettings class
875
*/
@@ -36,69 +103,3 @@ private predicate isJSEnabled(Variable v) {
36103
jsa.getArgument(0).(BooleanLiteral).getBooleanValue() = true
37104
)
38105
}
39-
40-
/**
41-
* Fetch URL method call on the `android.webkit.WebView` object
42-
*/
43-
private class FetchResourceMethodAccess extends MethodAccess {
44-
FetchResourceMethodAccess() {
45-
this.getMethod().getDeclaringType() instanceof TypeWebView and
46-
this.getMethod().hasName(["loadUrl", "postUrl"])
47-
}
48-
}
49-
50-
/**
51-
* Holds if `ma` loads URL `sink`
52-
*/
53-
private predicate fetchResource(FetchResourceMethodAccess ma, Expr sink) {
54-
sink = ma.getArgument(0)
55-
}
56-
57-
/**
58-
* A URL argument to a `loadUrl` or `postUrl` call, considered as a sink.
59-
*/
60-
private class UrlResourceSink extends DataFlow::ExprNode {
61-
UrlResourceSink() { fetchResource(_, this.getExpr()) }
62-
63-
/** Gets the fetch method that fetches this sink URL. */
64-
FetchResourceMethodAccess getMethodAccess() { fetchResource(result, this.getExpr()) }
65-
66-
/**
67-
* Holds if cross-origin access is enabled for this resource fetch.
68-
*
69-
* Specifically this looks for code like
70-
* `webView.getSettings().setAllow[File|Universal]AccessFromFileURLs(true);`
71-
*/
72-
predicate crossOriginAccessEnabled() {
73-
exists(MethodAccess ma, MethodAccess getSettingsMa |
74-
ma.getMethod() instanceof CrossOriginAccessMethod and
75-
ma.getArgument(0).(BooleanLiteral).getBooleanValue() = true and
76-
ma.getQualifier().(VarAccess).getVariable().getAnAssignedValue() = getSettingsMa and
77-
getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and
78-
getSettingsMa.getQualifier().(VarAccess).getVariable().getAnAccess() =
79-
getMethodAccess().getQualifier()
80-
)
81-
}
82-
83-
/**
84-
* Returns a description of this vulnerability, assuming Javascript is enabled and
85-
* the fetched URL is attacker-controlled.
86-
*/
87-
string getSinkType() {
88-
if crossOriginAccessEnabled()
89-
then result = "user input vulnerable to cross-origin and sensitive resource disclosure attacks"
90-
else result = "user input vulnerable to XSS attacks"
91-
}
92-
}
93-
94-
class FetchUntrustedResourceSink extends UrlResourceSink {
95-
FetchUntrustedResourceSink() {
96-
exists(VarAccess webviewVa, MethodAccess getSettingsMa, Variable v |
97-
this.getMethodAccess().getQualifier() = webviewVa and
98-
getSettingsMa.getMethod() instanceof WebViewGetSettingsMethod and
99-
webviewVa.getVariable().getAnAccess() = getSettingsMa.getQualifier() and
100-
v.getAnAssignedValue() = getSettingsMa and
101-
isJSEnabled(v)
102-
)
103-
}
104-
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ class Conf extends TaintTracking::Configuration {
99

1010
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
1111

12-
override predicate isSink(DataFlow::Node sink) { sink instanceof FetchUntrustedResourceSink }
12+
override predicate isSink(DataFlow::Node sink) { sink instanceof UrlResourceSink }
1313
}
1414

1515
class UnsafeAndroidAccessTest extends InlineExpectationsTest {

0 commit comments

Comments
 (0)