Skip to content

Commit 88d9694

Browse files
luchua-bcluchua-bc
committed
Query to detect insecure WebResourceResponse implementation
1 parent 958fd9b commit 88d9694

File tree

12 files changed

+1105
-3
lines changed

12 files changed

+1105
-3
lines changed

‎java/ql/src/experimental/Security/CWE/CWE-200/AndroidWebResourceResponse.qll

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/** Provides Android methods relating to web resource response. */
2+
3+
import java
4+
import semmle.code.java.dataflow.FlowSources
5+
6+
/**
7+
* The Android class `android.webkit.WebResourceRequest` for handling web requests.
8+
*/
9+
class WebResourceRequest extends RefType {
10+
WebResourceRequest() { this.hasQualifiedName("android.webkit", "WebResourceRequest") }
11+
}
12+
13+
/**
14+
* The Android class `android.webkit.WebResourceResponse` for rendering web responses.
15+
*/
16+
class WebResourceResponse extends RefType {
17+
WebResourceResponse() { this.hasQualifiedName("android.webkit", "WebResourceResponse") }
18+
}
19+
20+
/** The `shouldInterceptRequest` method of Android's `WebViewClient` class. */
21+
class ShouldInterceptRequestMethod extends Method {
22+
ShouldInterceptRequestMethod() {
23+
this.hasName("shouldInterceptRequest") and
24+
this.getDeclaringType().getASupertype*() instanceof TypeWebViewClient and
25+
this.getReturnType() instanceof WebResourceResponse
26+
}
27+
}
28+
29+
/** A method call to `setWebViewClient` of `WebView`. */
30+
class SetWebViewClientMethodAccess extends MethodAccess {
31+
SetWebViewClientMethodAccess() {
32+
this.getMethod().hasName("setWebViewClient") and
33+
this.getMethod().getDeclaringType().getASupertype*() instanceof TypeWebView and
34+
this.getMethod().getParameterType(0) instanceof TypeWebViewClient
35+
}
36+
}
37+
38+
/** A sink representing a constructor call of `WebResourceResponse` in Android `WebViewClient`. */
39+
class WebResourceResponseSink extends DataFlow::Node {
40+
WebResourceResponseSink() {
41+
exists(ConstructorCall cc |
42+
cc.getConstructedType() instanceof WebResourceResponse and
43+
(
44+
this.asExpr() = cc.getArgument(2) and cc.getNumArgument() = 3 // WebResourceResponse(String mimeType, String encoding, InputStream data)
45+
or
46+
this.asExpr() = cc.getArgument(5) and cc.getNumArgument() = 6 // WebResourceResponse(String mimeType, String encoding, int statusCode, String reasonPhrase, Map<String, String> responseHeaders, InputStream data)
47+
)
48+
)
49+
}
50+
}
51+
52+
/**
53+
* Value step from a fetching url call of `WebView` to `WebViewClient`.
54+
*/
55+
private class FetchUrlStep extends AdditionalValueStep {
56+
override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
57+
exists(
58+
// webview.loadUrl(url) -> webview.setWebViewClient(new WebViewClient() { shouldInterceptRequest(view, url) });
59+
WebViewLoadUrlMethod lm, ShouldInterceptRequestMethod im, SetWebViewClientMethodAccess sma
60+
|
61+
sma.getArgument(0).getType() = im.getDeclaringType().getASupertype*() and
62+
exists(MethodAccess lma |
63+
lma.getMethod() = lm and
64+
lma.getQualifier().getType() = sma.getQualifier().getType() and
65+
pred.asExpr() = lma.getArgument(0) and
66+
succ.asExpr() = im.getParameter(1).getAnAccess()
67+
)
68+
)
69+
}
70+
}
71+
72+
/** Value/taint steps relating to url loading and file reading in an Android application. */
73+
private class LoadUrlSource extends SummaryModelCsv {
74+
override predicate row(string row) {
75+
row =
76+
[
77+
"java.io;FileInputStream;true;FileInputStream;;;Argument[0];Argument[-1];taint",
78+
"android.net;Uri;false;getPath;;;Argument[0];ReturnValue;value",
79+
"android.webkit;WebResourceRequest;false;getUrl;;;Argument[-1];ReturnValue;value"
80+
]
81+
}
82+
}

‎java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
// BAD: no URI validation
2+
Uri uri = Uri.parse(url);
3+
FileInputStream inputStream = new FileInputStream(uri.getPath());
4+
String mimeType = getMimeTypeFromPath(uri.getPath());
5+
return new WebResourceResponse(mimeType, "UTF-8", inputStream);
6+
7+
8+
// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
9+
// (alternatively use `WebViewAssetsLoader`)
10+
if (uri.getPath().startsWith("/local_cache/") && !uri.getPath().contains("..")) {
11+
File cacheFile = new File(getCacheDir(), uri.getLastPathSegment());
12+
FileInputStream inputStream = new FileInputStream(cacheFile);
13+
String mimeType = getMimeTypeFromPath(uri.getPath());
14+
return new WebResourceResponse(mimeType, "UTF-8", inputStream);
15+
}
16+
17+
return assetLoader.shouldInterceptRequest(request.getUrl());

