Skip to content

Commit 527fe52

Browse files
atorralbaatorralba
committed
Add PathCreation.qll sinks to models-as-data
The old PathCreation sinks can't be removed because doing so would cause alert wobble in the path injection queries. See their getReportingNode predicates.
1 parent 84a7b3c commit 527fe52

File tree

10 files changed

+36
-17
lines changed

10 files changed

+36
-17
lines changed

‎java/ql/lib/change-notes/2023-05-19-path-injection-sinks-mad.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Path creation sinks modeled in `PathCreation.qll` have been added to the models-as-data sink kinds `create-file` and `read-file`.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ 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
610
- ["java.io", "File", True, "createTempFile", "(String,String,File)", "", "Argument[2]", "path-injection", "ai-manual"]
711
- ["java.io", "File", True, "renameTo", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
812
- ["java.io", "FileInputStream", True, "FileInputStream", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
@@ -11,6 +15,7 @@ extensions:
1115
- ["java.io", "FileOutputStream", False, "write", "", "", "Argument[0]", "file-content-store", "manual"]
1216
- ["java.io", "FileReader", True, "FileReader", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
1317
- ["java.io", "FileReader", True, "FileReader", "(String)", "", "Argument[0]", "path-injection", "ai-manual"]
18+
- ["java.io", "FileReader", True, "FileReader", "(String,Charset)", "", "Argument[0]", "path-injection", "manual"]
1419
- ["java.io", "FileSystem", True, "createDirectory", "(File)", "", "Argument[0]", "path-injection", "ai-manual"]
1520
- ["java.io", "FileWriter", False, "FileWriter", "", "", "Argument[0]", "path-injection", "manual"]
1621
- ["java.io", "PrintStream", False, "PrintStream", "(File)", "", "Argument[0]", "path-injection", "manual"]

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@ extensions:
33
pack: codeql/java-all
44
extensible: sinkModel
55
data:
6-
- ["java.nio.file", "Files", False, "copy", "(Path,OutputStream)", "", "Argument[0]", "path-injection", "manual"]
7-
- ["java.nio.file", "Files", False, "copy", "(Path,Path,CopyOption[])", "", "Argument[0]", "path-injection", "manual"]
8-
- ["java.nio.file", "Files", False, "copy", "(Path,Path,CopyOption[])", "", "Argument[1]", "path-injection", "manual"]
6+
- ["java.nio.file", "Files", False, "copy", "", "", "Argument[0]", "path-injection", "manual"]
97
- ["java.nio.file", "Files", False, "copy", "(InputStream,Path,CopyOption[])", "", "Argument[0]", "file-content-store", "manual"]
10-
- ["java.nio.file", "Files", False, "copy", "(InputStream,Path,CopyOption[])", "", "Argument[1]", "path-injection", "manual"]
8+
- ["java.nio.file", "Files", False, "copy", "", "", "Argument[1]", "path-injection", "manual"]
119
- ["java.nio.file", "Files", False, "createDirectories", "", "", "Argument[0]", "path-injection", "manual"]
1210
- ["java.nio.file", "Files", False, "createDirectory", "", "", "Argument[0]", "path-injection", "manual"]
1311
- ["java.nio.file", "Files", False, "createFile", "", "", "Argument[0]", "path-injection", "manual"]
@@ -40,6 +38,13 @@ extensions:
4038
- ["java.nio.file", "Files", True, "delete", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
4139
- ["java.nio.file", "Files", True, "newInputStream", "(Path,OpenOption[])", "", "Argument[0]", "path-injection", "ai-manual"]
4240
- ["java.nio.file", "Files", True, "newOutputStream", "(Path,OpenOption[])", "", "Argument[0]", "path-injection", "ai-manual"]
41+
- ["java.nio.file", "FileSystem", False, "getPath", "", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
42+
- ["java.nio.file", "Path", False, "of", "(String,String[])", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
43+
- ["java.nio.file", "Path", False, "of", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
44+
- ["java.nio.file", "Path", False, "resolve", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
45+
- ["java.nio.file", "Path", False, "resolveSibling", "(String)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
46+
- ["java.nio.file", "Paths", False, "get", "(String,String[])", "", "Argument[0..1]", "path-injection", "manual"] # old PathCreation
47+
- ["java.nio.file", "Paths", False, "get", "(URI)", "", "Argument[0]", "path-injection", "manual"] # old PathCreation
4348
- ["java.nio.file", "SecureDirectoryStream", True, "deleteDirectory", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
4449
- ["java.nio.file", "SecureDirectoryStream", True, "deleteFile", "(Path)", "", "Argument[0]", "path-injection", "ai-manual"]
4550
- addsTo:

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ extensions:
66
- ["java.nio", "ByteBuffer", False, "array", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"]
77
- ["java.nio", "ByteBuffer", False, "get", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
88
- ["java.nio", "ByteBuffer", False, "wrap", "(byte[])", "", "Argument[0]", "ReturnValue", "taint", "manual"]
9+
- ["java.nio", "Paths", False, "get", "(URI)", "", "Argument[0]", "ReturnValue", "taint", "manual"] # old PathCreation
910

1011
- addsTo:
1112
pack: codeql/java-all

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

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import semmle.code.java.frameworks.Networking
55
import semmle.code.java.dataflow.DataFlow
66
import semmle.code.java.dataflow.FlowSources
77
private import semmle.code.java.dataflow.ExternalFlow
8-
import semmle.code.java.security.PathCreation
98
import semmle.code.java.security.PathSanitizer
109

1110
/**
@@ -55,11 +54,7 @@ private class TaintPreservingUriCtorParam extends Parameter {
5554
module TaintedPathConfig implements DataFlow::ConfigSig {
5655
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
5756

58-
predicate isSink(DataFlow::Node sink) {
59-
sink.asExpr() = any(PathCreation p).getAnInput()
60-
or
61-
sinkNode(sink, "path-injection")
62-
}
57+
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
6358

6459
predicate isBarrier(DataFlow::Node sanitizer) {
6560
sanitizer.getType() instanceof BoxedType or
@@ -82,11 +77,7 @@ module TaintedPathFlow = TaintTracking::Global<TaintedPathConfig>;
8277
module TaintedPathLocalConfig implements DataFlow::ConfigSig {
8378
predicate isSource(DataFlow::Node source) { source instanceof LocalUserInput }
8479

85-
predicate isSink(DataFlow::Node sink) {
86-
sink.asExpr() = any(PathCreation p).getAnInput()
87-
or
88-
sinkNode(sink, "path-injection")
89-
}
80+
predicate isSink(DataFlow::Node sink) { sinkNode(sink, "path-injection") }
9081

9182
predicate isBarrier(DataFlow::Node sanitizer) {
9283
sanitizer.getType() instanceof BoxedType or

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
*/
1515

1616
import java
17+
import semmle.code.java.security.PathCreation
1718
import semmle.code.java.security.TaintedPathQuery
1819
import TaintedPathFlow::PathGraph
1920

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
*/
1515

1616
import java
17+
import semmle.code.java.security.PathCreation
1718
import semmle.code.java.security.TaintedPathQuery
1819
import TaintedPathLocalFlow::PathGraph
1920

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import java
1616
import semmle.code.java.dataflow.TaintTracking
1717
import semmle.code.java.dataflow.ExternalFlow
1818
import semmle.code.java.dataflow.FlowSources
19-
import semmle.code.java.security.PathCreation
2019
import JFinalController
2120
import semmle.code.java.security.PathSanitizer
2221
import InjectFilePathFlow::PathGraph
@@ -52,7 +51,7 @@ module InjectFilePathConfig implements DataFlow::ConfigSig {
5251
predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
5352

5453
predicate isSink(DataFlow::Node sink) {
55-
sink.asExpr() = any(PathCreation p).getAnInput() and
54+
sinkNode(sink, "path-injection") and
5655
not sink instanceof NormalizedPathNode
5756
}
5857

‎java/ql/test/experimental/query-tests/security/CWE-073/FilePathInjection.expected

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,30 @@ edges
22
| FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath |
33
| FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath |
44
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath |
5+
| FilePathInjection.java:177:50:177:58 | file : File | FilePathInjection.java:182:30:182:33 | file |
56
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath |
7+
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath : String |
8+
| FilePathInjection.java:209:15:209:32 | new File(...) : File | FilePathInjection.java:217:19:217:22 | file : File |
9+
| FilePathInjection.java:209:24:209:31 | filePath : String | FilePathInjection.java:209:15:209:32 | new File(...) : File |
10+
| FilePathInjection.java:217:19:217:22 | file : File | FilePathInjection.java:177:50:177:58 | file : File |
611
nodes
712
| FilePathInjection.java:21:21:21:34 | getPara(...) : String | semmle.label | getPara(...) : String |
813
| FilePathInjection.java:26:47:26:59 | finalFilePath | semmle.label | finalFilePath |
914
| FilePathInjection.java:64:21:64:34 | getPara(...) : String | semmle.label | getPara(...) : String |
1015
| FilePathInjection.java:72:47:72:59 | finalFilePath | semmle.label | finalFilePath |
1116
| FilePathInjection.java:87:21:87:34 | getPara(...) : String | semmle.label | getPara(...) : String |
1217
| FilePathInjection.java:95:47:95:59 | finalFilePath | semmle.label | finalFilePath |
18+
| FilePathInjection.java:177:50:177:58 | file : File | semmle.label | file : File |
19+
| FilePathInjection.java:182:30:182:33 | file | semmle.label | file |
1320
| FilePathInjection.java:205:17:205:44 | getParameter(...) : String | semmle.label | getParameter(...) : String |
21+
| FilePathInjection.java:209:15:209:32 | new File(...) : File | semmle.label | new File(...) : File |
1422
| FilePathInjection.java:209:24:209:31 | filePath | semmle.label | filePath |
23+
| FilePathInjection.java:209:24:209:31 | filePath : String | semmle.label | filePath : String |
24+
| FilePathInjection.java:217:19:217:22 | file : File | semmle.label | file : File |
1525
subpaths
1626
#select
1727
| FilePathInjection.java:26:47:26:59 | finalFilePath | FilePathInjection.java:21:21:21:34 | getPara(...) : String | FilePathInjection.java:26:47:26:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:21:21:21:34 | getPara(...) | user-provided value |
1828
| FilePathInjection.java:72:47:72:59 | finalFilePath | FilePathInjection.java:64:21:64:34 | getPara(...) : String | FilePathInjection.java:72:47:72:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:64:21:64:34 | getPara(...) | user-provided value |
1929
| FilePathInjection.java:95:47:95:59 | finalFilePath | FilePathInjection.java:87:21:87:34 | getPara(...) : String | FilePathInjection.java:95:47:95:59 | finalFilePath | External control of file name or path due to $@. | FilePathInjection.java:87:21:87:34 | getPara(...) | user-provided value |
30+
| FilePathInjection.java:182:30:182:33 | file | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:182:30:182:33 | file | External control of file name or path due to $@. | FilePathInjection.java:205:17:205:44 | getParameter(...) | user-provided value |
2031
| FilePathInjection.java:209:24:209:31 | filePath | FilePathInjection.java:205:17:205:44 | getParameter(...) : String | FilePathInjection.java:209:24:209:31 | filePath | External control of file name or path due to $@. | FilePathInjection.java:205:17:205:44 | getParameter(...) | user-provided value |

‎java/ql/test/query-tests/Telemetry/SupportedExternalSinks/SupportedExternalSinks.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
1+
| java.io.File#File(String) | 1 |
12
| java.io.FileWriter#FileWriter(File) | 1 |
23
| java.net.URL#openStream() | 1 |

0 commit comments

Comments
 (0)