Skip to content

Commit 86516b1

Browse files
aschackmullaschackmull
authored
Merge pull request #8884 from JLLeitschuh/feat/JLL/additional-file-taint-flow
Java: Add additional `File` taint value flow models
2 parents 1d44694 + c8e0d7f commit 86516b1

File tree

7 files changed

+29
-41
lines changed

7 files changed

+29
-41
lines changed

‎java/ql/lib/change-notes/2022-04-26-additional-file-taint-flow.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Add taint models for the following `File` methods:
5+
* `File::getAbsoluteFile`
6+
* `File::getCanonicalFile`
7+
* `File::getAbsolutePath`
8+
* `File::getCanonicalPath`

‎java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -300,8 +300,12 @@ private predicate summaryModelCsv(string row) {
300300
"java.net;URI;false;toURL;;;Argument[-1];ReturnValue;taint",
301301
"java.net;URI;false;toString;;;Argument[-1];ReturnValue;taint",
302302
"java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint",
303-
"java.io;File;false;toURI;;;Argument[-1];ReturnValue;taint",
304-
"java.io;File;false;toPath;;;Argument[-1];ReturnValue;taint",
303+
"java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint",
304+
"java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint",
305+
"java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint",
306+
"java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint",
307+
"java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint",
308+
"java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint",
305309
"java.nio;ByteBuffer;false;array;();;Argument[-1];ReturnValue;taint",
306310
"java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint",
307311
"java.io;BufferedReader;true;readLine;;;Argument[-1];ReturnValue;taint",

‎java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -134,16 +134,6 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
134134
source.asExpr() instanceof ExprSystemGetPropertyTempDirTainted
135135
}
136136

137-
/**
138-
* Find dataflow from the temp directory system property to the `File` constructor.
139-
* Examples:
140-
* - `new File(System.getProperty("java.io.tmpdir"))`
141-
* - `new File(new File(System.getProperty("java.io.tmpdir")), "/child")`
142-
*/
143-
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
144-
isAdditionalFileTaintStep(node1, node2)
145-
}
146-
147137
override predicate isSink(DataFlow::Node sink) {
148138
sink instanceof FileCreationSink and
149139
not any(TempDirSystemGetPropertyDirectlyToMkdirConfig config).hasFlowTo(sink)

‎java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -35,32 +35,6 @@ predicate isFileConstructorArgument(Expr expSource, Expr exprDest, int paramCoun
3535
)
3636
}
3737

38-
/**
39-
* A `File` method where the temporary directory is still part of the root path.
40-
*/
41-
private class TaintFollowingFileMethod extends Method {
42-
TaintFollowingFileMethod() {
43-
this.getDeclaringType() instanceof TypeFile and
44-
this.hasName(["getAbsoluteFile", "getCanonicalFile"])
45-
}
46-
}
47-
48-
private predicate isTaintPropagatingFileTransformation(Expr expSource, Expr exprDest) {
49-
exists(MethodAccess fileMethodAccess |
50-
fileMethodAccess.getMethod() instanceof TaintFollowingFileMethod and
51-
fileMethodAccess.getQualifier() = expSource and
52-
fileMethodAccess = exprDest
53-
)
54-
}
55-
56-
/**
57-
* Holds if taint should propagate from `node1` to `node2` across some file creation or transformation operation.
58-
* For example, `taintedFile.getCanonicalFile()` is itself tainted.
59-
*/
60-
predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
61-
isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr())
62-
}
63-
6438
/**
6539
* A method call to `java.io.File::setReadable`.
6640
*/

