hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-24 04:36:35 +01:00
Code Issues Packages Projects Releases Wiki Activity

Second query version

Browse Source 
Remove sinks flowing to write operations requirement
This commit is contained in:
Tony Torralba
2022-08-29 10:36:53 +02:00
parent 153ec5368e
commit 4a18892da9
12 changed files with 266 additions and 88 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.java Normal file
View File

@@ -0,0 +1,41 @@
import android.content.ContentResolver;
import android.net.Uri;
public class Example extends Activity {
public void onCreate() {
// BAD: Externally-provided URI directly used in content resolution
{
ContentResolver contentResolver = getContentResolver();
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
InputStream is = contentResolver.openInputStream(uri);
copyToExternalCache(is);
}
// BAD: input Uri is not normalized, and check can be bypassed with ".." characters
{
ContentResolver contentResolver = getContentResolver();
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
String path = uri.getPath();
if (path.startsWith("/data"))
throw new SecurityException();
InputStream is = contentResolver.openInputStream(uri);
copyToExternalCache(is);
}
// GOOD: URI gets properly validated to avoid access to internal files
{
ContentResolver contentResolver = getContentResolver();
Uri uri = (Uri) getIntent().getParcelableExtra("URI_EXTRA");
String path = uri.getPath();
java.nio.file.Path normalized =
java.nio.file.FileSystems.getDefault().getPath(path).normalize();
if (normalized.startsWith("/data"))
throw new SecurityException();
InputStream is = contentResolver.openInputStream(uri);
copyToExternalCache(is);
}
}
private void copyToExternalCache(InputStream is) {
// Reads the contents of is and writes a file in the app's external
// cache directory, which can be read publicly by applications in the same device.
}
}

49
java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.qhelp Normal file
View File

@@ -0,0 +1,49 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>
When an Android application wants to access data in a content provider, it uses the <code>ContentResolver</code>
object. <code>ContentResolver</code>s communicate with an instance of a class that implements the
<code>ContentProvider</code> interface via URIs with the <code>content://</code> scheme.
The authority part (the first path segment) of the URI passed as parameter to the <code>ContentResolver</code>
determines which content provider is contacted for the operation. Specific operations that act on files also
support the <code>file://</code> scheme, in which case the local filesystem is queried instead.
If an external component, like a malicious or compromised application, controls the URI that is used in a
<code>ContentResolver</code> operation, it can trick the vulnerable application into accessing its own private
files or non-exported content providers. Depending on what the vulnerable application does after accessing the file,
the attacking application might get access to the file by forcing it to be copied to a public directory like the
external storage, or tamper with it by making the application overwrite it with unexpected data.
</p>
</overview>
<recommendation>
<p>
If possible, avoid using externally-provided data to determine URIs used by a <code>ContentResolver</code>.
If that is not an option, validate that the incoming URI can only reference trusted components, like an allow list
of content providers and/or applications, or alternatively make sure that the URI does not reference private
directories like <code>/data/</code>.
</p>
</recommendation>
<example>
This example shows two ways of opening a file using a <code>ContentResolver</code>. In the first case, externally-provided
data coming from an intent is directly used in the file-reading operation, allowing an attacker to provide a URI
of the form <code>/data/data/(vulnerable app package)/(private file)</code> to trick the application into reading it and
copying it to the external storage. In the second case, the URI is validated before being used, making sure it does not reference
any internal application files.
<p>
</p>
<sample src="UnsafeContentUriResolution.java" />
</example>
<references>
<li>
Android developers:
<a href="https://developer.android.com/guide/topics/providers/content-provider-basics">Content provider basics</a>
</li>
<li>
<a href="https://developer.android.com/reference/android/content/ContentResolver">The ContentResolver class</a>
</li>
</references>
</qhelp>

9
java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.ql
View File

@@ -1,9 +1,9 @@
/**
* @name Uncontrolled data used in path expression
* @name Uncontrolled data used in content resolution
* @description Resolving externally-provided content URIs without validation can allow an attacker
* to access unexpected resources.
* @kind path-problem
* @problem.severity error
* @problem.severity warning
* @precision high
* @id java/android/unsafe-content-uri-resolution
* @tags security
@@ -12,11 +12,10 @@
*/
import java
import UnsafeContentUriResolutionQuery
import semmle.code.java.security.UnsafeContentUriResolutionQuery
import DataFlow::PathGraph
from DataFlow::PathNode src, DataFlow::PathNode sink
where any(UnsafeContentResolutionConf c).hasFlowPath(src, sink)
select sink.getNode(), src, sink,
"This $@ flows to a ContentResolver method that resolves a URI. The result is then used in a write operation.",
select sink.getNode(), src, sink, "This $@ flows to a ContentResolver method that resolves a URI.",
src.getNode(), "user input"

91
java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolution.qll
View File

