Skip to content

Commit 9675f34

Browse files
smowtonsmowton
authored
Merge pull request #8257 from luchua-bc/java/insecure-webview-resource-response
Java: CWE-200 Query to detect insecure WebResourceResponse implementation
2 parents 031d183 + fa2a6a7 commit 9675f34

File tree

12 files changed

+1115
-3
lines changed

12 files changed

+1115
-3
lines changed

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

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

‎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> class, which allows an Android application to behave
7+
as a web server by handling requests of popular protocols such as <code>http(s)</code>, <code>file</code>,
8+
as well as <code>javascript</code> and returning a response (including status code, content type, content
9+
encoding, headers and the response body). Improper implementation with insufficient input validation can lead
10+
to leakage of sensitive configuration files or user data because requests could refer to paths intended to be
11+
application-private.
12+
</p>
13+
</overview>
14+
15+
<recommendation>
16+
<p>
17+
Unsanitized user-provided URLs must not be used to serve a response directly. When handling a request,
18+
always validate that the requested file path is not in the receiver's protected directory. Alternatively
19+
the Android class <code>WebViewAssetLoader</code> can be used, which safely processes data from resources,
20+
assets or a predefined directory.
21+
</p>
22+
</recommendation>
23+
24+
<example>
25+
<p>
26+
The following examples show a bad scenario and two good scenarios respectively. In the bad scenario, a
27+
response is served without path validation. In the good scenario, 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+
Oversecured:
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 An insecure implementation of Android `WebResourceResponse` may lead to leakage of arbitrary
4+
* 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 $@.",
33+
source.getNode(), "this user input"

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

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

23+
/**
24+
* Given input `e` = `v.method1(...).method2(...)...`, returns `v` where `v` is a `VarAccess`.
25+
*
26+
* This is used to look through field accessors such as `uri.getPath()`.
27+
*/
28+
private Expr getUnderlyingVarAccess(Expr e) {
29+
result = getUnderlyingVarAccess(e.(MethodAccess).getQualifier())
30+
or
31+
result = e.(VarAccess)
32+
}
33+
2334
private class AllowListGuard extends Guard instanceof MethodAccess {
2435
AllowListGuard() {
2536
(isStringPartialMatch(this) or isPathPartialMatch(this)) and
2637
not isDisallowedWord(super.getAnArgument())
2738
}
2839

29-
Expr getCheckedExpr() { result = super.getQualifier() }
40+
Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) }
3041
}
3142

3243
/**
@@ -73,7 +84,7 @@ private class BlockListGuard extends Guard instanceof MethodAccess {
7384
isDisallowedWord(super.getAnArgument())
7485
}
7586

76-
Expr getCheckedExpr() { result = super.getQualifier() }
87+
Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) }
7788
}
7889

7990
/**
@@ -144,7 +155,7 @@ class PathTraversalGuard extends Guard instanceof MethodAccess {
144155
super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".."
145156
}
146157

147-
Expr getCheckedExpr() { result = super.getQualifier() }
158+
Expr getCheckedExpr() { result = getUnderlyingVarAccess(super.getQualifier()) }
148159
}
149160

150161
/** 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)