Skip to content

Commit 19cb7ad

Browse files
atorralbaatorralba
committed
Migrate path injection sinks to MaD
Deprecate and stop using PathCreation Path creation sinks are now summaries
1 parent 52d7bd9 commit 19cb7ad

File tree

14 files changed

+219
-390
lines changed

14 files changed

+219
-390
lines changed

‎java/ql/lib/change-notes/2023-04-20-deprecated-path-creation.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: deprecated
3+
---
4+
* The `PathCreation` class in `PathCreation.qll` has been deprecated.

‎java/ql/lib/ext/java.io.model.yml‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,17 @@ extensions:
33
pack: codeql/java-all
44
extensible: sinkModel
55
data:
6-
- ["java.io", "File", False, "File", "(File,String)", "", "Argument[1]", "path-injection", "manual"] # old PathCreation
7-
- ["java.io", "File", False, "File", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
8-
- ["java.io", "File", False, "File", "(String,String)", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
9-
- ["java.io", "File", False, "File", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
106
- ["java.io", "File", True, "createNewFile", "()", "", "Argument[this]", "path-injection", "ai-manual"]
117
- ["java.io", "File", True, "createTempFile", "(String,String,File)", "", "Argument[2]", "path-injection", "ai-manual"]
128
- ["java.io", "File", True, "renameTo", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
139
- ["java.io", "FileInputStream", True, "FileInputStream", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
10+
- ["java.io", "FileInputStream", True, "FileInputStream", "(FileDescriptor)", "", "Argument[0]", "path-injection", "manual"]
1411
- ["java.io", "FileInputStream", True, "FileInputStream", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
1512
- ["java.io", "FileOutputStream", False, "FileOutputStream", "", "", "Argument[0]", "path-injection", "manual"]
1613
- ["java.io", "FileOutputStream", False, "write", "", "", "Argument[0]", "file-content-store", "manual"]
1714
- ["java.io", "FileReader", True, "FileReader", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
15+
- ["java.io", "FileReader", True, "FileReader", "(FileDescriptor)", "", "Argument[0]", "path-injection", "manual"]
16+
- ["java.io", "FileReader", True, "FileReader", "(File,Charset)", "", "Argument[0]", "path-injection", "manual"]
1817
- ["java.io", "FileReader", True, "FileReader", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
1918
- ["java.io", "FileReader", True, "FileReader", "(String,Charset)", "", "Argument[0]", "path-injection", "manual"]
2019
- ["java.io", "FileSystem", True, "createDirectory", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]

‎java/ql/lib/ext/java.nio.file.model.yml‎

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -37,15 +37,8 @@ extensions:
3737
- ["java.nio.file", "Files", False, "write", "", "", "Argument[1]", "file-content-store", "manual"]
3838
- ["java.nio.file", "Files", False, "writeString", "", "", "Argument[0]", "path-injection", "manual"]
3939
- ["java.nio.file", "Files", False, "writeString", "", "", "Argument[1]", "file-content-store", "manual"]
40-
- ["java.nio.file", "FileSystem", False, "getPath", "", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
4140
- ["java.nio.file", "FileSystems", False, "newFileSystem", "(URI,Map)", "", "Argument[0]", "path-injection", "ai-manual"]
4241
- ["java.nio.file", "FileSystems", False, "newFileSystem", "(URI,Map)", "", "Argument[0]", "request-forgery", "ai-manual"]
43-
- ["java.nio.file", "Path", False, "of", "(String,String[])", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
44-
- ["java.nio.file", "Path", False, "of", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
45-
- ["java.nio.file", "Path", False, "resolve", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
46-
- ["java.nio.file", "Path", False, "resolveSibling", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
47-
- ["java.nio.file", "Paths", False, "get", "(String,String[])", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
48-
- ["java.nio.file", "Paths", False, "get", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
4942
- ["java.nio.file", "SecureDirectoryStream", True, "deleteDirectory", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
5043
- ["java.nio.file", "SecureDirectoryStream", True, "deleteFile", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
5144
- addsTo:
@@ -63,7 +56,7 @@ extensions:
6356
- ["java.nio.file", "Files", True, "newDirectoryStream", "(Path,DirectoryStream$Filter)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
6457
- ["java.nio.file", "Files", True, "newDirectoryStream", "(Path)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
6558
- ["java.nio.file", "Files", True, "walk", "(Path,FileVisitOption[])", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
66-
- ["java.nio.file", "FileSystem", True, "getPath", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
59+
- ["java.nio.file", "FileSystem", True, "getPath", "(String,String[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
6760
- ["java.nio.file", "FileSystem", True, "getPath", "(String,String[])", "", "Argument[1]", "ReturnValue", "taint", "ai-manual"]
6861
- ["java.nio.file", "FileSystem", True, "getPathMatcher", "(String)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
6962
- ["java.nio.file", "FileSystem", True, "getRootDirectories", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
@@ -76,7 +69,8 @@ extensions:
7669
- ["java.nio.file", "Path", True, "relativize", "(Path)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
7770
- ["java.nio.file", "Path", True, "resolve", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
7871
- ["java.nio.file", "Path", True, "resolve", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
79-
- ["java.nio.file", "Path", True, "resolveSibling", "(String)", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
72+
- ["java.nio.file", "Path", True, "resolveSibling", "", "", "Argument[0]", "ReturnValue", "taint", "ai-manual"]
73+
- ["java.nio.file", "Path", True, "resolveSibling", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
8074
- ["java.nio.file", "Path", True, "toAbsolutePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
8175
- ["java.nio.file", "Path", False, "toFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
8276
- ["java.nio.file", "Path", True, "toString", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]

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

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
/**
2+
* DEPRECATED.
3+
*
24
* Models the different ways to create paths. Either by using `java.io.File`-related APIs or `java.nio.file.Path`-related APIs.
35
*/
46

57
import java
68

7-
/** Models the creation of a path. */
8-
abstract class PathCreation extends Expr {
9+
/** DEPRECATED: Models the creation of a path. */
10+
abstract deprecated class PathCreation extends Expr {
911
/**
1012
* Gets an input that is used in the creation of this path.
1113
* This excludes inputs of type `File` and `Path`.
@@ -14,7 +16,7 @@ abstract class PathCreation extends Expr {
1416
}
1517

1618
/** Models the `java.nio.file.Paths.get` method. */
17-
private class PathsGet extends PathCreation, MethodCall {
19+
deprecated private class PathsGet extends PathCreation, MethodCall {
1820
PathsGet() {
1921
exists(Method m | m = this.getMethod() |
2022
m.getDeclaringType() instanceof TypePaths and
@@ -26,7 +28,7 @@ private class PathsGet extends PathCreation, MethodCall {
2628
}
2729

2830
/** Models the `java.nio.file.FileSystem.getPath` method. */
29-
private class FileSystemGetPath extends PathCreation, MethodCall {
31+
deprecated private class FileSystemGetPath extends PathCreation, MethodCall {
3032
FileSystemGetPath() {
3133
exists(Method m | m = this.getMethod() |
3234
m.getDeclaringType() instanceof TypeFileSystem and
@@ -38,7 +40,7 @@ private class FileSystemGetPath extends PathCreation, MethodCall {
3840
}
3941

4042
/** Models the `new java.io.File(...)` constructor. */
41-
private class FileCreation extends PathCreation, ClassInstanceExpr {
43+
deprecated private class FileCreation extends PathCreation, ClassInstanceExpr {
4244
FileCreation() { this.getConstructedType() instanceof TypeFile }
4345

4446
override Expr getAnInput() {
@@ -49,7 +51,7 @@ private class FileCreation extends PathCreation, ClassInstanceExpr {
4951
}
5052

5153
/** Models the `java.nio.file.Path.resolveSibling` method. */
52-
private class PathResolveSiblingCreation extends PathCreation, MethodCall {
54+
deprecated private class PathResolveSiblingCreation extends PathCreation, MethodCall {
5355
PathResolveSiblingCreation() {
5456
exists(Method m | m = this.getMethod() |
5557
m.getDeclaringType() instanceof TypePath and
@@ -65,7 +67,7 @@ private class PathResolveSiblingCreation extends PathCreation, MethodCall {
6567
}
6668

6769
/** Models the `java.nio.file.Path.resolve` method. */
68-
private class PathResolveCreation extends PathCreation, MethodCall {
70+
deprecated private class PathResolveCreation extends PathCreation, MethodCall {
6971
PathResolveCreation() {
7072
exists(Method m | m = this.getMethod() |
7173
m.getDeclaringType() instanceof TypePath and
@@ -81,7 +83,7 @@ private class PathResolveCreation extends PathCreation, MethodCall {
8183
}
8284

8385
/** Models the `java.nio.file.Path.of` method. */
84-
private class PathOfCreation extends PathCreation, MethodCall {
86+
deprecated private class PathOfCreation extends PathCreation, MethodCall {
8587
PathOfCreation() {
8688
exists(Method m | m = this.getMethod() |
8789
m.getDeclaringType() instanceof TypePath and
@@ -93,7 +95,7 @@ private class PathOfCreation extends PathCreation, MethodCall {
9395
}
9496

9597
/** Models the `new java.io.FileWriter(...)` constructor. */
96-
private class FileWriterCreation extends PathCreation, ClassInstanceExpr {
98+
deprecated private class FileWriterCreation extends PathCreation, ClassInstanceExpr {
9799
FileWriterCreation() { this.getConstructedType().hasQualifiedName("java.io", "FileWriter") }
98100

99101
override Expr getAnInput() {
@@ -104,7 +106,7 @@ private class FileWriterCreation extends PathCreation, ClassInstanceExpr {
104106
}
105107

106108
/** Models the `new java.io.FileReader(...)` constructor. */
107-
private class FileReaderCreation extends PathCreation, ClassInstanceExpr {
109+
deprecated private class FileReaderCreation extends PathCreation, ClassInstanceExpr {
108110
FileReaderCreation() { this.getConstructedType().hasQualifiedName("java.io", "FileReader") }
109111

110112
override Expr getAnInput() {
@@ -115,7 +117,7 @@ private class FileReaderCreation extends PathCreation, ClassInstanceExpr {
115117
}
116118

117119
/** Models the `new java.io.FileInputStream(...)` constructor. */
118-
private class FileInputStreamCreation extends PathCreation, ClassInstanceExpr {
120+
deprecated private class FileInputStreamCreation extends PathCreation, ClassInstanceExpr {
119121
FileInputStreamCreation() {
120122
this.getConstructedType().hasQualifiedName("java.io", "FileInputStream")
121123
}
@@ -128,7 +130,7 @@ private class FileInputStreamCreation extends PathCreation, ClassInstanceExpr {
128130
}
129131

130132
/** Models the `new java.io.FileOutputStream(...)` constructor. */
131-
private class FileOutputStreamCreation extends PathCreation, ClassInstanceExpr {
133+
deprecated private class FileOutputStreamCreation extends PathCreation, ClassInstanceExpr {
132134
FileOutputStreamCreation() {
133135
this.getConstructedType().hasQualifiedName("java.io", "FileOutputStream")
134136
}

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,13 @@ private import semmle.code.java.dataflow.ExternalFlow
88
import semmle.code.java.security.PathSanitizer
99
private import semmle.code.java.security.Sanitizers
1010

11+
/** A sink for tainted path flow configurations. */
12+
abstract class TaintedPathSink extends DataFlow::Node { }
13+
14+
private class DefaultTaintedPathSink extends TaintedPathSink {
15+
DefaultTaintedPathSink() { sinkNode(this, "path-injection") }
16+
}
17+
1118
/**
1219
* A unit class for adding additional taint steps.
1320
*
@@ -55,7 +62,7 @@ private class TaintPreservingUriCtorParam extends Parameter {
5562
module TaintedPathConfig implements DataFlow::ConfigSig {
5663
predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource }
5764

58-
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
65+
predicate isSink(DataFlow::Node sink) { sink instanceof TaintedPathSink }
5966

6067
predicate isBarrier(DataFlow::Node sanitizer) {
6168
sanitizer instanceof SimpleTypeSanitizer or
@@ -76,7 +83,7 @@ module TaintedPathFlow = TaintTracking::Global<TaintedPathConfig>;
7683
module TaintedPathLocalConfig implements DataFlow::ConfigSig {
7784
predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
7885

79-
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
86+
predicate isSink(DataFlow::Node sink) { sink instanceof TaintedPathSink }
8087

8188
predicate isBarrier(DataFlow::Node sanitizer) {
8289
sanitizer instanceof SimpleTypeSanitizer or

‎java/ql/src/Security/CWE/CWE-022/TaintedPath.ql‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,21 +18,7 @@ import semmle.code.java.security.PathCreation
1818
import semmle.code.java.security.TaintedPathQuery
1919
import TaintedPathFlow::PathGraph
2020

21-
/**
22-
* Gets the data-flow node at which to report a path ending at `sink`.
23-
*
24-
* Previously this query flagged alerts exclusively at `PathCreation` sites,
25-
* so to avoid perturbing existing alerts, where a `PathCreation` exists we
26-
* continue to report there; otherwise we report directly at `sink`.
27-
*/
28-
DataFlow::Node getReportingNode(DataFlow::Node sink) {
29-
TaintedPathFlow::flowTo(sink) and
30-
if exists(PathCreation pc | pc.getAnInput() = sink.asExpr())
31-
then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr())
32-
else result = sink
33-
}
34-
3521
from TaintedPathFlow::PathNode source, TaintedPathFlow::PathNode sink
3622
where TaintedPathFlow::flowPath(source, sink)
37-
select getReportingNode(sink.getNode()), source, sink, "This path depends on a $@.",
38-
source.getNode(), "user-provided value"
23+
select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
24+
"user-provided value"

‎java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql‎

Lines changed: 2 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,21 +18,7 @@ import semmle.code.java.security.PathCreation
1818
import semmle.code.java.security.TaintedPathQuery
1919
import TaintedPathLocalFlow::PathGraph
2020

21-
/**
22-
* Gets the data-flow node at which to report a path ending at `sink`.
23-
*
24-
* Previously this query flagged alerts exclusively at `PathCreation` sites,
25-
* so to avoid perturbing existing alerts, where a `PathCreation` exists we
26-
* continue to report there; otherwise we report directly at `sink`.
27-
*/
28-
DataFlow::Node getReportingNode(DataFlow::Node sink) {
29-
TaintedPathLocalFlow::flowTo(sink) and
30-
if exists(PathCreation pc | pc.getAnInput() = sink.asExpr())
31-
then result.asExpr() = any(PathCreation pc | pc.getAnInput() = sink.asExpr())
32-
else result = sink
33-
}
34-
3521
from TaintedPathLocalFlow::PathNode source, TaintedPathLocalFlow::PathNode sink
3622
where TaintedPathLocalFlow::flowPath(source, sink)
37-
select getReportingNode(sink.getNode()), source, sink, "This path depends on a $@.",
38-
source.getNode(), "user-provided value"
23+
select sink.getNode(), source, sink, "This path depends on a $@.", source.getNode(),
24+
"user-provided value"

‎java/ql/src/change-notes/2023-04-20-path-injection-precision-improved.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: majorAnalysis
3+
---
4+
* The sinks of the queries `java/path-injection` and `java/path-injection-local` have been reworked. Path creation sinks have been converted to summaries instead, while sinks now are actual file read/write operations only. This has reduced the false positive ratio of both queries.

‎java/ql/src/experimental/Security/CWE/CWE-073/FilePathInjection.ql‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ import java
1616
import semmle.code.java.dataflow.TaintTracking
1717
import semmle.code.java.dataflow.ExternalFlow
1818
import semmle.code.java.dataflow.FlowSources
19+
<<<<<<< HEAD
20+
=======
21+
import semmle.code.java.security.TaintedPathQuery
22+
>>>>>>> 9e469c9c32 (Migrate path injection sinks to MaD)
1923
import JFinalController
2024
import semmle.code.java.security.PathSanitizer
2125
private import semmle.code.java.security.Sanitizers
@@ -52,7 +56,11 @@ module InjectFilePathConfig implements DataFlow::ConfigSig {
5256
predicate isSource(DataFlow::Node source) { source instanceof ThreatModelFlowSource }
5357

5458
predicate isSink(DataFlow::Node sink) {
59+
<<<<<<< HEAD
5560
sinkNode(sink, "path-injection") and
61+
=======
62+
sink instanceof TaintedPathSink and
63+
>>>>>>> 9e469c9c32 (Migrate path injection sinks to MaD)
5664
not sink instanceof NormalizedPathNode
5765
}
5866

‎java/ql/test/library-tests/pathcreation/PathCreation.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
WARNING: Type PathCreation has been deprecated and may be removed in future (PathCreation.ql:4,6-18)
12
| PathCreation.java:13:18:13:32 | new File(...) | PathCreation.java:13:27:13:31 | "dir" |
23
| PathCreation.java:14:19:14:40 | new File(...) | PathCreation.java:14:28:14:32 | "dir" |
34
| PathCreation.java:14:19:14:40 | new File(...) | PathCreation.java:14:35:14:39 | "sub" |

0 commit comments

Comments
 (0)