Skip to content

Commit 4a18892

Browse files
atorralbaatorralba
committed
Second query version
Remove sinks flowing to write operations requirement
1 parent 153ec53 commit 4a18892

12 files changed

+266
-88
lines changed

‎java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.qll renamed to ‎java/ql/lib/semmle/code/java/security/UnsafeContentUriResolution.qll

Lines changed: 18 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,12 @@
11
/** Provides classes to reason about vulnerabilites related to content URIs. */
22

33
import java
4-
import semmle.code.java.dataflow.DataFlow
5-
import semmle.code.java.frameworks.android.Android
4+
private import semmle.code.java.dataflow.TaintTracking
5+
private import semmle.code.java.frameworks.android.Android
6+
private import semmle.code.java.security.PathSanitizer
67

78
/** A URI that gets resolved by a `ContentResolver`. */
8-
abstract class ContentUriResolutionSink extends DataFlow::Node {
9-
/** Gets the call node that resolves this URI. */
10-
abstract DataFlow::Node getCallNode();
11-
}
9+
abstract class ContentUriResolutionSink extends DataFlow::Node { }
1210

1311
/** A sanitizer for content URIs. */
1412
abstract class ContentUriResolutionSanitizer extends DataFlow::Node { }
@@ -31,10 +29,16 @@ private class DefaultContentUriResolutionSink extends ContentUriResolutionSink {
3129
this.getType().(RefType).hasQualifiedName("android.net", "Uri")
3230
)
3331
}
32+
}
3433

35-
/** Gets the call node of this argument. */
36-
override DataFlow::Node getCallNode() {
37-
result = DataFlow::exprNode(this.asExpr().(Argument).getCall())
34+
/** A `ContentResolver` method that resolves a URI. */
35+
private class UriOpeningContentResolverMethod extends Method {
36+
UriOpeningContentResolverMethod() {
37+
this.hasName([
38+
"openInputStream", "openOutputStream", "openAssetFile", "openAssetFileDescriptor",
39+
"openFile", "openFileDescriptor", "openTypedAssetFile", "openTypedAssetFileDescriptor",
40+
]) and
41+
this.getDeclaringType() instanceof AndroidContentResolver
3842
}
3943
}
4044

@@ -46,6 +50,9 @@ private class UninterestingTypeSanitizer extends ContentUriResolutionSanitizer {
4650
}
4751
}
4852

53+
private class PathSanitizer extends ContentUriResolutionSanitizer instanceof PathInjectionSanitizer {
54+
}
55+
4956
private class FilenameOnlySanitizer extends ContentUriResolutionSanitizer {
5057
FilenameOnlySanitizer() {
5158
exists(Method m | this.asExpr().(MethodAccess).getMethod() = m |
@@ -73,19 +80,8 @@ private class DecodedAsAnImageSanitizer extends ContentUriResolutionSanitizer {
7380
"decodeStream"
7481
])
7582
|
76-
DataFlow::localFlow(this.(ContentUriResolutionSink).getCallNode(),
77-
DataFlow::exprNode(decodeArg))
83+
TaintTracking::localExprTaint(this.(ContentUriResolutionSink).asExpr().(Argument).getCall(),
84+
decodeArg)
7885
)
7986
}
8087
}
81-
82-
/** A `ContentResolver` method that resolves a URI. */
83-
private class UriOpeningContentResolverMethod extends Method {
84-
UriOpeningContentResolverMethod() {
85-
this.hasName([
86-
"openInputStream", "openOutputStream", "openAssetFile", "openAssetFileDescriptor",
87-
"openFile", "openFileDescriptor", "openTypedAssetFile", "openTypedAssetFileDescriptor",
88-
]) and
89-
this.getDeclaringType() instanceof AndroidContentResolver
90-
}
91-
}

‎java/ql/lib/semmle/code/java/security/UnsafeContentUriResolutionQuery.qll

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
/** Provides taint tracking configurations to be used in unsafe content URI resolution queries. */
2+
3+
import java
4+
import semmle.code.java.dataflow.ExternalFlow
5+
import semmle.code.java.dataflow.FlowSources
6+
import semmle.code.java.dataflow.TaintTracking
7+
import semmle.code.java.security.UnsafeContentUriResolution
8+
9+
/** A taint-tracking configuration to find paths from remote sources to content URI resolutions. */
10+
class UnsafeContentResolutionConf extends TaintTracking::Configuration {
11+
UnsafeContentResolutionConf() { this = "UnsafeContentResolutionConf" }
12+
13+
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
14+
15+
override predicate isSink(DataFlow::Node sink) { sink instanceof ContentUriResolutionSink }
16+
17+
override predicate isSanitizer(DataFlow::Node sanitizer) {
18+
sanitizer instanceof ContentUriResolutionSanitizer
19+
}
20+
21+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
22+
any(ContentUriResolutionAdditionalTaintStep s).step(node1, node2)
23+
}
24+
}

