diff --git a/java/ql/lib/change-notes/2022-04-26-additional-file-taint-flow.md b/java/ql/lib/change-notes/2022-04-26-additional-file-taint-flow.md new file mode 100644 index 000000000000..bd9312200457 --- /dev/null +++ b/java/ql/lib/change-notes/2022-04-26-additional-file-taint-flow.md @@ -0,0 +1,8 @@ +--- +category: minorAnalysis +--- + * Add taint models for the following `File` methods: + * `File::getAbsoluteFile` + * `File::getCanonicalFile` + * `File::getAbsolutePath` + * `File::getCanonicalPath` \ No newline at end of file diff --git a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll index 28cfced918fe..c1fbbdb275e0 100644 --- a/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll +++ b/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll @@ -298,8 +298,12 @@ private predicate summaryModelCsv(string row) { "java.net;URI;false;toURL;;;Argument[-1];ReturnValue;taint", "java.net;URI;false;toString;;;Argument[-1];ReturnValue;taint", "java.net;URI;false;toAsciiString;;;Argument[-1];ReturnValue;taint", - "java.io;File;false;toURI;;;Argument[-1];ReturnValue;taint", - "java.io;File;false;toPath;;;Argument[-1];ReturnValue;taint", + "java.io;File;true;toURI;;;Argument[-1];ReturnValue;taint", + "java.io;File;true;toPath;;;Argument[-1];ReturnValue;taint", + "java.io;File;true;getAbsoluteFile;;;Argument[-1];ReturnValue;taint", + "java.io;File;true;getCanonicalFile;;;Argument[-1];ReturnValue;taint", + "java.io;File;true;getAbsolutePath;;;Argument[-1];ReturnValue;taint", + "java.io;File;true;getCanonicalPath;;;Argument[-1];ReturnValue;taint", "java.nio;ByteBuffer;false;array;();;Argument[-1];ReturnValue;taint", "java.nio.file;Path;false;toFile;;;Argument[-1];ReturnValue;taint", "java.io;BufferedReader;true;readLine;;;Argument[-1];ReturnValue;taint", diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index 01d60f1062ed..16e8bb72c930 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -134,16 +134,6 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf source.asExpr() instanceof ExprSystemGetPropertyTempDirTainted } - /** - * Find dataflow from the temp directory system property to the `File` constructor. - * Examples: - * - `new File(System.getProperty("java.io.tmpdir"))` - * - `new File(new File(System.getProperty("java.io.tmpdir")), "/child")` - */ - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - isAdditionalFileTaintStep(node1, node2) - } - override predicate isSink(DataFlow::Node sink) { sink instanceof FileCreationSink and not any(TempDirSystemGetPropertyDirectlyToMkdirConfig config).hasFlowTo(sink) diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll index 1984f16a58c0..cb2d8dd7875e 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll +++ b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll @@ -35,32 +35,6 @@ predicate isFileConstructorArgument(Expr expSource, Expr exprDest, int paramCoun ) } -/** - * A `File` method where the temporary directory is still part of the root path. - */ -private class TaintFollowingFileMethod extends Method { - TaintFollowingFileMethod() { - this.getDeclaringType() instanceof TypeFile and - this.hasName(["getAbsoluteFile", "getCanonicalFile"]) - } -} - -private predicate isTaintPropagatingFileTransformation(Expr expSource, Expr exprDest) { - exists(MethodAccess fileMethodAccess | - fileMethodAccess.getMethod() instanceof TaintFollowingFileMethod and - fileMethodAccess.getQualifier() = expSource and - fileMethodAccess = exprDest - ) -} - -/** - * Holds if taint should propagate from `node1` to `node2` across some file creation or transformation operation. - * For example, `taintedFile.getCanonicalFile()` is itself tainted. - */ -predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr()) -} - /** * A method call to `java.io.File::setReadable`. */ diff --git a/java/ql/test/library-tests/dataflow/taint/B.java b/java/ql/test/library-tests/dataflow/taint/B.java index 6e33111ad19c..94a09dd915f7 100644 --- a/java/ql/test/library-tests/dataflow/taint/B.java +++ b/java/ql/test/library-tests/dataflow/taint/B.java @@ -11,7 +11,7 @@ public class B { public static void sink(Object o) { } - public static void maintest() throws java.io.UnsupportedEncodingException, java.net.MalformedURLException { + public static void maintest() throws java.io.UnsupportedEncodingException, java.net.MalformedURLException, java.io.IOException { String[] args = taint(); // tainted - access to main args String[] aaaargs = args; @@ -156,6 +156,12 @@ public static void maintest() throws java.io.UnsupportedEncodingException, java. // Tainted File to Path to File sink(new java.io.File(s).toPath().toFile()); + // Tainted file to absolute file + sink(new java.io.File(s).getAbsoluteFile()); + + // Tainted file to canonical file + sink(new java.io.File(s).getCanonicalFile()); + return; } diff --git a/java/ql/test/library-tests/dataflow/taint/test.expected b/java/ql/test/library-tests/dataflow/taint/test.expected index 158b7977dba4..451248e18c05 100644 --- a/java/ql/test/library-tests/dataflow/taint/test.expected +++ b/java/ql/test/library-tests/dataflow/taint/test.expected @@ -41,6 +41,8 @@ | B.java:15:21:15:27 | taint(...) | B.java:151:10:151:44 | toURL(...) | | B.java:15:21:15:27 | taint(...) | B.java:154:10:154:37 | toPath(...) | | B.java:15:21:15:27 | taint(...) | B.java:157:10:157:46 | toFile(...) | +| B.java:15:21:15:27 | taint(...) | B.java:160:10:160:46 | getAbsoluteFile(...) | +| B.java:15:21:15:27 | taint(...) | B.java:163:10:163:47 | getCanonicalFile(...) | | CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:8:12:8:14 | seq | | CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:11:12:11:21 | seqFromSeq | | CharSeq.java:7:26:7:32 | taint(...) | CharSeq.java:14:12:14:24 | stringFromSeq | diff --git a/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected b/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected index 0791e49c71b0..03c431822b5c 100644 --- a/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected +++ b/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected @@ -8,9 +8,11 @@ edges | Test.java:50:29:50:94 | new File(...) : File | Test.java:53:63:53:74 | tempDirChild | | Test.java:50:38:50:83 | new File(...) : File | Test.java:50:29:50:94 | new File(...) : File | | Test.java:50:47:50:82 | getProperty(...) : String | Test.java:50:38:50:83 | new File(...) : File | -| Test.java:61:24:61:69 | new File(...) : File | Test.java:64:63:64:69 | tempDir | +| Test.java:61:24:61:69 | new File(...) : File | Test.java:61:24:61:88 | getCanonicalFile(...) : File | +| Test.java:61:24:61:88 | getCanonicalFile(...) : File | Test.java:64:63:64:69 | tempDir | | Test.java:61:33:61:68 | getProperty(...) : String | Test.java:61:24:61:69 | new File(...) : File | -| Test.java:75:24:75:69 | new File(...) : File | Test.java:78:63:78:69 | tempDir | +| Test.java:75:24:75:69 | new File(...) : File | Test.java:75:24:75:87 | getAbsoluteFile(...) : File | +| Test.java:75:24:75:87 | getAbsoluteFile(...) : File | Test.java:78:63:78:69 | tempDir | | Test.java:75:33:75:68 | getProperty(...) : String | Test.java:75:24:75:69 | new File(...) : File | | Test.java:110:29:110:84 | new File(...) : File | Test.java:113:9:113:20 | tempDirChild | | Test.java:110:38:110:73 | getProperty(...) : String | Test.java:110:29:110:84 | new File(...) : File | @@ -68,9 +70,11 @@ nodes | Test.java:50:47:50:82 | getProperty(...) : String | semmle.label | getProperty(...) : String | | Test.java:53:63:53:74 | tempDirChild | semmle.label | tempDirChild | | Test.java:61:24:61:69 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:61:24:61:88 | getCanonicalFile(...) : File | semmle.label | getCanonicalFile(...) : File | | Test.java:61:33:61:68 | getProperty(...) : String | semmle.label | getProperty(...) : String | | Test.java:64:63:64:69 | tempDir | semmle.label | tempDir | | Test.java:75:24:75:69 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:75:24:75:87 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File | | Test.java:75:33:75:68 | getProperty(...) : String | semmle.label | getProperty(...) : String | | Test.java:78:63:78:69 | tempDir | semmle.label | tempDir | | Test.java:97:24:97:65 | createTempDir(...) | semmle.label | createTempDir(...) |