@@ -1,91 +0,0 @@
/** Provides classes to reason about vulnerabilites related to content URIs. */
import java
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.frameworks.android.Android
/** A URI that gets resolved by a `ContentResolver`. */
abstract class ContentUriResolutionSink extends DataFlow::Node {
/** Gets the call node that resolves this URI. */
abstract DataFlow::Node getCallNode();
}
/** A sanitizer for content URIs. */
abstract class ContentUriResolutionSanitizer extends DataFlow::Node { }
/**
* A unit class for adding additional taint steps to configurations related to
* content URI resolution vulnerabilities.
*/
abstract class ContentUriResolutionAdditionalTaintStep extends Unit {
/** Holds if the step from `node1` to `node2` should be considered an additional taint step. */
abstract predicate step(DataFlow::Node node1, DataFlow::Node node2);
}
/** The URI argument of a call to a `ContentResolver` URI-opening method. */
private class DefaultContentUriResolutionSink extends ContentUriResolutionSink {
DefaultContentUriResolutionSink() {
exists(MethodAccess ma |
ma.getMethod() instanceof UriOpeningContentResolverMethod and
this.asExpr() = ma.getAnArgument() and
this.getType().(RefType).hasQualifiedName("android.net", "Uri")
)
}
/** Gets the call node of this argument. */
override DataFlow::Node getCallNode() {
result = DataFlow::exprNode(this.asExpr().(Argument).getCall())
}
}
private class UninterestingTypeSanitizer extends ContentUriResolutionSanitizer {
UninterestingTypeSanitizer() {
this.getType() instanceof BoxedType or
this.getType() instanceof PrimitiveType or
this.getType() instanceof NumberType
}
}
private class FilenameOnlySanitizer extends ContentUriResolutionSanitizer {
FilenameOnlySanitizer() {
exists(Method m | this.asExpr().(MethodAccess).getMethod() = m |
m.hasQualifiedName("java.io", "File", "getName") or
m.hasQualifiedName("kotlin.io", "FilesKt", ["getNameWithoutExtension", "getExtension"]) or
m.hasQualifiedName("org.apache.commons.io", "FilenameUtils", "getName")
)
}
}
/**
* A `ContentUriResolutionSink` that flows to an image-decoding function.
* Such functions raise exceptions when the input is not a valid image,
* which prevents accessing arbitrary non-image files.
*/
private class DecodedAsAnImageSanitizer extends ContentUriResolutionSanitizer {
DecodedAsAnImageSanitizer() {
exists(Argument decodeArg, MethodAccess decode |
decode.getArgument(0) = decodeArg and
decode
.getMethod()
.hasQualifiedName("android.graphics", "BitmapFactory",
[
"decodeByteArray", "decodeFile", "decodeFileDescriptor", "decodeResource",
"decodeStream"
])
|
DataFlow::localFlow(this.(ContentUriResolutionSink).getCallNode(),
DataFlow::exprNode(decodeArg))
)
}
}
/** A `ContentResolver` method that resolves a URI. */
private class UriOpeningContentResolverMethod extends Method {
UriOpeningContentResolverMethod() {
this.hasName([
"openInputStream", "openOutputStream", "openAssetFile", "openAssetFileDescriptor",
"openFile", "openFileDescriptor", "openTypedAssetFile", "openTypedAssetFileDescriptor",
]) and
this.getDeclaringType() instanceof AndroidContentResolver
}
}

61
java/ql/src/Security/CWE/CWE-441/UnsafeContentUriResolutionQuery.qll
View File

@@ -1,61 +0,0 @@
/** Provides taint tracking configurations to be used in unsafe content URI resolution queries. */
import java
import semmle.code.java.dataflow.ExternalFlow
import semmle.code.java.dataflow.FlowSources
import semmle.code.java.dataflow.TaintTracking
import semmle.code.java.dataflow.TaintTracking2
import UnsafeContentUriResolution
/** A taint-tracking configuration to find paths from remote sources to content URI resolutions. */
class UnsafeContentResolutionConf extends TaintTracking::Configuration {
UnsafeContentResolutionConf() { this = "UnsafeContentResolutionConf" }
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
override predicate isSink(DataFlow::Node sink) {
flowsToWrite(sink.(ContentUriResolutionSink).getCallNode())
}
override predicate isSanitizer(DataFlow::Node sanitizer) {
sanitizer instanceof ContentUriResolutionSanitizer
}
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
any(ContentUriResolutionAdditionalTaintStep s).step(node1, node2)
}
}
/** Holds if `node` flows to a write operation. */
private predicate flowsToWrite(DataFlow::Node node) { any(FlowsToWriteConfig c).hasFlow(node, _) }
/** A taint-tracking configuration to find paths to write operations. */
private class FlowsToWriteConfig extends TaintTracking2::Configuration {
FlowsToWriteConfig() { this = "FlowsToWriteConfig" }
override predicate isSource(DataFlow::Node src) {
src = any(ContentUriResolutionSink s).getCallNode()
}
override predicate isSink(DataFlow::Node sink) {
sinkNode(sink, "create-file")
or
sinkNode(sink, "write-file")
or
exists(MethodAccess ma | sink.asExpr() = ma.getArgument(0) |
ma.getMethod() instanceof WriteStreamMethod
)
}
}
private class WriteStreamMethod extends Method {
WriteStreamMethod() {
this.getAnOverride*().hasQualifiedName("java.io", "OutputStream", "write")
or
this.hasQualifiedName("org.apache.commons.io", "IOUtils", "copy")
or
this.hasQualifiedName("org.springframework.util", ["StreamUtils", "CopyUtils"], "copy")
or
this.hasQualifiedName("com.google.common.io", ["ByteStreams", "CharStreams"], "copy")
}
}