‎java/ql/test/library-tests/dataflow/taint/B.java

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ public class B {
1111

1212
public static void sink(Object o) { }
1313

14-
public static void maintest() throws java.io.UnsupportedEncodingException, java.net.MalformedURLException {
14+
public static void maintest() throws java.io.UnsupportedEncodingException, java.net.MalformedURLException, java.io.IOException {
1515
String[] args = taint();
1616
// tainted - access to main args
1717
String[] aaaargs = args;
@@ -156,6 +156,12 @@ public static void maintest() throws java.io.UnsupportedEncodingException, java.
156156
// Tainted File to Path to File
157157
sink(new java.io.File(s).toPath().toFile());
158158

159+
// Tainted file to absolute file
160+
sink(new java.io.File(s).getAbsoluteFile());
161+
162+
// Tainted file to canonical file
163+
sink(new java.io.File(s).getCanonicalFile());
164+
159165
return;
160166
}
161167

‎java/ql/test/library-tests/dataflow/taint/test.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@
4141
| B.java:15:21:15:27 | taint(...) | B.java:151:10:151:44 | toURL(...) |
4242
| B.java:15:21:15:27 | taint(...) | B.java:154:10:154:37 | toPath(...) |
4343
| B.java:15:21:15:27 | taint(...) | B.java:157:10:157:46 | toFile(...) |
44+
| B.java:15:21:15:27 | taint(...) | B.java:160:10:160:46 | getAbsoluteFile(...) |
45+
| B.java:15:21:15:27 | taint(...) | B.java:163:10:163:47 | getCanonicalFile(...) |
4446
| CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:8:12:8:14 | seq |
4547
| CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:11:12:11:21 | seqFromSeq |
4648
| CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:14:12:14:24 | stringFromSeq |

‎java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@ edges
88
| Test.java:50:29:50:94 | new File(...) : File | Test.java:53:63:53:74 | tempDirChild |
99
| Test.java:50:38:50:83 | new File(...) : File | Test.java:50:29:50:94 | new File(...) : File |
1010
| Test.java:50:47:50:82 | getProperty(...) : String | Test.java:50:38:50:83 | new File(...) : File |
11-
| Test.java:61:24:61:69 | new File(...) : File | Test.java:64:63:64:69 | tempDir |
11+
| Test.java:61:24:61:69 | new File(...) : File | Test.java:61:24:61:88 | getCanonicalFile(...) : File |
12+
| Test.java:61:24:61:88 | getCanonicalFile(...) : File | Test.java:64:63:64:69 | tempDir |
1213
| Test.java:61:33:61:68 | getProperty(...) : String | Test.java:61:24:61:69 | new File(...) : File |
13-
| Test.java:75:24:75:69 | new File(...) : File | Test.java:78:63:78:69 | tempDir |
14+
| Test.java:75:24:75:69 | new File(...) : File | Test.java:75:24:75:87 | getAbsoluteFile(...) : File |
15+
| Test.java:75:24:75:87 | getAbsoluteFile(...) : File | Test.java:78:63:78:69 | tempDir |
1416
| Test.java:75:33:75:68 | getProperty(...) : String | Test.java:75:24:75:69 | new File(...) : File |
1517
| Test.java:110:29:110:84 | new File(...) : File | Test.java:113:9:113:20 | tempDirChild |
1618
| Test.java:110:38:110:73 | getProperty(...) : String | Test.java:110:29:110:84 | new File(...) : File |
@@ -68,9 +70,11 @@ nodes
6870
| Test.java:50:47:50:82 | getProperty(...) : String | semmle.label | getProperty(...) : String |
6971
| Test.java:53:63:53:74 | tempDirChild | semmle.label | tempDirChild |
7072
| Test.java:61:24:61:69 | new File(...) : File | semmle.label | new File(...) : File |
73+
| Test.java:61:24:61:88 | getCanonicalFile(...) : File | semmle.label | getCanonicalFile(...) : File |
7174
| Test.java:61:33:61:68 | getProperty(...) : String | semmle.label | getProperty(...) : String |
7275
| Test.java:64:63:64:69 | tempDir | semmle.label | tempDir |
7376
| Test.java:75:24:75:69 | new File(...) : File | semmle.label | new File(...) : File |
77+
| Test.java:75:24:75:87 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File |
7478
| Test.java:75:33:75:68 | getProperty(...) : String | semmle.label | getProperty(...) : String |
7579
| Test.java:78:63:78:69 | tempDir | semmle.label | tempDir |
7680
| Test.java:97:24:97:65 | createTempDir(...) | semmle.label | createTempDir(...) |

0 commit comments

Comments
 (0)