Skip to content

Commit fd8f8cb

Browse files
atorralbaatorralba
authored
Merge pull request github#10223 from atorralba/atorralba/unsafe-content-resolver
Java: New Android query to detect unsafe content URI resolution
2 parents 0f499df + 01a08d4 commit fd8f8cb

11 files changed

+361
-0
lines changed

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

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
/** Provides classes to reason about vulnerabilites related to content URIs. */
2+
3+
import java
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
7+
8+
/** A URI that gets resolved by a `ContentResolver`. */
9+
abstract class ContentUriResolutionSink extends DataFlow::Node { }
10+
11+
/** A sanitizer for content URIs. */
12+
abstract class ContentUriResolutionSanitizer extends DataFlow::Node { }
13+
14+
/**
15+
* A unit class for adding additional taint steps to configurations related to
16+
* content URI resolution vulnerabilities.
17+
*/
18+
class ContentUriResolutionAdditionalTaintStep extends Unit {
19+
/** Holds if the step from `node1` to `node2` should be considered an additional taint step. */
20+
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
21+
}
22+
23+
/** The URI argument of a call to a `ContentResolver` URI-opening method. */
24+
private class DefaultContentUriResolutionSink extends ContentUriResolutionSink {
25+
DefaultContentUriResolutionSink() {
26+
exists(MethodAccess ma |
27+
ma.getMethod() instanceof UriOpeningContentResolverMethod and
28+
this.asExpr() = ma.getAnArgument() and
29+
this.getType().(RefType).hasQualifiedName("android.net", "Uri")
30+
)
31+
}
32+
}
33+
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
42+
}
43+
}
44+
45+
private class UninterestingTypeSanitizer extends ContentUriResolutionSanitizer {
46+
UninterestingTypeSanitizer() {
47+
this.getType() instanceof BoxedType or
48+
this.getType() instanceof PrimitiveType or
49+
this.getType() instanceof NumberType
50+
}
51+
}
52+
53+
private class PathSanitizer extends ContentUriResolutionSanitizer instanceof PathInjectionSanitizer {
54+
}
55+
56+
private class FilenameOnlySanitizer extends ContentUriResolutionSanitizer {
57+
FilenameOnlySanitizer() {
58+
exists(Method m | this.asExpr().(MethodAccess).getMethod() = m |
59+
m.hasQualifiedName("java.io", "File", "getName") or
60+
m.hasQualifiedName("kotlin.io", "FilesKt", ["getNameWithoutExtension", "getExtension"]) or
61+
m.hasQualifiedName("org.apache.commons.io", "FilenameUtils", "getName")
62+
)
63+
}
64+
}
65+
66+
/**
67+
* A `ContentUriResolutionSink` that flows to an image-decoding function.
68+
* Such functions raise exceptions when the input is not a valid image,
69+
* which prevents accessing arbitrary non-image files.
70+
*/
71+
private class DecodedAsAnImageSanitizer extends ContentUriResolutionSanitizer {
72+
DecodedAsAnImageSanitizer() {
73+
exists(Argument decodeArg, MethodAccess decode |
74+
decode.getArgument(0) = decodeArg and
75+
decode
76+
.getMethod()
77+
.hasQualifiedName("android.graphics", "BitmapFactory",
78+
[
79+
"decodeByteArray", "decodeFile", "decodeFileDescriptor", "decodeResource",
80+
"decodeStream"
81+
])
82+
|
83+
TaintTracking::localExprTaint(this.(ContentUriResolutionSink).asExpr().(Argument).getCall(),
84+
decodeArg)
85+
)
86+
}
87+
}

‎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 is properly validated to block 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 for a
16+
<code>ContentResolver</code> operation, it can trick the vulnerable application into accessing its own private
17+
files or non-exported content providers. The attacking application might be able to get access to the file by forcing it to be copied to a public directory, like
18+
external storage, or tamper with the contents by making the application overwrite the file with unexpected data.
19+
</p>
20+
</overview>
21+
<recommendation>
22+
<p>
23+
If possible, avoid using externally-provided data to determine the URI for a <code>ContentResolver</code> to use.
24+
If that is not an option, validate that the incoming URI can only reference trusted components, like an allow list
25+
of content providers and/or applications, or alternatively make sure that the URI does not reference private
26+
directories like <code>/data/</code>.
27+
</p>
28+
</recommendation>
29+
<example>
30+
<p>
31+
This example shows three ways of opening a file using a <code>ContentResolver</code>. In the first case, externally-provided
32+
data from an intent is used directly in the file-reading operation. This allows 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, an insufficient check is performed on the externally-provided URI, still
35+
leaving room for exploitation. In the third case, the URI is correctly validated before being used, making sure it does not reference
36+
any internal application files.
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: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
/**
2+
* @name Uncontrolled data used in content resolution
3+
* @description Resolving externally-provided content URIs without validation can allow an attacker
4+
* to access unexpected resources.
5+
* @kind path-problem
6+
* @problem.severity warning
7+
* @security-severity 7.5
8+
* @precision high
9+
* @id java/android/unsafe-content-uri-resolution
10+
* @tags security
11+
* external/cwe/cwe-441
12+
* external/cwe/cwe-610
13+
*/
14+
15+
import java
16+
import semmle.code.java.security.UnsafeContentUriResolutionQuery
17+
import DataFlow::PathGraph
18+
19+
from DataFlow::PathNode src, DataFlow::PathNode sink
20+
where any(UnsafeContentResolutionConf c).hasFlowPath(src, sink)
21+
select sink.getNode(), src, sink,
22+
"This ContentResolver method that resolves a URI depends on a $@.", src.getNode(),
23+
"user-provided value"