‎java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.qhelp

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>Android provides a <code>WebResourceResponse</code> API, which is a <code>WebView</code> class that
7+
allows an Android application to behave as a web server by handling requests of popular protocols such
8+
as <code>http(s)</code>, <code>file</code>, as well as <code>javascript</code>; and returning a response
9+
(including status code, content type, content encoding, headers and the response body). Improper
10+
implementation with insufficient input validation can lead to leaking of sensitive configuration file
11+
or user data because requests could refer to paths intended to be application-private.
12+
</p>
13+
</overview>
14+
15+
<recommendation>
16+
<p>
17+
Unsanitized user provided url must not be used to serve a response directly. When handling a request,
18+
always validate that the file path is not the receiver's protected directory. Alternatively the Android
19+
API <code>WebViewAssetLoader</code> can be used, which safely processes data from resources, assets or
20+
a predefined directory.
21+
</p>
22+
</recommendation>
23+
24+
<example>
25+
<p>
26+
The following examples show a bad situation and two good situations respectively. In the bad situation, a
27+
response is served without path validation. In the good situation, a response is either served with path
28+
validation or through the safe <code>WebViewAssetLoader</code> implementation.
29+
</p>
30+
<sample src="InsecureWebResourceResponse.java" />
31+
</example>
32+
33+
<references>
34+
<li>
35+
Google:
36+
<a href="https://blog.oversecured.com/Android-Exploring-vulnerabilities-in-WebResourceResponse/">Android: Exploring vulnerabilities in WebResourceResponse</a>.
37+
</li>
38+
<li>
39+
CVE:
40+
<a href="https://cordova.apache.org/announcements/2014/08/04/android-351.html">CVE-2014-3502: Cordova apps can potentially leak data to other apps via URL loading</a>.
41+
</li>
42+
</references>
43+
</qhelp>

‎java/ql/src/experimental/Security/CWE/CWE-200/InsecureWebResourceResponse.ql

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* @name Insecure Android WebView Resource Response
3+
* @description Insecure implementation of Android WebResourceResponse intercepts malicious app requests
4+
* and return arbitrary sensitive content.
5+
* @kind path-problem
6+
* @id java/insecure-webview-resource-response
7+
* @problem.severity error
8+
* @tags security
9+
* external/cwe/cwe-200
10+
*/
11+
12+
import java
13+
import semmle.code.java.controlflow.Guards
14+
import experimental.semmle.code.java.PathSanitizer
15+
import AndroidWebResourceResponse
16+
import DataFlow::PathGraph
17+
18+
class InsecureWebResourceResponseConfig extends TaintTracking::Configuration {
19+
InsecureWebResourceResponseConfig() { this = "InsecureWebResourceResponseConfig" }
20+
21+
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
22+
23+
override predicate isSink(DataFlow::Node sink) { sink instanceof WebResourceResponseSink }
24+
25+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
26+
guard instanceof PathTraversalBarrierGuard
27+
}
28+
}
29+
30+
from DataFlow::PathNode source, DataFlow::PathNode sink, InsecureWebResourceResponseConfig conf
31+
where conf.hasFlowPath(source, sink)
32+
select sink.getNode(), source, sink, "Leaking arbitrary content in Android from $@.", source.getNode(),
33+
"this user input"

‎java/ql/src/experimental/semmle/code/java/PathSanitizer.qll

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,25 @@ private class ExactStringPathMatchGuard extends PathTraversalBarrierGuard instan
2020
}
2121
}
2222

23+
/**
24+
* Returns the qualifier of a method call if it's a variable access, or the qualifier of the qualifier if
25+
* the qualifier itself is a method call to `getPath`, which helps to reduce FPs by handling scenarios
26+
* such as `!uri.getPath().contains("..")`.
27+
*/
28+
private Expr getRealQualifier(Expr e) {
29+
e.(MethodAccess).getMethod().hasQualifiedName("android.net", "Uri", "getPath") and
30+
result = e.(MethodAccess).getQualifier()
31+
or
32+
result = e.(VarAccess)
33+
}
34+
2335
private class AllowListGuard extends Guard instanceof MethodAccess {
2436
AllowListGuard() {
2537
(isStringPartialMatch(this) or isPathPartialMatch(this)) and
2638
not isDisallowedWord(super.getAnArgument())
2739
}
2840

29-
Expr getCheckedExpr() { result = super.getQualifier() }
41+
Expr getCheckedExpr() { result = getRealQualifier(super.getQualifier()) }
3042
}
3143

3244
/**
@@ -73,7 +85,7 @@ private class BlockListGuard extends Guard instanceof MethodAccess {
7385
isDisallowedWord(super.getAnArgument())
7486
}
7587

76-
Expr getCheckedExpr() { result = super.getQualifier() }
88+
Expr getCheckedExpr() { result = getRealQualifier(super.getQualifier()) }
7789
}
7890

7991
/**
@@ -144,7 +156,7 @@ class PathTraversalGuard extends Guard instanceof MethodAccess {
144156
super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
145157
}
146158

147-
Expr getCheckedExpr() { result = super.getQualifier() }
159+
Expr getCheckedExpr() { result = getRealQualifier(super.getQualifier()) }
148160
}
149161

150162
/** A complementary sanitizer that protects against path traversal using path normalization. */

‎java/ql/test/experimental/query-tests/security/CWE-200/AndroidManifest.xml

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
2+
package="com.example.app"
3+
android:installLocation="auto"
4+
android:versionCode="1"
5+
android:versionName="0.1" >
6+
7+
<uses-permission android:name="android.permission.INTERNET" />
8+
9+
<application
10+
android:icon="@drawable/ic_launcher"
11+
android:label="@string/app_name"
12+
android:theme="@style/AppTheme" >
13+
<activity
14+
android:name=".InsecureWebResourceResponse"
15+
android:icon="@drawable/ic_launcher"
16+
android:label="@string/app_name">
17+
<intent-filter>
18+
<action android:name="android.intent.action.MAIN" />
19+
<category android:name="android.intent.category.LAUNCHER" />
20+
</intent-filter>
21+
</activity>
22+
23+
<activity android:name=".InsecureWebViewActivity">
24+
<intent-filter>
25+
<action android:name="android.intent.action.VIEW" />
26+
</intent-filter>
27+
</activity>
28+
</application>
29+
30+
</manifest>

0 commit comments

Comments
 (0)