Skip to content

Commit 153ec53

Browse files
atorralbaatorralba
committed
First query version requiring sinks to flow to write operations
1 parent 7a7d164 commit 153ec53

File tree

3 files changed

+174
-0
lines changed

3 files changed

+174
-0
lines changed

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/**
2+
* @name Uncontrolled data used in path expression
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 error
7+
* @precision high
8+
* @id java/android/unsafe-content-uri-resolution
9+
* @tags security
10+
* external/cwe/cwe-441
11+
* external/cwe/cwe-610
12+
*/
13+
14+
import java
15+
import UnsafeContentUriResolutionQuery
16+
import DataFlow::PathGraph
17+
18+
from DataFlow::PathNode src, DataFlow::PathNode sink
19+
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.",
22+
src.getNode(), "user input"

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

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

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
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.dataflow.TaintTracking2
8+
import UnsafeContentUriResolution
9+
10+
/** A taint-tracking configuration to find paths from remote sources to content URI resolutions. */
11+
class UnsafeContentResolutionConf extends TaintTracking::Configuration {
12+
UnsafeContentResolutionConf() { this = "UnsafeContentResolutionConf" }
13+
14+
override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource }
15+
16+
override predicate isSink(DataFlow::Node sink) {
17+
flowsToWrite(sink.(ContentUriResolutionSink).getCallNode())
18+
}
19+
20+
override predicate isSanitizer(DataFlow::Node sanitizer) {
21+
sanitizer instanceof ContentUriResolutionSanitizer
22+
}
23+
24+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
25+
any(ContentUriResolutionAdditionalTaintStep s).step(node1, node2)
26+
}
27+
}
28+
29+
/** Holds if `node` flows to a write operation. */
30+
private predicate flowsToWrite(DataFlow::Node node) { any(FlowsToWriteConfig c).hasFlow(node, _) }
31+
32+
/** A taint-tracking configuration to find paths to write operations. */
33+
private class FlowsToWriteConfig extends TaintTracking2::Configuration {
34+
FlowsToWriteConfig() { this = "FlowsToWriteConfig" }
35+
36+
override predicate isSource(DataFlow::Node src) {
37+
src = any(ContentUriResolutionSink s).getCallNode()
38+
}
39+
40+
override predicate isSink(DataFlow::Node sink) {
41+
sinkNode(sink, "create-file")
42+
or
43+
sinkNode(sink, "write-file")
44+
or
45+
exists(MethodAccess ma | sink.asExpr() = ma.getArgument(0) |
46+
ma.getMethod() instanceof WriteStreamMethod
47+
)
48+
}
49+
}
50+
51+
private class WriteStreamMethod extends Method {
52+
WriteStreamMethod() {
53+
this.getAnOverride*().hasQualifiedName("java.io", "OutputStream", "write")
54+
or
55+
this.hasQualifiedName("org.apache.commons.io", "IOUtils", "copy")
56+
or
57+
this.hasQualifiedName("org.springframework.util", ["StreamUtils", "CopyUtils"], "copy")
58+
or
59+
this.hasQualifiedName("com.google.common.io", ["ByteStreams", "CharStreams"], "copy")
60+
}
61+
}

0 commit comments

Comments
 (0)