‎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: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
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+
contentResolver.openOutputStream(uri); // $ hasTaintFlow
35+
contentResolver.openAssetFile(uri, null, null); // $ hasTaintFlow
36+
contentResolver.openAssetFileDescriptor(uri, null); // $ hasTaintFlow
37+
contentResolver.openFile(uri, null, null); // $ hasTaintFlow
38+
contentResolver.openFileDescriptor(uri, null); // $ hasTaintFlow
39+
contentResolver.openTypedAssetFile(uri, null, null, null); // $ hasTaintFlow
40+
contentResolver.openTypedAssetFileDescriptor(uri, null, null); // $ hasTaintFlow
41+
}
42+
{
43+
ContentResolver contentResolver = getContentResolver();
44+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
45+
String path = uri.getPath();
46+
if (path.startsWith("/data"))
47+
throw new SecurityException();
48+
contentResolver.openInputStream(uri); // $ hasTaintFlow
49+
}
50+
// Equals checks
51+
{
52+
ContentResolver contentResolver = getContentResolver();
53+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
54+
if (!uri.equals(Uri.parse("content://safe/uri")))
55+
throw new SecurityException();
56+
contentResolver.openInputStream(uri); // Safe
57+
}
58+
{
59+
ContentResolver contentResolver = getContentResolver();
60+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
61+
validateWithEquals(uri);
62+
contentResolver.openInputStream(uri); // Safe
63+
}
64+
// Allow list checks
65+
{
66+
ContentResolver contentResolver = getContentResolver();
67+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
68+
String path = uri.getPath();
69+
if (!path.startsWith("/safe/path"))
70+
throw new SecurityException();
71+
contentResolver.openInputStream(uri); // $ hasTaintFlow
72+
}
73+
{
74+
ContentResolver contentResolver = getContentResolver();
75+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
76+
String path = uri.getPath();
77+
java.nio.file.Path normalized =
78+
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
79+
if (!normalized.startsWith("/safe/path"))
80+
throw new SecurityException();
81+
contentResolver.openInputStream(uri); // Safe
82+
}
83+
{
84+
ContentResolver contentResolver = getContentResolver();
85+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
86+
validateWithAllowList(uri);
87+
contentResolver.openInputStream(uri); // Safe
88+
}
89+
// Block list checks
90+
{
91+
ContentResolver contentResolver = getContentResolver();
92+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
93+
String path = uri.getPath();
94+
if (path.startsWith("/data"))
95+
throw new SecurityException();
96+
contentResolver.openInputStream(uri); // $ hasTaintFlow
97+
}
98+
{
99+
ContentResolver contentResolver = getContentResolver();
100+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
101+
String path = uri.getPath();
102+
java.nio.file.Path normalized =
103+
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
104+
if (normalized.startsWith("/data"))
105+
throw new SecurityException();
106+
contentResolver.openInputStream(uri); // Safe
107+
}
108+
{
109+
ContentResolver contentResolver = getContentResolver();
110+
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
111+
validateWithBlockList(uri);
112+
contentResolver.openInputStream(uri); // Safe
113+
}
114+
}
115+
}

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

Whitespace-only changes.

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import java
2+
import TestUtilities.InlineFlowTest
3+
import semmle.code.java.security.UnsafeContentUriResolutionQuery
4+
5+
class Test extends InlineFlowTest {
6+
override DataFlow::Configuration getValueFlowConfig() { none() }
7+
8+
override TaintTracking::Configuration getTaintFlowConfig() {
9+
result instanceof UnsafeContentResolutionConf
10+
}
11+
}

0 commit comments

Comments
 (0)