‎java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.java

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import android.content.ContentResolver;
2+
import android.net.Uri;
3+
4+
public class Example extends Activity {
5+
public void onCreate() {
6+
// BAD: Externally-provided URI directly used in content resolution
7+
{
8+
ContentResolver contentResolver = getContentResolver();
9+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
10+
InputStream is = contentResolver.openInputStream(uri);
11+
copyToExternalCache(is);
12+
}
13+
// BAD: input Uri is not normalized, and check can be bypassed with ".." characters
14+
{
15+
ContentResolver contentResolver = getContentResolver();
16+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
17+
String path = uri.getPath();
18+
if (path.startsWith("/data"))
19+
throw new SecurityException();
20+
InputStream is = contentResolver.openInputStream(uri);
21+
copyToExternalCache(is);
22+
}
23+
// GOOD: URI gets properly validated to avoid access to internal files
24+
{
25+
ContentResolver contentResolver = getContentResolver();
26+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
27+
String path = uri.getPath();
28+
java.nio.file.Path normalized =
29+
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
30+
if (normalized.startsWith("/data"))
31+
throw new SecurityException();
32+
InputStream is = contentResolver.openInputStream(uri);
33+
copyToExternalCache(is);
34+
}
35+
}
36+
37+
private void copyToExternalCache(InputStream is) {
38+
// Reads the contents of is and writes a file in the app's external
39+
// cache directory, which can be read publicly by applications in the same device.
40+
}
41+
}

‎java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.qhelp

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
When an Android application wants to access data in a content provider, it uses the <code>ContentResolver</code>
8+
object. <code>ContentResolver</code>s communicate with an instance of a class that implements the
9+
<code>ContentProvider</code> interface via URIs with the <code>content://</code> scheme.
10+
11+
The authority part (the first path segment) of the URI passed as parameter to the <code>ContentResolver</code>
12+
determines which content provider is contacted for the operation. Specific operations that act on files also
13+
support the <code>file://</code> scheme, in which case the local filesystem is queried instead.
14+
15+
If an external component, like a malicious or compromised application, controls the URI that is used in a
16+
<code>ContentResolver</code> operation, it can trick the vulnerable application into accessing its own private
17+
files or non-exported content providers. Depending on what the vulnerable application does after accessing the file,
18+
the attacking application might get access to the file by forcing it to be copied to a public directory like the
19+
external storage, or tamper with it by making the application overwrite it with unexpected data.
20+
</p>
21+
</overview>
22+
<recommendation>
23+
<p>
24+
If possible, avoid using externally-provided data to determine URIs used by a <code>ContentResolver</code>.
25+
If that is not an option, validate that the incoming URI can only reference trusted components, like an allow list
26+
of content providers and/or applications, or alternatively make sure that the URI does not reference private
27+
directories like <code>/data/</code>.
28+
</p>
29+
</recommendation>
30+
<example>
31+
This example shows two ways of opening a file using a <code>ContentResolver</code>. In the first case, externally-provided
32+
data coming from an intent is directly used in the file-reading operation, allowing an attacker to provide a URI
33+
of the form <code>/data/data/(vulnerable app package)/(private file)</code> to trick the application into reading it and
34+
copying it to the external storage. In the second case, the URI is validated before being used, making sure it does not reference
35+
any internal application files.
36+
<p>
37+
</p>
38+
<sample src="UnsafeContentUriResolution.java" />
39+
</example>
40+
<references>
41+
<li>
42+
Android developers:
43+
<a href="https://developer.android.com/guide/topics/providers/content-provider-basics">Content provider basics</a>
44+
</li>
45+
<li>
46+
<a href="https://developer.android.com/reference/android/content/ContentResolver">The ContentResolver class</a>
47+
</li>
48+
</references>
49+
</qhelp>

‎java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.ql

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
/**
2-
* @name Uncontrolled data used in path expression
2+
* @name Uncontrolled data used in content resolution
33
* @description Resolving externally-provided content URIs without validation can allow an attacker
44
* to access unexpected resources.
55
* @kind path-problem
6-
* @problem.severity error
6+
* @problem.severity warning
77
* @precision high
88
* @id java/android/unsafe-content-uri-resolution
99
* @tags security
@@ -12,11 +12,10 @@
1212
*/
1313

1414
import java
15-
import UnsafeContentUriResolutionQuery
15+
import semmle.code.java.security.UnsafeContentUriResolutionQuery
1616
import DataFlow::PathGraph
1717

1818
from DataFlow::PathNode src, DataFlow::PathNode sink
1919
where any(UnsafeContentResolutionConf c).hasFlowPath(src, sink)
20-
select sink.getNode(), src, sink,
21-
"This $@ flows to a ContentResolver method that resolves a URI. The result is then used in a write operation.",
20+
select sink.getNode(), src, sink, "This $@ flows to a ContentResolver method that resolves a URI.",
2221
src.getNode(), "user input"

‎java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolutionQuery.qll

Lines changed: 0 additions & 61 deletions
This file was deleted.

‎java/ql/src/change-notes/2022-08-26-unsafe-content-uri-resolution.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* A new query "Uncontrolled data used in content resolution" (`java/androd/unsafe-content-uri-resolution`) has been added. This query finds paths from user-provided data to URI resolution operations in Android's `ContentResolver` without previous validation or sanitization.

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="test">
2+
<application>
3+
<activity android:exported="true" android:name=".Test">
4+
</activity>
5+
</application>
6+
</manifest>

‎java/ql/test/query-tests/security/CWE-441/Test.java

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
package test;
2+
3+
import android.content.ContentResolver;
4+
import android.net.Uri;
5+
import android.app.Activity;
6+
7+
public class Test extends Activity {
8+
private void validateWithEquals(Uri uri) {
9+
if (!uri.equals(Uri.parse("content://safe/uri")))
10+
throw new SecurityException();
11+
}
12+
13+
private void validateWithAllowList(Uri uri) throws SecurityException {
14+
String path = uri.getPath();
15+
java.nio.file.Path normalized =
16+
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
17+
if (!normalized.startsWith("/safe/path"))
18+
throw new SecurityException();
19+
}
20+
21+
private void validateWithBlockList(Uri uri) throws SecurityException {
22+
String path = uri.getPath();
23+
java.nio.file.Path normalized =
24+
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
25+
if (normalized.startsWith("/data"))
26+
throw new SecurityException();
27+
}
28+
29+
public void onCreate() {
30+
{
31+
ContentResolver contentResolver = getContentResolver();
32+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
33+
contentResolver.openInputStream(uri); // $ hasTaintFlow
34+
}
35+
{
36+
ContentResolver contentResolver = getContentResolver();
37+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
38+
String path = uri.getPath();
39+
if (path.startsWith("/data"))
40+
throw new SecurityException();
41+
contentResolver.openInputStream(uri); // $ hasTaintFlow
42+
}
43+
// Equals checks
44+
{
45+
ContentResolver contentResolver = getContentResolver();
46+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
47+
if (!uri.equals(Uri.parse("content://safe/uri")))
48+
throw new SecurityException();
49+
contentResolver.openInputStream(uri); // Safe
50+
}
51+
{
52+
ContentResolver contentResolver = getContentResolver();
53+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
54+
validateWithEquals(uri);
55+
contentResolver.openInputStream(uri); // Safe
56+
}
57+
// Allow list checks
58+
{
59+
ContentResolver contentResolver = getContentResolver();
60+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
61+
String path = uri.getPath();
62+
if (!path.startsWith("/safe/path"))
63+
throw new SecurityException();
64+
contentResolver.openInputStream(uri); // $ hasTaintFlow
65+
}
66+
{
67+
ContentResolver contentResolver = getContentResolver();
68+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
69+
String path = uri.getPath();
70+
java.nio.file.Path normalized =
71+
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
72+
if (!normalized.startsWith("/safe/path"))
73+
throw new SecurityException();
74+
contentResolver.openInputStream(uri); // Safe
75+
}
76+
{
77+
ContentResolver contentResolver = getContentResolver();
78+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
79+
validateWithAllowList(uri);
80+
contentResolver.openInputStream(uri); // Safe
81+
}
82+
// Block list checks
83+
{
84+
ContentResolver contentResolver = getContentResolver();
85+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
86+
String path = uri.getPath();
87+
if (path.startsWith("/data"))
88+
throw new SecurityException();
89+
contentResolver.openInputStream(uri); // $ hasTaintFlow
90+
}
91+
{
92+
ContentResolver contentResolver = getContentResolver();
93+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
94+
String path = uri.getPath();
95+
java.nio.file.Path normalized =
96+
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
97+
if (normalized.startsWith("/data"))
98+
throw new SecurityException();
99+
contentResolver.openInputStream(uri); // Safe
100+
}
101+
{
102+
ContentResolver contentResolver = getContentResolver();
103+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
104+
validateWithBlockList(uri);
105+
contentResolver.openInputStream(uri); // Safe
106+
}
107+
}
108+
}

‎java/ql/test/query-tests/security/CWE-441/UnsafeContentUriResolutionTest.expected

Whitespace-only changes.

0 commit comments

Comments
 (0)