From ba727af7c4e4760ab9d7e07ab16c1ea6c0f40c20 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 14 Oct 2020 09:29:05 -0400 Subject: [PATCH 01/28] Java: CWE-378: Temp Directory Hijacking Race Condition Vulnerability --- .../CWE-378/TempDirHijackingVulnerability.ql | 75 +++++++++++++++++++ .../tests/TempDirHijackingVulnerability.qlref | 1 + .../security/CWE-378/semmle/tests/Test.java | 39 ++++++++++ 3 files changed, 115 insertions(+) create mode 100644 java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql create mode 100644 java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.qlref create mode 100644 java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql new file mode 100644 index 000000000000..543d829ffa4b --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -0,0 +1,75 @@ +/** + * @name Temporary Directory Hijacking Vulnerability disclosure + * @description Detect temporary directory hijack vulnerability + * @kind path-problem + * @problem.severity error + * @precision very-high + * @id java/temp-directory-hijacking + */ + +import java +import semmle.code.java.dataflow.FlowSources +import DataFlow::PathGraph + +/** + * All `java.io.File::createTempFile` methods. + */ +private class MethodFileCreateTempFile extends Method { + MethodFileCreateTempFile() { + this.getDeclaringType() instanceof TypeFile and + this.hasName("createTempFile") + } +} + +private class MethodFileMkdir extends Method { + MethodFileMkdir() { + getDeclaringType() instanceof TypeFile and + hasName("mkdir") + or + hasName("mkdirs") + } +} + +private class MethodFileDelete extends Method { + MethodFileDelete() { + getDeclaringType() instanceof TypeFile and + hasName("delete") + } +} + +private class DeleteFileNode extends DataFlow::Node { + DeleteFileNode() { + exists(MethodAccess ma | + asExpr() = ma.getQualifier() and + ma.getMethod() instanceof MethodFileDelete + ) + } +} + +private class TempDirHijackingConfig extends TaintTracking::Configuration { + TempDirHijackingConfig() { this = "TempDirHijackingConfig" } + + override predicate isSource(DataFlow::Node source) { + exists(MethodAccess ma | + ma.getMethod() instanceof MethodFileCreateTempFile and + DataFlow::localFlow(DataFlow::exprNode(ma), source) + ) and + source instanceof DeleteFileNode + } + + override predicate isSink(DataFlow::Node sink) { + exists(MethodAccess ma | + ma.getMethod() instanceof MethodFileMkdir and + ma.getQualifier() = sink.asExpr() + ) + } +} + +from + DataFlow::PathNode source, DataFlow::PathNode sink, TempDirHijackingConfig conf, + IfStmt ifStmnt, MethodAccess ma +where + conf.hasFlowPath(source, sink) and + not DataFlow::localFlow(DataFlow::exprNode(ma), DataFlow::exprNode(ifStmnt.getCondition())) and + ma.getQualifier() = sink.getNode().asExpr() +select source.getNode(), source, sink, "Blah %s", sink.getNode() diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.qlref b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.qlref new file mode 100644 index 000000000000..9b0d1cbebdd5 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-378/TempDirHijackingVulnerability.ql diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java new file mode 100644 index 000000000000..581cc3c40077 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -0,0 +1,39 @@ +import java.io.File; + +public class Test { + + static File vulnerable() { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + temp.mkdir(); + return temp; + } + + static File notVulnerableToHijacking() { + File temp = File.createTempFile("test", "directory"); + temp.mkdir(); + return temp; + } + + static File safe() { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + if (temp.mkdir()) { + return temp; + } else { + throw new RuntimeException("Failed to create directory"); + } + } + + static File safe2() { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + boolean didMkdirs = temp.mkdirs(); + if (didMkdirs) { + return temp; + } else { + throw new RuntimeException("Failed to create directory"); + } + } + +} From aea017094c89a0c361758928dcb2e6dee7f6c5db Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 14 Oct 2020 19:09:22 -0400 Subject: [PATCH 02/28] Better temp file deletion and file creation tracking --- .../CWE-378/TempDirHijackingVulnerability.ql | 51 ++++++++++++------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 543d829ffa4b..fe01f387539e 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -37,26 +37,31 @@ private class MethodFileDelete extends Method { } } -private class DeleteFileNode extends DataFlow::Node { - DeleteFileNode() { - exists(MethodAccess ma | - asExpr() = ma.getQualifier() and - ma.getMethod() instanceof MethodFileDelete - ) - } +predicate isDeleteFileExpr(Expr expr) { + exists(MethodAccess ma | + expr = ma.getQualifier() and + ma.getMethod() instanceof MethodFileDelete + ) } -private class TempDirHijackingConfig extends TaintTracking::Configuration { - TempDirHijackingConfig() { this = "TempDirHijackingConfig" } +private class TempDirHijackingToDeleteConfig extends DataFlow::Configuration { + TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" } override predicate isSource(DataFlow::Node source) { exists(MethodAccess ma | ma.getMethod() instanceof MethodFileCreateTempFile and - DataFlow::localFlow(DataFlow::exprNode(ma), source) - ) and - source instanceof DeleteFileNode + source.asExpr() = ma + ) } + override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) } +} + +private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration { + TempDirHijackingFromDeleteConfig() { this = "TempDirHijackingFromDeleteConfig" } + + override predicate isSource(DataFlow::Node source) { isDeleteFileExpr(source.asExpr()) } + override predicate isSink(DataFlow::Node sink) { exists(MethodAccess ma | ma.getMethod() instanceof MethodFileMkdir and @@ -65,11 +70,21 @@ private class TempDirHijackingConfig extends TaintTracking::Configuration { } } +predicate isSinkConstrainedByIfCheck(DataFlow2::Node sink) { + exists(MethodAccess ma, IfStmt ifStmt | + sink.asExpr() = ma.getQualifier() and + // Data flow from the return value from `mkdirs` into an 'if' check + DataFlow2::localExprFlow(ma, ifStmt.getCondition()) + ) +} + from - DataFlow::PathNode source, DataFlow::PathNode sink, TempDirHijackingConfig conf, - IfStmt ifStmnt, MethodAccess ma + DataFlow::Node source, DataFlow::Node deleteCheckpoint, DataFlow2::PathNode deleteCheckpoint2, + DataFlow2::PathNode sink, TempDirHijackingToDeleteConfig toDeleteConfig, + TempDirHijackingFromDeleteConfig fromDeleteConfig where - conf.hasFlowPath(source, sink) and - not DataFlow::localFlow(DataFlow::exprNode(ma), DataFlow::exprNode(ifStmnt.getCondition())) and - ma.getQualifier() = sink.getNode().asExpr() -select source.getNode(), source, sink, "Blah %s", sink.getNode() + toDeleteConfig.hasFlow(source, deleteCheckpoint) and + fromDeleteConfig.hasFlowPath(deleteCheckpoint2, sink) and + deleteCheckpoint.asExpr() = deleteCheckpoint2.getNode().asExpr() and + isSinkConstrainedByIfCheck(sink.getNode()) +select deleteCheckpoint2, deleteCheckpoint2, sink, "TODO %", sink From 545eb2c4b6de5f5c574c1d08775726c515fe9c8c Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Thu, 15 Oct 2020 16:10:06 -0400 Subject: [PATCH 03/28] Fix inverted predicate logic and add additional test cases --- .../CWE-378/TempDirHijackingVulnerability.ql | 14 +++++----- .../security/CWE-378/semmle/tests/Test.java | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index fe01f387539e..f3a3f06eccdb 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -79,12 +79,12 @@ predicate isSinkConstrainedByIfCheck(DataFlow2::Node sink) { } from - DataFlow::Node source, DataFlow::Node deleteCheckpoint, DataFlow2::PathNode deleteCheckpoint2, - DataFlow2::PathNode sink, TempDirHijackingToDeleteConfig toDeleteConfig, + DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, + DataFlow2::Node sink, TempDirHijackingToDeleteConfig toDeleteConfig, TempDirHijackingFromDeleteConfig fromDeleteConfig where - toDeleteConfig.hasFlow(source, deleteCheckpoint) and - fromDeleteConfig.hasFlowPath(deleteCheckpoint2, sink) and - deleteCheckpoint.asExpr() = deleteCheckpoint2.getNode().asExpr() and - isSinkConstrainedByIfCheck(sink.getNode()) -select deleteCheckpoint2, deleteCheckpoint2, sink, "TODO %", sink + toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and + fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and + deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and + not isSinkConstrainedByIfCheck(sink) +select deleteCheckpoint.getNode(), source, deleteCheckpoint, "Local temporary directory hijacking race condition $@", sink, "here" \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index 581cc3c40077..29f03180760c 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -35,5 +35,33 @@ static File safe2() { throw new RuntimeException("Failed to create directory"); } } + + static File safe3() { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + if (!(temp.mkdirs()))) { + throw new RuntimeException("Failed to create directory"); + } + return temp; + } + + static File safe4() { + boolean success = true; + File temp = File.createTempFile("test", "directory"); + success &= temp.delete(); + success &= f.mkdir(); + if (!success) { + throw new RuntimeException("Failed to create directory"); + } + } + + static File safe5() { + File temp = File.createTempFile("test", "directory"); + if (temp.delete() && temp.mkdir()) { + return temp; + } else { + throw new RuntimeException("Failed to create directory"); + } + } } From 253c96f706e0d2330d4887b97db3a783f97aa11b Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 19 Jan 2022 19:35:24 +0000 Subject: [PATCH 04/28] Improve TempDir hijacking detection with Guard --- .../CWE-378/TempDirHijackingVulnerability.ql | 32 +++++++++---- .../security/CWE-378/semmle/tests/Test.java | 47 +++++++++++++++---- 2 files changed, 62 insertions(+), 17 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index f3a3f06eccdb..1bff84dd07aa 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -8,6 +8,7 @@ */ import java +import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph @@ -70,21 +71,36 @@ private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration } } -predicate isSinkConstrainedByIfCheck(DataFlow2::Node sink) { - exists(MethodAccess ma, IfStmt ifStmt | - sink.asExpr() = ma.getQualifier() and - // Data flow from the return value from `mkdirs` into an 'if' check - DataFlow2::localExprFlow(ma, ifStmt.getCondition()) +predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) { + exists(Guard g, MethodAccess ma | + any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to delete + sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink + g = ma and // The guard is the method access + DataFlow::localExprFlow(sink.asExpr(), unsafeUse) and // There is some flow from the sink to an unsafe use of the File + unsafeUse != sink.asExpr() and // The unsafe use is not the sink itself + not safeUse(unsafeUse) and // The unsafe use is not a safe use + not g.controls(unsafeUse.getBasicBlock(), true) + ) +} + +private predicate safeUse(Expr e) { + exists(AndLogicalExpr andExp | + andExp.getType() instanceof BooleanType and andExp.getAnOperand() = e + ) + or + exists(AssignAndExpr assignAndExp | + assignAndExp.getType() instanceof BooleanType and assignAndExp.getSource() = e ) } from DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, - DataFlow2::Node sink, TempDirHijackingToDeleteConfig toDeleteConfig, + DataFlow2::Node sink, TempDirHijackingToDeleteConfig toDeleteConfig, Expr unsafeUse, TempDirHijackingFromDeleteConfig fromDeleteConfig where toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and - not isSinkConstrainedByIfCheck(sink) -select deleteCheckpoint.getNode(), source, deleteCheckpoint, "Local temporary directory hijacking race condition $@", sink, "here" \ No newline at end of file + isUnsafeUseUnconstrainedByIfCheck(sink, unsafeUse) +select deleteCheckpoint.getNode(), source, deleteCheckpoint, + "Local temporary directory hijacking race condition $@", sink, "here" diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index 29f03180760c..bc2782142595 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -1,21 +1,37 @@ import java.io.File; +import java.io.IOException; public class Test { - static File vulnerable() { + private static final File vulnerableAssigned; + + static { + File temp = null; + try { + temp = File.createTempFile("test", "directory"); + temp.delete(); + temp.mkdir(); + } catch(IOException e) { + System.out.println("Could not create temporary directory"); + System.exit(-1); + } + vulnerableAssigned = temp; + } + + static File vulnerable() throws IOException { File temp = File.createTempFile("test", "directory"); temp.delete(); temp.mkdir(); return temp; } - static File notVulnerableToHijacking() { + static File notVulnerableToHijacking() throws IOException { File temp = File.createTempFile("test", "directory"); temp.mkdir(); return temp; } - static File safe() { + static File safe() throws IOException { File temp = File.createTempFile("test", "directory"); temp.delete(); if (temp.mkdir()) { @@ -25,7 +41,7 @@ static File safe() { } } - static File safe2() { + static File safe2() throws IOException { File temp = File.createTempFile("test", "directory"); temp.delete(); boolean didMkdirs = temp.mkdirs(); @@ -36,26 +52,39 @@ static File safe2() { } } - static File safe3() { + static File safe3() throws IOException { File temp = File.createTempFile("test", "directory"); temp.delete(); - if (!(temp.mkdirs()))) { + if (!(temp.mkdirs())) { throw new RuntimeException("Failed to create directory"); } return temp; } - static File safe4() { + static File safe4() throws IOException { boolean success = true; File temp = File.createTempFile("test", "directory"); success &= temp.delete(); - success &= f.mkdir(); + success &= temp.mkdir(); if (!success) { throw new RuntimeException("Failed to create directory"); } + return temp; + } + + static File safe5() throws IOException { + boolean success = true; + File temp = File.createTempFile("test", "directory"); + success &= temp.delete(); + success &= temp.mkdir(); + if (success) { + return temp; + } else { + throw new RuntimeException("Failed to create directory"); + } } - static File safe5() { + static File safe6() throws IOException { File temp = File.createTempFile("test", "directory"); if (temp.delete() && temp.mkdir()) { return temp; From d9d5b67949592fd032aa78610e67a0d63c4144c4 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 19 Jan 2022 21:23:27 +0000 Subject: [PATCH 05/28] Improve warning in TempDirHijackingVulnerability --- .../CWE-378/TempDirHijackingVulnerability.ql | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 1bff84dd07aa..a456298d1a31 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -31,6 +31,13 @@ private class MethodFileMkdir extends Method { } } +/** + * An expression that will create a directory without throwing an exception if a file/directory already exists. + */ +private predicate isNonThrowingDirectoryCreationExpression(Expr expr, MethodAccess creationCall) { + creationCall.getMethod() instanceof MethodFileMkdir and creationCall.getQualifier() = expr +} + private class MethodFileDelete extends Method { MethodFileDelete() { getDeclaringType() instanceof TypeFile and @@ -64,10 +71,7 @@ private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration override predicate isSource(DataFlow::Node source) { isDeleteFileExpr(source.asExpr()) } override predicate isSink(DataFlow::Node sink) { - exists(MethodAccess ma | - ma.getMethod() instanceof MethodFileMkdir and - ma.getQualifier() = sink.asExpr() - ) + isNonThrowingDirectoryCreationExpression(sink.asExpr(), _) } } @@ -95,12 +99,13 @@ private predicate safeUse(Expr e) { from DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, - DataFlow2::Node sink, TempDirHijackingToDeleteConfig toDeleteConfig, Expr unsafeUse, + DataFlow2::Node sink, MethodAccess creationCall, TempDirHijackingToDeleteConfig toDeleteConfig, TempDirHijackingFromDeleteConfig fromDeleteConfig where toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and - isUnsafeUseUnconstrainedByIfCheck(sink, unsafeUse) + isUnsafeUseUnconstrainedByIfCheck(sink, _) and + isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall) select deleteCheckpoint.getNode(), source, deleteCheckpoint, - "Local temporary directory hijacking race condition $@", sink, "here" + "Local temporary directory hijacking race condition $@", creationCall, "here" From 1fc762932145dceb5d4e61b1dfc1bb6605303b72 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Thu, 20 Jan 2022 00:08:41 +0000 Subject: [PATCH 06/28] Add documentation and additional test cases --- .../CWE/CWE-378/TempDirCreationSafe.java | 13 ++++++ .../CWE-378/TempDirCreationVulnerable.java | 14 ++++++ .../TempDirHijackingVulnerability.qhelp | 45 +++++++++++++++++++ .../CWE-378/TempDirHijackingVulnerability.ql | 21 +++++++-- .../security/CWE-378/semmle/tests/Test.java | 32 +++++++++++++ 5 files changed, 122 insertions(+), 3 deletions(-) create mode 100644 java/ql/src/Security/CWE/CWE-378/TempDirCreationSafe.java create mode 100644 java/ql/src/Security/CWE/CWE-378/TempDirCreationVulnerable.java create mode 100644 java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirCreationSafe.java b/java/ql/src/Security/CWE/CWE-378/TempDirCreationSafe.java new file mode 100644 index 000000000000..664a31bc0186 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-378/TempDirCreationSafe.java @@ -0,0 +1,13 @@ +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + +public class TempDirCreationSafe { + File exampleSafeTemporaryDirectoryCreation() { + // Best option! + Path tempDir = + Files.createTempDirectory("temporary-stuff"); // GOOD! - Files.createTempDirectory creates a temporary directory with safe permissions. + return tempDir.toFile(); + } +} diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirCreationVulnerable.java b/java/ql/src/Security/CWE/CWE-378/TempDirCreationVulnerable.java new file mode 100644 index 000000000000..971ec2676045 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-378/TempDirCreationVulnerable.java @@ -0,0 +1,14 @@ +import java.io.File; +import java.io.IOException; + +public class TempDirCreationVulnerable { + File exampleVulnerableTemporaryDirectoryCreation() throws IOException { + File temp = File.createTempFile("temporary", "stuff"); // Attacker knows the full path of the directory that will be generated. + // delete the file that was created + temp.delete(); // Attacker sees file is deleted and begins a race to create their own directory with wide file permissions. + // and make a directory of the same name. + // SECURITY VULNERABILITY: Race Condition! - If the attacker beats your application to directory creation they will own this directory. + temp.mkdir(); // BAD! Race condition + return temp; + } +} diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp new file mode 100644 index 000000000000..ab465a398f7d --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp @@ -0,0 +1,45 @@ + + + +

Local temporary directory hijacking can occur when files/directories are created within directories that are shared between all users on the system.

+ +

On most unix-like systems, +the system temporary directory is shared between local users. +If directories are created within the system temporary directory without using +APIs that explicitly set the correct file permissions, vulnerabilties ranging from +local information disclosure, to directory hijacking, to local privilege escalation, can occur.

+ +

In the worst case, where local privilege escalation occurs, this vulnerability can have a +CVSS v3.1 base score of 7.8.

+ + + +

Use JDK methods that specifically protect against this vulnerability:

+ +

Otherwise, create the file/directory by manually specifying the expected posix file permissions. +For example: PosixFilePermissions.asFileAttribute(EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE))

+ + + + +

In the following example, the directory that is created can be hijacked during the creation process due to a race condition.

+ + +

In the following example, the Files.createTempDirectory API is used which creates a temporary directory that is guarnteed to be safe.

+ + + + +
  • OSWAP: Insecure Temporary File.
    • +
  • CERT: FIO00-J. Do not operate on files in shared directories
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index a456298d1a31..632d43bb3fdf 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -95,17 +95,32 @@ private predicate safeUse(Expr e) { exists(AssignAndExpr assignAndExp | assignAndExp.getType() instanceof BooleanType and assignAndExp.getSource() = e ) + or + // File is being concatenated to a string, probably for a log or exception message + exists(AddExpr addExp | + addExp.getType() instanceof TypeString and + ( + addExp.getRightOperand() = e + or + // A method call, like `File.getAbsolutePath()` is being called and concatenated into a string + exists(MethodAccess fileMethodAccess | + fileMethodAccess.getQualifier() = e and + addExp.getRightOperand() = fileMethodAccess and + fileMethodAccess.getMethod().getReturnType() instanceof TypeString + ) + ) + ) } from DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, - DataFlow2::Node sink, MethodAccess creationCall, TempDirHijackingToDeleteConfig toDeleteConfig, + DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe, TempDirHijackingToDeleteConfig toDeleteConfig, TempDirHijackingFromDeleteConfig fromDeleteConfig where toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and - isUnsafeUseUnconstrainedByIfCheck(sink, _) and + isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall) select deleteCheckpoint.getNode(), source, deleteCheckpoint, - "Local temporary directory hijacking race condition $@", creationCall, "here" + "Local temporary directory hijacking race condition $@ file $@ may have been hijacked", creationCall, "here", unsafe, "here" diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index bc2782142595..d8c1789ab7e1 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -92,5 +92,37 @@ static File safe6() throws IOException { throw new RuntimeException("Failed to create directory"); } } + + static File safe7() throws IOException { + File temp = File.createTempFile("test", "directory"); + if (!temp.delete() || !temp.mkdir()) { + throw new IOException("Can not convert temporary file " + temp + "to directory"); + } else { + return temp; + } + } + + /** + * When `isDirectory` is true, create a temporary directory, else create a temporary file. + */ + static File safe8(boolean isDirectory) throws IOException { + File temp = File.createTempFile("test", "directory"); + if (isDirectory && (!temp.delete() || !temp.mkdir())) { + throw new IOException("Can not convert temporary file " + temp + "to directory"); + } else { + return temp; + } + } + + static File safe9() throws IOException { + File temp = File.createTempFile("test", "directory"); + if (!temp.delete()) { + throw new IOException("Could not delete temp file: " + temp.getAbsolutePath()); + } + if (!temp.mkdir()) { + throw new IOException("Could not create temp directory: " + temp.getAbsolutePath()); + } + return temp; + } } From b42ff137315cb24fbab6be07b32891035b8d1e4f Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Thu, 20 Jan 2022 15:27:57 +0000 Subject: [PATCH 07/28] Improve TempDirHijackingVulnerability message --- .../src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 632d43bb3fdf..ff27b63b9e71 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -123,4 +123,4 @@ where isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall) select deleteCheckpoint.getNode(), source, deleteCheckpoint, - "Local temporary directory hijacking race condition $@ file $@ may have been hijacked", creationCall, "here", unsafe, "here" + "Local temporary directory hijacking race condition $@, file $@ may have been hijacked", creationCall, "here", unsafe, "here" From 20bd05b530e6a95f648011610e8fe3cd55f2c65d Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 9 Mar 2022 15:46:46 -0500 Subject: [PATCH 08/28] Add predicate handling `isDirectory` case --- .../CWE-378/TempDirHijackingVulnerability.ql | 29 +++++++++++--- .../TempDirHijackingVulnerability.expected | 40 +++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) create mode 100644 java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index ff27b63b9e71..737c05c3b805 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -75,15 +75,33 @@ private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration } } +/** + * Holds when there is an epression `unsafeUse` which is an unsafe use of the file that + * is not guarded by a check of the return value of `File::mkdir(s)`. + */ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) { exists(Guard g, MethodAccess ma | - any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to delete + any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to `mkdir` or `mkdirs` sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink g = ma and // The guard is the method access DataFlow::localExprFlow(sink.asExpr(), unsafeUse) and // There is some flow from the sink to an unsafe use of the File unsafeUse != sink.asExpr() and // The unsafe use is not the sink itself not safeUse(unsafeUse) and // The unsafe use is not a safe use - not g.controls(unsafeUse.getBasicBlock(), true) + not g.controls(unsafeUse.getBasicBlock(), true) and + not booleanVarAccessGuardsGuard(g) // The guard is not guarded by a boolean variable access guard + ) +} + +/** + * Holds for any guard `g` that that is itself guarded by a a boolean variable access guard. + * + * For example, the following code: `if (isDirectory && !file.mkdir()) { ... }`. + */ +private predicate booleanVarAccessGuardsGuard(Guard g) { + exists(Guard g2 | + g2 = any(VarAccess va | va.getType() instanceof BooleanType) + | + g2.controls(g.getBasicBlock(), true) ) } @@ -114,8 +132,8 @@ private predicate safeUse(Expr e) { from DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, - DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe, TempDirHijackingToDeleteConfig toDeleteConfig, - TempDirHijackingFromDeleteConfig fromDeleteConfig + DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe, + TempDirHijackingToDeleteConfig toDeleteConfig, TempDirHijackingFromDeleteConfig fromDeleteConfig where toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and @@ -123,4 +141,5 @@ where isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall) select deleteCheckpoint.getNode(), source, deleteCheckpoint, - "Local temporary directory hijacking race condition $@, file $@ may have been hijacked", creationCall, "here", unsafe, "here" + "Local temporary directory hijacking race condition $@, file $@ may have been hijacked", + creationCall, "here", unsafe, "here" diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected new file mode 100644 index 000000000000..932d77395473 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -0,0 +1,40 @@ +edges +| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | +| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | +| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:36:9:36:12 | temp | +| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:46:9:46:12 | temp | +| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:57:9:57:12 | temp | +| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:67:20:67:23 | temp | +| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:78:20:78:23 | temp | +| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:13:89:16 | temp | +| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:14:98:17 | temp | +| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp | +| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp | +nodes +| Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:12:13:12:16 | temp | semmle.label | temp | +| Test.java:22:21:22:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:23:9:23:12 | temp | semmle.label | temp | +| Test.java:35:21:35:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:36:9:36:12 | temp | semmle.label | temp | +| Test.java:45:21:45:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:46:9:46:12 | temp | semmle.label | temp | +| Test.java:56:21:56:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:57:9:57:12 | temp | semmle.label | temp | +| Test.java:66:21:66:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:67:20:67:23 | temp | semmle.label | temp | +| Test.java:77:21:77:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:78:20:78:23 | temp | semmle.label | temp | +| Test.java:88:21:88:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:89:13:89:16 | temp | semmle.label | temp | +| Test.java:97:21:97:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:98:14:98:17 | temp | semmle.label | temp | +| Test.java:109:21:109:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:110:30:110:33 | temp | semmle.label | temp | +| Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:119:14:119:17 | temp | semmle.label | temp | +subpaths +#select +| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:9:18:33 | ...=... | here | +| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:30:18:33 | temp | here | +| Test.java:23:9:23:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:24:9:24:20 | mkdir(...) | here | Test.java:25:16:25:19 | temp | here | From 442ef834c854ef1702745241fab88e5dca048e0f Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Thu, 10 Mar 2022 11:50:46 -0500 Subject: [PATCH 09/28] Add deleteOnExit as safe usage --- .../CWE/CWE-378/TempDirHijackingVulnerability.ql | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 737c05c3b805..2bd38eb52761 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -94,13 +94,11 @@ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) /** * Holds for any guard `g` that that is itself guarded by a a boolean variable access guard. - * + * * For example, the following code: `if (isDirectory && !file.mkdir()) { ... }`. */ private predicate booleanVarAccessGuardsGuard(Guard g) { - exists(Guard g2 | - g2 = any(VarAccess va | va.getType() instanceof BooleanType) - | + exists(Guard g2 | g2 = any(VarAccess va | va.getType() instanceof BooleanType) | g2.controls(g.getBasicBlock(), true) ) } @@ -128,6 +126,13 @@ private predicate safeUse(Expr e) { ) ) ) + or + // A call to `File::deleteOnExit` + exists(MethodAccess ma | + ma.getMethod().hasName("deleteOnExit") and + ma.getMethod().getDeclaringType() instanceof TypeFile and + ma.getQualifier() = e + ) } from From fbecfdd8184f0f7892211f739f3f8648cdd8dc37 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Tue, 15 Mar 2022 13:50:53 -0400 Subject: [PATCH 10/28] Start taint hijacking tracking with `java.io.tmpdir` --- .../src/Security/CWE/CWE-200/TempDirUtils.qll | 2 +- .../CWE-378/TempDirHijackingVulnerability.ql | 31 +++++++++++-------- .../security/CWE-378/semmle/tests/Test.java | 14 +++++++++ 3 files changed, 33 insertions(+), 14 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll index cb2d8dd7875e..7a7f2a5ac016 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll +++ b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll @@ -64,7 +64,7 @@ private class FileSetRedableMethodAccess extends MethodAccess { /** * Hold's if temporary directory's use is protected if there is an explicit call to - * `setReadable(false, false)`, then `setRedabale(true, true)`. + * `setReadable(false, false)`, then `setReadable(true, true)`. */ predicate isPermissionsProtectedTempDirUse(DataFlow::Node sink) { exists(FileSetRedableMethodAccess setReadable1, FileSetRedableMethodAccess setReadable2 | diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 2bd38eb52761..b5991e73cb35 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -46,20 +46,26 @@ private class MethodFileDelete extends Method { } predicate isDeleteFileExpr(Expr expr) { - exists(MethodAccess ma | - expr = ma.getQualifier() and - ma.getMethod() instanceof MethodFileDelete - ) + expr = any(MethodFileDelete m).getAReference().getQualifier() } -private class TempDirHijackingToDeleteConfig extends DataFlow::Configuration { +private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuration { TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" } override predicate isSource(DataFlow::Node source) { - exists(MethodAccess ma | - ma.getMethod() instanceof MethodFileCreateTempFile and - source.asExpr() = ma - ) + source.asExpr() = + any(MethodAccess ma | + ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2 + ) or + // TODO: Replace with getSystemProperty("java.io.tmpdir") + source.asExpr() = any(MethodAccessSystemGetProperty maSgp | maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir")) + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + node2.asExpr() = + any(MethodAccess ma | + ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() + ) } override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) } @@ -137,11 +143,10 @@ private predicate safeUse(Expr e) { from DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, - DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe, - TempDirHijackingToDeleteConfig toDeleteConfig, TempDirHijackingFromDeleteConfig fromDeleteConfig + DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe where - toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and - fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and + any(TempDirHijackingToDeleteConfig c).hasFlowPath(source, deleteCheckpoint) and + any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint2, sink) and deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall) diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index d8c1789ab7e1..6f381a984260 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -124,5 +124,19 @@ static File safe9() throws IOException { } return temp; } + + static File vulnerable2() throws IOException { + File temp = File.createTempFile("test", "directory", new File(System.getProperty("java.io.tmpdir"))); + temp.delete(); + temp.mkdir(); + return temp; + } + + static File safe10() throws IOException { + File temp = File.createTempFile("test", "directory", new File(System.getProperty("user.dir"))); + temp.delete(); + temp.mkdir(); + return temp; + } } From 8a7d64dba8df555201a2895b57b4ae366cf37c5e Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Tue, 15 Mar 2022 17:03:36 -0400 Subject: [PATCH 11/28] Refactor common logic into TempFileLib --- .../semmle/code/java/security/TempFileLib.qll | 23 +++++++++++++++++++ .../src/Security/CWE/CWE-200/TempDirUtils.qll | 11 +-------- .../CWE-378/TempDirHijackingVulnerability.ql | 11 +-------- .../TempDirHijackingVulnerability.expected | 14 +++++++++++ .../security/CWE-378/semmle/tests/Test.java | 11 +++++++++ 5 files changed, 50 insertions(+), 20 deletions(-) create mode 100644 java/ql/lib/semmle/code/java/security/TempFileLib.qll diff --git a/java/ql/lib/semmle/code/java/security/TempFileLib.qll b/java/ql/lib/semmle/code/java/security/TempFileLib.qll new file mode 100644 index 000000000000..ff2e2cc78eab --- /dev/null +++ b/java/ql/lib/semmle/code/java/security/TempFileLib.qll @@ -0,0 +1,23 @@ +/** Provides classes to reason about temporary directory vulnerabilities. */ + +import java +import semmle.code.java.dataflow.ExternalFlow + +/** + * All `java.io.File::createTempFile` methods. + */ +class MethodFileCreateTempFile extends Method { + MethodFileCreateTempFile() { + this.getDeclaringType() instanceof TypeFile and + this.hasName("createTempFile") + } +} + +private class TemporaryFileFlow extends SummaryModelCsv { + override predicate row(string row) { + // qualifier to return + row = + "java.io;File;true;" + ["getAbsoluteFile", "getCanonicalFile"] + + ";;;Argument[-1];ReturnValue;taint" + } +} diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll index 7a7f2a5ac016..238f3c862aab 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll +++ b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll @@ -4,6 +4,7 @@ import java private import semmle.code.java.environment.SystemProperty +import semmle.code.java.security.TempFileLib import semmle.code.java.dataflow.FlowSources /** @@ -13,16 +14,6 @@ class ExprSystemGetPropertyTempDirTainted extends Expr { ExprSystemGetPropertyTempDirTainted() { this = getSystemProperty("java.io.tmpdir") } } -/** - * A `java.io.File::createTempFile` method. - */ -class MethodFileCreateTempFile extends Method { - MethodFileCreateTempFile() { - this.getDeclaringType() instanceof TypeFile and - this.hasName("createTempFile") - } -} - /** * Holds if `expDest` is some constructor call `new java.io.File(expSource)`, where the specific `File` constructor being used has `paramCount` parameters. */ diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index b5991e73cb35..fbb0e4e68cbe 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -10,18 +10,9 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources +import semmle.code.java.security.TempFileLib import DataFlow::PathGraph -/** - * All `java.io.File::createTempFile` methods. - */ -private class MethodFileCreateTempFile extends Method { - MethodFileCreateTempFile() { - this.getDeclaringType() instanceof TypeFile and - this.hasName("createTempFile") - } -} - private class MethodFileMkdir extends Method { MethodFileMkdir() { getDeclaringType() instanceof TypeFile and diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index 932d77395473..adf2cbcc3ef2 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -10,6 +10,11 @@ edges | Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:14:98:17 | temp | | Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp | | Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp | +| Test.java:129:62:129:107 | new File(...) : File | Test.java:130:9:130:12 | temp | +| Test.java:129:71:129:106 | getProperty(...) : String | Test.java:129:62:129:107 | new File(...) : File | +| Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | +| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:148:9:148:12 | temp | +| Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:12:13:12:16 | temp | semmle.label | temp | @@ -33,8 +38,17 @@ nodes | Test.java:110:30:110:33 | temp | semmle.label | temp | | Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:119:14:119:17 | temp | semmle.label | temp | +| Test.java:129:62:129:107 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:129:71:129:106 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:130:9:130:12 | temp | semmle.label | temp | +| Test.java:146:13:146:58 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File | +| Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:148:9:148:12 | temp | semmle.label | temp | subpaths #select | Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:9:18:33 | ...=... | here | | Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:30:18:33 | temp | here | | Test.java:23:9:23:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:24:9:24:20 | mkdir(...) | here | Test.java:25:16:25:19 | temp | here | +| Test.java:130:9:130:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:130:9:130:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:131:9:131:20 | mkdir(...) | here | Test.java:132:16:132:19 | temp | here | +| Test.java:148:9:148:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:148:9:148:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:149:9:149:20 | mkdir(...) | here | Test.java:150:16:150:19 | temp | here | diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index 6f381a984260..aabf46e8983e 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -138,5 +138,16 @@ static File safe10() throws IOException { temp.mkdir(); return temp; } + + static File vulnerable3() throws IOException { + File temp = File.createTempFile( + "test", + "directory", + new File(System.getProperty("java.io.tmpdir")).getAbsoluteFile() + ); + temp.delete(); + temp.mkdir(); + return temp; + } } From 884db9ee334a92531df2aff20bb29c0b258f1b6f Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Tue, 15 Mar 2022 18:18:06 -0400 Subject: [PATCH 12/28] Refactor more logic to TempFileLib --- .../semmle/code/java/security/TempFileLib.qll | 10 +++++++++ .../TempDirLocalInformationDisclosure.ql | 3 +-- .../CWE-378/TempDirHijackingVulnerability.ql | 21 +++++++------------ 3 files changed, 19 insertions(+), 15 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/TempFileLib.qll b/java/ql/lib/semmle/code/java/security/TempFileLib.qll index ff2e2cc78eab..229ec1846801 100644 --- a/java/ql/lib/semmle/code/java/security/TempFileLib.qll +++ b/java/ql/lib/semmle/code/java/security/TempFileLib.qll @@ -13,6 +13,16 @@ class MethodFileCreateTempFile extends Method { } } +/** + * All methods on the class `java.io.File` that create directories. + */ +class MethodFileCreatesDirs extends Method { + MethodFileCreatesDirs() { + getDeclaringType() instanceof TypeFile and + hasName(["mkdir", "mkdirs"]) + } +} + private class TemporaryFileFlow extends SummaryModelCsv { override predicate row(string row) { // qualifier to return diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index 16e8bb72c930..5fb0add69bbc 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -21,8 +21,7 @@ abstract private class MethodFileSystemFileCreation extends Method { MethodFileSystemFileCreation() { this.getDeclaringType() instanceof TypeFile } } -private class MethodFileDirectoryCreation extends MethodFileSystemFileCreation { - MethodFileDirectoryCreation() { this.hasName(["mkdir", "mkdirs"]) } +private class MethodFileDirectoryCreation extends MethodFileSystemFileCreation instanceof MethodFileCreatesDirs { } private class MethodFileFileCreation extends MethodFileSystemFileCreation { diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index fbb0e4e68cbe..47b539f57d71 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -13,20 +13,11 @@ import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.TempFileLib import DataFlow::PathGraph -private class MethodFileMkdir extends Method { - MethodFileMkdir() { - getDeclaringType() instanceof TypeFile and - hasName("mkdir") - or - hasName("mkdirs") - } -} - /** * An expression that will create a directory without throwing an exception if a file/directory already exists. */ private predicate isNonThrowingDirectoryCreationExpression(Expr expr, MethodAccess creationCall) { - creationCall.getMethod() instanceof MethodFileMkdir and creationCall.getQualifier() = expr + creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = expr } private class MethodFileDelete extends Method { @@ -47,9 +38,13 @@ private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuratio source.asExpr() = any(MethodAccess ma | ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2 - ) or - // TODO: Replace with getSystemProperty("java.io.tmpdir") - source.asExpr() = any(MethodAccessSystemGetProperty maSgp | maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir")) + ) + or + // TODO: Replace with getSystemProperty("java.io.tmpdir") + source.asExpr() = + any(MethodAccessSystemGetProperty maSgp | + maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir") + ) } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { From 6f4ed4bd3b3e62a0ffbd17bd3c6453cc6220fc11 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 16 Mar 2022 10:44:44 -0400 Subject: [PATCH 13/28] Apply suggestions from code review Co-authored-by: Marcono1234 --- .../CWE-378/TempDirHijackingVulnerability.qhelp | 14 +++++++------- .../CWE/CWE-378/TempDirHijackingVulnerability.ql | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp index ab465a398f7d..ae981d93614f 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp @@ -18,15 +18,15 @@ local information disclosure, to directory hijacking, to local privilege escalat

    Use JDK methods that specifically protect against this vulnerability:

    Otherwise, create the file/directory by manually specifying the expected posix file permissions. For example: PosixFilePermissions.asFileAttribute(EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE))

     @@ -34,12 +34,12 @@ For example: PosixFilePermissions.asFileAttribute(EnumSet.of(PosixFilePerm

    In the following example, the directory that is created can be hijacked during the creation process due to a race condition.

    -

    In the following example, the Files.createTempDirectory API is used which creates a temporary directory that is guarnteed to be safe.

    +

    In the following example, the Files.createTempDirectory API is used which creates a temporary directory that is guaranteed to be safe.

  • OSWAP: Insecure Temporary File.
    • -
  • CERT: FIO00-J. Do not operate on files in shared directories
    • +
  • CERT: FIO00-J. Do not operate on files in shared directories.
    •  diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 47b539f57d71..b0d2c05a4e6f 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -68,7 +68,7 @@ private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration } /** - * Holds when there is an epression `unsafeUse` which is an unsafe use of the file that + * Holds when there is an expression `unsafeUse` which is an unsafe use of the file that * is not guarded by a check of the return value of `File::mkdir(s)`. */ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) { @@ -85,7 +85,7 @@ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) } /** - * Holds for any guard `g` that that is itself guarded by a a boolean variable access guard. + * Holds for any guard `g` that is itself guarded by a boolean variable access guard. * * For example, the following code: `if (isDirectory && !file.mkdir()) { ... }`. */ From 03983f1f100a12260e38827f7f64f189b811582f Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 16 Mar 2022 13:43:38 -0400 Subject: [PATCH 14/28] Refactor TempDirHijacking to show complete path --- .../CWE-378/TempDirHijackingVulnerability.ql | 69 +++++++++++++------ .../TempDirHijackingVulnerability.expected | 65 ++++++++--------- 2 files changed, 82 insertions(+), 52 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index b0d2c05a4e6f..547f428ef88b 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -11,6 +11,9 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources import semmle.code.java.security.TempFileLib +import semmle.code.java.dataflow.DataFlow3 +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.TaintTracking2 import DataFlow::PathGraph /** @@ -31,33 +34,41 @@ predicate isDeleteFileExpr(Expr expr) { expr = any(MethodFileDelete m).getAReference().getQualifier() } -private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuration { - TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" } - - override predicate isSource(DataFlow::Node source) { - source.asExpr() = +private class DirHijackingTaintSource extends DataFlow::Node { + DirHijackingTaintSource() { + this.asExpr() = any(MethodAccess ma | ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2 ) or // TODO: Replace with getSystemProperty("java.io.tmpdir") - source.asExpr() = + this.asExpr() = any(MethodAccessSystemGetProperty maSgp | maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir") ) } +} + +private predicate isAdditionalTaintStepCommon(DataFlow::Node node1, DataFlow::Node node2) { + node2.asExpr() = + any(MethodAccess ma | + ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() + ) +} + +private class TempDirHijackingToDeleteConfig extends TaintTracking2::Configuration { + TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" } + + override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - node2.asExpr() = - any(MethodAccess ma | - ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() - ) + isAdditionalTaintStepCommon(node1, node2) } override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) } } -private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration { +private class TempDirHijackingFromDeleteConfig extends DataFlow3::Configuration { TempDirHijackingFromDeleteConfig() { this = "TempDirHijackingFromDeleteConfig" } override predicate isSource(DataFlow::Node source) { isDeleteFileExpr(source.asExpr()) } @@ -127,15 +138,31 @@ private predicate safeUse(Expr e) { ) } +private class TempDirHijackingFullPath extends TaintTracking::Configuration { + TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" } + + override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource } + + override predicate isSink(DataFlow::Node sink) { + isNonThrowingDirectoryCreationExpression(sink.asExpr(), _) + } + + override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + isAdditionalTaintStepCommon(node1, node2) + or + node1.asExpr() = node2.asExpr() and isDeleteFileExpr(node1.asExpr()) + } +} + from - DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2, - DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe + DataFlow::PathNode pathSource, DataFlow::PathNode pathSink, DataFlow::Node deleteCheckpoint, + MethodAccess creationCall, Expr unsafe where - any(TempDirHijackingToDeleteConfig c).hasFlowPath(source, deleteCheckpoint) and - any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint2, sink) and - deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and - isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and - isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall) -select deleteCheckpoint.getNode(), source, deleteCheckpoint, - "Local temporary directory hijacking race condition $@, file $@ may have been hijacked", - creationCall, "here", unsafe, "here" + any(TempDirHijackingFullPath c).hasFlowPath(pathSource, pathSink) and + any(TempDirHijackingToDeleteConfig c).hasFlow(pathSource.getNode(), deleteCheckpoint) and + any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and + isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and + isNonThrowingDirectoryCreationExpression(pathSink.getNode().asExpr(), creationCall) +select pathSink, pathSource, pathSink, + "Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user.", + deleteCheckpoint.asExpr(), "delete here", unsafe, "here" diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index adf2cbcc3ef2..4e377052e4ba 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -1,54 +1,57 @@ edges -| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | -| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | -| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:36:9:36:12 | temp | -| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:46:9:46:12 | temp | -| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:57:9:57:12 | temp | -| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:67:20:67:23 | temp | -| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:78:20:78:23 | temp | -| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:13:89:16 | temp | -| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:14:98:17 | temp | -| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp | -| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp | -| Test.java:129:62:129:107 | new File(...) : File | Test.java:130:9:130:12 | temp | +| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | +| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | +| Test.java:29:21:29:60 | createTempFile(...) : File | Test.java:30:9:30:12 | temp | +| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:37:13:37:16 | temp | +| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:47:29:47:32 | temp | +| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:58:15:58:18 | temp | +| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:68:20:68:23 | temp | +| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:79:20:79:23 | temp | +| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:30:89:33 | temp | +| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:32:98:35 | temp | +| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:48:110:51 | temp | +| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:122:14:122:17 | temp | +| Test.java:129:62:129:107 | new File(...) : File | Test.java:131:9:131:12 | temp | | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:129:62:129:107 | new File(...) : File | | Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | -| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:148:9:148:12 | temp | +| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:149:9:149:12 | temp | | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:12:13:12:16 | temp | semmle.label | temp | +| Test.java:13:13:13:16 | temp | semmle.label | temp | | Test.java:22:21:22:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:23:9:23:12 | temp | semmle.label | temp | +| Test.java:24:9:24:12 | temp | semmle.label | temp | +| Test.java:29:21:29:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:30:9:30:12 | temp | semmle.label | temp | | Test.java:35:21:35:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:36:9:36:12 | temp | semmle.label | temp | +| Test.java:37:13:37:16 | temp | semmle.label | temp | | Test.java:45:21:45:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:46:9:46:12 | temp | semmle.label | temp | +| Test.java:47:29:47:32 | temp | semmle.label | temp | | Test.java:56:21:56:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:57:9:57:12 | temp | semmle.label | temp | +| Test.java:58:15:58:18 | temp | semmle.label | temp | | Test.java:66:21:66:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:67:20:67:23 | temp | semmle.label | temp | +| Test.java:68:20:68:23 | temp | semmle.label | temp | | Test.java:77:21:77:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:78:20:78:23 | temp | semmle.label | temp | +| Test.java:79:20:79:23 | temp | semmle.label | temp | | Test.java:88:21:88:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:89:13:89:16 | temp | semmle.label | temp | +| Test.java:89:30:89:33 | temp | semmle.label | temp | | Test.java:97:21:97:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:98:14:98:17 | temp | semmle.label | temp | +| Test.java:98:32:98:35 | temp | semmle.label | temp | | Test.java:109:21:109:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:110:30:110:33 | temp | semmle.label | temp | +| Test.java:110:48:110:51 | temp | semmle.label | temp | | Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:119:14:119:17 | temp | semmle.label | temp | +| Test.java:122:14:122:17 | temp | semmle.label | temp | | Test.java:129:62:129:107 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:129:71:129:106 | getProperty(...) : String | semmle.label | getProperty(...) : String | -| Test.java:130:9:130:12 | temp | semmle.label | temp | +| Test.java:131:9:131:12 | temp | semmle.label | temp | | Test.java:146:13:146:58 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File | | Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String | -| Test.java:148:9:148:12 | temp | semmle.label | temp | +| Test.java:149:9:149:12 | temp | semmle.label | temp | subpaths #select -| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:9:18:33 | ...=... | here | -| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:30:18:33 | temp | here | -| Test.java:23:9:23:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:24:9:24:20 | mkdir(...) | here | Test.java:25:16:25:19 | temp | here | -| Test.java:130:9:130:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:130:9:130:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:131:9:131:20 | mkdir(...) | here | Test.java:132:16:132:19 | temp | here | -| Test.java:148:9:148:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:148:9:148:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:149:9:149:20 | mkdir(...) | here | Test.java:150:16:150:19 | temp | here | +| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here | +| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:30:18:33 | temp | here | +| Test.java:24:9:24:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:23:9:23:12 | temp | delete here | Test.java:25:16:25:19 | temp | here | +| Test.java:131:9:131:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:131:9:131:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:130:9:130:12 | temp | delete here | Test.java:132:16:132:19 | temp | here | +| Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here | From 37b1e1d30d4aebfe04a077a771ab53175947fd72 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 18 Mar 2022 11:12:31 -0400 Subject: [PATCH 15/28] Update to use new getSystemProperty predicate --- .../CWE/CWE-378/TempDirHijackingVulnerability.ql | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 547f428ef88b..8c32f12ff828 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -10,10 +10,11 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources -import semmle.code.java.security.TempFileLib import semmle.code.java.dataflow.DataFlow3 import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.TaintTracking2 +import semmle.code.java.environment.SystemProperty +import semmle.code.java.security.TempFileLib import DataFlow::PathGraph /** @@ -41,11 +42,7 @@ private class DirHijackingTaintSource extends DataFlow::Node { ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2 ) or - // TODO: Replace with getSystemProperty("java.io.tmpdir") - this.asExpr() = - any(MethodAccessSystemGetProperty maSgp | - maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir") - ) + this.asExpr() = getSystemProperty("java.io.tmpdir") } } @@ -149,8 +146,6 @@ private class TempDirHijackingFullPath extends TaintTracking::Configuration { override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { isAdditionalTaintStepCommon(node1, node2) - or - node1.asExpr() = node2.asExpr() and isDeleteFileExpr(node1.asExpr()) } } From ac8e1cc9bce3f6a92f31f9ff1e6277f18e3a0898 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 18 Mar 2022 15:06:52 -0400 Subject: [PATCH 16/28] Add additional test cases --- .../CWE-378/TempDirHijackingVulnerability.ql | 46 +++++++++++++-- .../security/CWE-378/semmle/tests/Test.java | 57 +++++++++++++++++++ 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 8c32f12ff828..f252c1bcee03 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -11,6 +11,7 @@ import java import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources import semmle.code.java.dataflow.DataFlow3 +import semmle.code.java.dataflow.DataFlow4 import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.TaintTracking2 import semmle.code.java.environment.SystemProperty @@ -103,6 +104,28 @@ private predicate booleanVarAccessGuardsGuard(Guard g) { ) } +/** + * Gets any `MethodAccess` that access string returning methods on the expression `e` of type `File`. + */ +private MethodAccess getStringAccessOnFile(Expr e) { + result = + any(MethodAccess fileMethodAccess | + fileMethodAccess.getMethod().getDeclaringType() instanceof TypeFile and + fileMethodAccess.getQualifier() = e and + fileMethodAccess.getMethod().getReturnType() instanceof TypeString + ) +} + +private Argument getThrowableConstructorParam() { + result = + any(Argument a | + exists(ConstructorCall c | + c.getConstructor().getDeclaringType().getAnAncestor() instanceof TypeThrowable and + c.getAnArgument() = a + ) + ) +} + private predicate safeUse(Expr e) { exists(AndLogicalExpr andExp | andExp.getType() instanceof BooleanType and andExp.getAnOperand() = e @@ -118,21 +141,32 @@ private predicate safeUse(Expr e) { ( addExp.getRightOperand() = e or - // A method call, like `File.getAbsolutePath()` is being called and concatenated into a string - exists(MethodAccess fileMethodAccess | - fileMethodAccess.getQualifier() = e and - addExp.getRightOperand() = fileMethodAccess and - fileMethodAccess.getMethod().getReturnType() instanceof TypeString - ) + // A method call, like `File.getAbsolutePath()` is being called and concatenated to the end of a a string + addExp.getRightOperand() = getStringAccessOnFile(e) + or + // A method call, like `File.getAbsolutePath()` is being called and prepended to another string with a leading space character. + addExp.getLeftOperand() = getStringAccessOnFile(e) and + addExp.getRightOperand().(CompileTimeConstantExpr).getStringValue().matches(" %") ) ) or + // File is being used to construct an exception message + e = getThrowableConstructorParam() + or // A call to `File::deleteOnExit` exists(MethodAccess ma | ma.getMethod().hasName("deleteOnExit") and ma.getMethod().getDeclaringType() instanceof TypeFile and ma.getQualifier() = e ) + or + // An assignment to a variable that is exclusively used when safe + e = any(VariableAssign assign | safeUse(assign.getDestVar().getAnAccess())).getSource() and + not e = any(VariableAssign assign | not safeUse(assign.getDestVar().getAnAccess())).getSource() + or + // Data flow exists exclusively to locations that are known to be safe + DataFlow::localExprFlow(getStringAccessOnFile(e), any(Expr sink | safeUse(sink))) and + not DataFlow::localExprFlow(getStringAccessOnFile(e), any(Expr sink | not safeUse(sink))) } private class TempDirHijackingFullPath extends TaintTracking::Configuration { diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index aabf46e8983e..e680dff207a1 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -149,5 +149,62 @@ static File vulnerable3() throws IOException { temp.mkdir(); return temp; } + + static File safe11() throws IOException { + File temp = null; + if (temp == null) { + while (true) { + temp = File.createTempFile("test", "directory"); + if (temp.delete() && temp.mkdir()) { + break; + } + } + } + return temp; + } + + File safe12temp; + File safe12() throws IOException { + if (safe12temp == null) { + while (true) { + safe12temp = File.createTempFile("test", "directory"); + if (safe12temp.delete() && safe12temp.mkdir()) { + break; + } + } + } + return safe12temp; + } + + File safe13() throws IOException { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + if (temp.mkdir()) { + return temp; + } else { + throw new IOException(temp.getAbsolutePath() + " could not be created"); + } + } + + File safe14() throws IOException { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + if (temp.mkdir()) { + return temp; + } else { + throw new IOException(temp.getAbsolutePath()); + } + } + + File safe15() throws IOException { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + if (temp.mkdir()) { + return temp; + } else { + final String absolutePath = temp.getAbsolutePath(); + throw new IOException(absolutePath); + } + } } From 84003c1e44291ba98d447a0fa8dd58170598d501 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 18 Mar 2022 16:26:27 -0400 Subject: [PATCH 17/28] Fix some false positive paths with FlowState --- .../CWE-378/TempDirHijackingVulnerability.ql | 62 ++++++++++++++++--- .../TempDirHijackingVulnerability.expected | 27 ++++++++ .../security/CWE-378/semmle/tests/Test.java | 16 +++++ 3 files changed, 97 insertions(+), 8 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index f252c1bcee03..a79422333dcd 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -36,15 +36,29 @@ predicate isDeleteFileExpr(Expr expr) { expr = any(MethodFileDelete m).getAReference().getQualifier() } -private class DirHijackingTaintSource extends DataFlow::Node { - DirHijackingTaintSource() { +abstract private class DirHijackingTaintSource extends DataFlow::Node { + abstract DataFlow::FlowState getFlowState(); +} + +private class FromSystemPropertyFlowState extends DataFlow::FlowState { + FromSystemPropertyFlowState() { this = "FromSystemPropertyFlowState" } +} + +private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTaintSource { + FileCreateTempFileDirHijackingTaintSource() { this.asExpr() = any(MethodAccess ma | ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2 ) - or - this.asExpr() = getSystemProperty("java.io.tmpdir") } + + override DataFlow::FlowState getFlowState() { result instanceof DataFlow::FlowStateEmpty } +} + +private class SystemPropertyDirHijackingTaintSource extends DirHijackingTaintSource { + SystemPropertyDirHijackingTaintSource() { this.asExpr() = getSystemProperty("java.io.tmpdir") } + + override DataFlow::FlowState getFlowState() { result instanceof FromSystemPropertyFlowState } } private predicate isAdditionalTaintStepCommon(DataFlow::Node node1, DataFlow::Node node2) { @@ -76,6 +90,28 @@ private class TempDirHijackingFromDeleteConfig extends DataFlow3::Configuration } } +private class TempDirHijackingFromDirectoryCreateToUnsafeUseConfig extends DataFlow4::Configuration { + TempDirHijackingFromDirectoryCreateToUnsafeUseConfig() { + this = "TempDirHijackingFromDirectoryCreateToUnsafeUseConfig" + } + + override predicate isSource(DataFlow::Node source) { + isNonThrowingDirectoryCreationExpression(source.asExpr(), _) + } + + override predicate isSink(DataFlow::Node sink) { not safeUse(sink.asExpr()) } + + override predicate isBarrierGuard(DataFlow::BarrierGuard guard) { + guard instanceof DirectoryCreationBarrierGuard + } +} + +private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard { + DirectoryCreationBarrierGuard() { isNonThrowingDirectoryCreationExpression(_, this) } + + override predicate checks(Expr e, boolean branch) { this.controls(e, branch) } +} + /** * Holds when there is an expression `unsafeUse` which is an unsafe use of the file that * is not guarded by a check of the return value of `File::mkdir(s)`. @@ -85,7 +121,8 @@ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to `mkdir` or `mkdirs` sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink g = ma and // The guard is the method access - DataFlow::localExprFlow(sink.asExpr(), unsafeUse) and // There is some flow from the sink to an unsafe use of the File + any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c) + .hasFlow(sink, DataFlow::exprNode(unsafeUse)) and // There is some flow from the sink to an unsafe use of the File unsafeUse != sink.asExpr() and // The unsafe use is not the sink itself not safeUse(unsafeUse) and // The unsafe use is not a safe use not g.controls(unsafeUse.getBasicBlock(), true) and @@ -172,14 +209,23 @@ private predicate safeUse(Expr e) { private class TempDirHijackingFullPath extends TaintTracking::Configuration { TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" } - override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource } + override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { + exists(DirHijackingTaintSource taintSource | + source = taintSource and state = taintSource.getFlowState() + ) + } override predicate isSink(DataFlow::Node sink) { isNonThrowingDirectoryCreationExpression(sink.asExpr(), _) } - override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - isAdditionalTaintStepCommon(node1, node2) + override predicate isAdditionalTaintStep( + DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, + DataFlow::FlowState state2 + ) { + isAdditionalTaintStepCommon(node1, node2) and + state1 instanceof FromSystemPropertyFlowState and + state2 instanceof DataFlow::FlowStateEmpty } } diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index 4e377052e4ba..b325b1b374c9 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -16,6 +16,17 @@ edges | Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:149:9:149:12 | temp | | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File | +| Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp | +| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:21:171:30 | this <.field> [safe12temp] : File | +| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:44:171:53 | this <.field> [safe12temp] : File | +| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | +| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | +| Test.java:171:21:171:30 | safe12temp : File | Test.java:171:44:171:53 | safe12temp | +| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | Test.java:171:21:171:30 | safe12temp : File | +| Test.java:171:44:171:53 | this <.field> [safe12temp] : File | Test.java:171:44:171:53 | safe12temp | +| Test.java:180:21:180:60 | createTempFile(...) : File | Test.java:182:13:182:16 | temp | +| Test.java:190:21:190:60 | createTempFile(...) : File | Test.java:192:13:192:16 | temp | +| Test.java:200:21:200:60 | createTempFile(...) : File | Test.java:202:13:202:16 | temp | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:13:13:13:16 | temp | semmle.label | temp | @@ -48,6 +59,20 @@ nodes | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File | | Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String | | Test.java:149:9:149:12 | temp | semmle.label | temp | +| Test.java:157:24:157:63 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:158:38:158:41 | temp | semmle.label | temp | +| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | semmle.label | this <.field> [post update] [safe12temp] : File | +| Test.java:170:30:170:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File | +| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File | +| Test.java:171:44:171:53 | safe12temp | semmle.label | safe12temp | +| Test.java:171:44:171:53 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File | +| Test.java:180:21:180:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:182:13:182:16 | temp | semmle.label | temp | +| Test.java:190:21:190:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:192:13:192:16 | temp | semmle.label | temp | +| Test.java:200:21:200:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:202:13:202:16 | temp | semmle.label | temp | subpaths #select | Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here | @@ -55,3 +80,5 @@ subpaths | Test.java:24:9:24:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:23:9:23:12 | temp | delete here | Test.java:25:16:25:19 | temp | here | | Test.java:131:9:131:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:131:9:131:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:130:9:130:12 | temp | delete here | Test.java:132:16:132:19 | temp | here | | Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here | +| Test.java:158:38:158:41 | temp | Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:158:21:158:24 | temp | delete here | Test.java:163:16:163:19 | temp | here | +| Test.java:171:44:171:53 | safe12temp | Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:171:21:171:30 | safe12temp | delete here | Test.java:176:16:176:25 | safe12temp | here | diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index e680dff207a1..c5d2a1fee8a2 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -206,5 +206,21 @@ File safe15() throws IOException { throw new IOException(absolutePath); } } + + File vulnerable4() throws IOException { + File temp = new File(System.getProperty("java.io.tmpdir")); + ensureDirectory(temp); + File workDir = File.createTempFile("test", "directory", temp); + if (!workDir.delete()) { + throw new IOException("Could not delete temp file: " + workDir.getAbsolutePath()); + } + ensureDirectory(workDir); + return temp; + } + private static void ensureDirectory(File dir) throws IOException { + if (!dir.mkdirs() && !dir.isDirectory()) { + throw new IOException("Mkdirs failed to create " + dir.toString()); + } + } } From 4b6d1a461e64c62b6b79bdc1af3ecc2821647ae6 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 18 Mar 2022 17:31:31 -0400 Subject: [PATCH 18/28] Finalize and document FlowState usage --- .../CWE-378/TempDirHijackingVulnerability.ql | 69 ++++++--- .../TempDirHijackingVulnerability.expected | 134 ++++++++++++++---- 2 files changed, 163 insertions(+), 40 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index a79422333dcd..01c5bc62a8b0 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -18,11 +18,38 @@ import semmle.code.java.environment.SystemProperty import semmle.code.java.security.TempFileLib import DataFlow::PathGraph +/** + * The flow state after the call to `System.getProperty`. + */ +private class FromSystemPropertyFlowState extends DataFlow::FlowState { + FromSystemPropertyFlowState() { this = "FromSystemPropertyFlowState" } +} + +/** + * The flow state after the `File.createTempFile` method has been called on the tracked taint. + * + * This state may be achieved two ways: + * 1. If the source of the taint is a `File.createTempFile(_, _)` call. + * 2. From state `FromSystemPropertyFlowState` if `File.createTempFile(_, _, taint)` is called. + */ +private class FromFileCreateTempFileFlowState extends DataFlow::FlowState { + FromFileCreateTempFileFlowState() { this = "FromFileCreateTempFileFlowState" } +} + +/** + * The flow state after the `File::delete` method has been called on tracked taint. + * This is the final state of the flow before hijackable call `File::mkdir(s)` (ie. the sink) is reached. + */ +private class FromDeleteFileFlowState extends DataFlow::FlowState { + FromDeleteFileFlowState() { this = "FromDeleteFileFlowState" } +} + /** * An expression that will create a directory without throwing an exception if a file/directory already exists. */ -private predicate isNonThrowingDirectoryCreationExpression(Expr expr, MethodAccess creationCall) { - creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = expr +private predicate isNonThrowingDirectoryCreationExpression(Expr qualifier, MethodAccess creationCall) { + creationCall.getMethod() instanceof MethodFileCreatesDirs and + creationCall.getQualifier() = qualifier } private class MethodFileDelete extends Method { @@ -40,10 +67,6 @@ abstract private class DirHijackingTaintSource extends DataFlow::Node { abstract DataFlow::FlowState getFlowState(); } -private class FromSystemPropertyFlowState extends DataFlow::FlowState { - FromSystemPropertyFlowState() { this = "FromSystemPropertyFlowState" } -} - private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTaintSource { FileCreateTempFileDirHijackingTaintSource() { this.asExpr() = @@ -52,7 +75,7 @@ private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTain ) } - override DataFlow::FlowState getFlowState() { result instanceof DataFlow::FlowStateEmpty } + override DataFlow::FlowState getFlowState() { result instanceof FromFileCreateTempFileFlowState } } private class SystemPropertyDirHijackingTaintSource extends DirHijackingTaintSource { @@ -61,9 +84,14 @@ private class SystemPropertyDirHijackingTaintSource extends DirHijackingTaintSou override DataFlow::FlowState getFlowState() { result instanceof FromSystemPropertyFlowState } } -private predicate isAdditionalTaintStepCommon(DataFlow::Node node1, DataFlow::Node node2) { +/** + * Holds when for a call to `File.createTempFile(_, _, node2)` where `node1` is the `MethodAccess`. + */ +private predicate isTaintStepFileCreateTempFile(DataFlow::Node node1, DataFlow::Node node2) { node2.asExpr() = any(MethodAccess ma | + // Argument 2 is the target directory. + // This vulnerability exists when the system property `java.io.tmpdir` is used as the target directory. ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() ) } @@ -74,7 +102,7 @@ private class TempDirHijackingToDeleteConfig extends TaintTracking2::Configurati override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - isAdditionalTaintStepCommon(node1, node2) + isTaintStepFileCreateTempFile(node1, node2) } override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) } @@ -215,17 +243,26 @@ private class TempDirHijackingFullPath extends TaintTracking::Configuration { ) } - override predicate isSink(DataFlow::Node sink) { - isNonThrowingDirectoryCreationExpression(sink.asExpr(), _) - } - override predicate isAdditionalTaintStep( DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2 ) { - isAdditionalTaintStepCommon(node1, node2) and + // From a temporary directory system property access to `File.createTempFile(_, _, flow)` call + isTaintStepFileCreateTempFile(node1, node2) and state1 instanceof FromSystemPropertyFlowState and - state2 instanceof DataFlow::FlowStateEmpty + state2 instanceof FromFileCreateTempFileFlowState + or + // From `File.createTempFile(_, _, flow)` to a call to `File::delete` + node1 = node2 and + isDeleteFileExpr(node1.asExpr()) and + state1 instanceof FromFileCreateTempFileFlowState and + state2 instanceof FromDeleteFileFlowState + } + + override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { + // From a `File::delete` call, to a call to `File::mkdir(s)` + isNonThrowingDirectoryCreationExpression(sink.asExpr(), _) and + state instanceof FromDeleteFileFlowState } } @@ -233,7 +270,7 @@ from DataFlow::PathNode pathSource, DataFlow::PathNode pathSink, DataFlow::Node deleteCheckpoint, MethodAccess creationCall, Expr unsafe where - any(TempDirHijackingFullPath c).hasFlowPath(pathSource, pathSink) and + exists(TempDirHijackingFullPath c | c.hasFlowPath(pathSource, pathSink)) and any(TempDirHijackingToDeleteConfig c).hasFlow(pathSource.getNode(), deleteCheckpoint) and any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index b325b1b374c9..442892ae14a9 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -1,78 +1,163 @@ edges -| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | -| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | -| Test.java:29:21:29:60 | createTempFile(...) : File | Test.java:30:9:30:12 | temp | -| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:37:13:37:16 | temp | -| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:47:29:47:32 | temp | -| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:58:15:58:18 | temp | -| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:68:20:68:23 | temp | -| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:79:20:79:23 | temp | -| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:30:89:33 | temp | -| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:32:98:35 | temp | -| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:48:110:51 | temp | -| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:122:14:122:17 | temp | -| Test.java:129:62:129:107 | new File(...) : File | Test.java:131:9:131:12 | temp | +| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp : File | +| Test.java:12:13:12:16 | temp : File | Test.java:12:13:12:16 | temp : File | +| Test.java:12:13:12:16 | temp : File | Test.java:13:13:13:16 | temp | +| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp : File | +| Test.java:23:9:23:12 | temp : File | Test.java:23:9:23:12 | temp : File | +| Test.java:23:9:23:12 | temp : File | Test.java:24:9:24:12 | temp | +| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:36:9:36:12 | temp : File | +| Test.java:36:9:36:12 | temp : File | Test.java:36:9:36:12 | temp : File | +| Test.java:36:9:36:12 | temp : File | Test.java:37:13:37:16 | temp | +| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:46:9:46:12 | temp : File | +| Test.java:46:9:46:12 | temp : File | Test.java:46:9:46:12 | temp : File | +| Test.java:46:9:46:12 | temp : File | Test.java:47:29:47:32 | temp | +| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:57:9:57:12 | temp : File | +| Test.java:57:9:57:12 | temp : File | Test.java:57:9:57:12 | temp : File | +| Test.java:57:9:57:12 | temp : File | Test.java:58:15:58:18 | temp | +| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:67:20:67:23 | temp : File | +| Test.java:67:20:67:23 | temp : File | Test.java:67:20:67:23 | temp : File | +| Test.java:67:20:67:23 | temp : File | Test.java:68:20:68:23 | temp | +| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:78:20:78:23 | temp : File | +| Test.java:78:20:78:23 | temp : File | Test.java:78:20:78:23 | temp : File | +| Test.java:78:20:78:23 | temp : File | Test.java:79:20:79:23 | temp | +| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:13:89:16 | temp : File | +| Test.java:89:13:89:16 | temp : File | Test.java:89:13:89:16 | temp : File | +| Test.java:89:13:89:16 | temp : File | Test.java:89:30:89:33 | temp | +| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:14:98:17 | temp : File | +| Test.java:98:14:98:17 | temp : File | Test.java:98:14:98:17 | temp : File | +| Test.java:98:14:98:17 | temp : File | Test.java:98:32:98:35 | temp | +| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp : File | +| Test.java:110:30:110:33 | temp : File | Test.java:110:30:110:33 | temp : File | +| Test.java:110:30:110:33 | temp : File | Test.java:110:48:110:51 | temp | +| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp : File | +| Test.java:119:14:119:17 | temp : File | Test.java:119:14:119:17 | temp : File | +| Test.java:119:14:119:17 | temp : File | Test.java:122:14:122:17 | temp | +| Test.java:129:21:129:108 | createTempFile(...) : File | Test.java:130:9:130:12 | temp : File | +| Test.java:129:62:129:107 | new File(...) : File | Test.java:129:21:129:108 | createTempFile(...) : File | | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:129:62:129:107 | new File(...) : File | +| Test.java:130:9:130:12 | temp : File | Test.java:130:9:130:12 | temp : File | +| Test.java:130:9:130:12 | temp : File | Test.java:131:9:131:12 | temp | +| Test.java:143:21:147:9 | createTempFile(...) : File | Test.java:148:9:148:12 | temp : File | | Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | -| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:149:9:149:12 | temp | +| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:143:21:147:9 | createTempFile(...) : File | | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File | -| Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp | +| Test.java:148:9:148:12 | temp : File | Test.java:148:9:148:12 | temp : File | +| Test.java:148:9:148:12 | temp : File | Test.java:149:9:149:12 | temp | +| Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:21:158:24 | temp : File | +| Test.java:158:21:158:24 | temp : File | Test.java:158:21:158:24 | temp : File | +| Test.java:158:21:158:24 | temp : File | Test.java:158:38:158:41 | temp | | Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:21:171:30 | this <.field> [safe12temp] : File | -| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:44:171:53 | this <.field> [safe12temp] : File | | Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | -| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | +| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:21:171:30 | safe12temp : File | +| Test.java:171:21:171:30 | safe12temp : File | Test.java:171:21:171:30 | safe12temp : File | | Test.java:171:21:171:30 | safe12temp : File | Test.java:171:44:171:53 | safe12temp | | Test.java:171:21:171:30 | this <.field> [safe12temp] : File | Test.java:171:21:171:30 | safe12temp : File | -| Test.java:171:44:171:53 | this <.field> [safe12temp] : File | Test.java:171:44:171:53 | safe12temp | -| Test.java:180:21:180:60 | createTempFile(...) : File | Test.java:182:13:182:16 | temp | -| Test.java:190:21:190:60 | createTempFile(...) : File | Test.java:192:13:192:16 | temp | -| Test.java:200:21:200:60 | createTempFile(...) : File | Test.java:202:13:202:16 | temp | +| Test.java:180:21:180:60 | createTempFile(...) : File | Test.java:181:9:181:12 | temp : File | +| Test.java:181:9:181:12 | temp : File | Test.java:181:9:181:12 | temp : File | +| Test.java:181:9:181:12 | temp : File | Test.java:182:13:182:16 | temp | +| Test.java:190:21:190:60 | createTempFile(...) : File | Test.java:191:9:191:12 | temp : File | +| Test.java:191:9:191:12 | temp : File | Test.java:191:9:191:12 | temp : File | +| Test.java:191:9:191:12 | temp : File | Test.java:192:13:192:16 | temp | +| Test.java:200:21:200:60 | createTempFile(...) : File | Test.java:201:9:201:12 | temp : File | +| Test.java:201:9:201:12 | temp : File | Test.java:201:9:201:12 | temp : File | +| Test.java:201:9:201:12 | temp : File | Test.java:202:13:202:16 | temp | +| Test.java:211:21:211:66 | new File(...) : File | Test.java:213:65:213:68 | temp : File | +| Test.java:211:30:211:65 | getProperty(...) : String | Test.java:211:21:211:66 | new File(...) : File | +| Test.java:213:24:213:69 | createTempFile(...) : File | Test.java:214:14:214:20 | workDir : File | +| Test.java:213:65:213:68 | temp : File | Test.java:213:24:213:69 | createTempFile(...) : File | +| Test.java:214:14:214:20 | workDir : File | Test.java:214:14:214:20 | workDir : File | +| Test.java:214:14:214:20 | workDir : File | Test.java:217:25:217:31 | workDir : File | +| Test.java:217:25:217:31 | workDir : File | Test.java:221:41:221:48 | dir : File | +| Test.java:221:41:221:48 | dir : File | Test.java:222:14:222:16 | dir | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:12:13:12:16 | temp : File | semmle.label | temp : File | +| Test.java:12:13:12:16 | temp : File | semmle.label | temp : File | | Test.java:13:13:13:16 | temp | semmle.label | temp | | Test.java:22:21:22:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:23:9:23:12 | temp : File | semmle.label | temp : File | +| Test.java:23:9:23:12 | temp : File | semmle.label | temp : File | | Test.java:24:9:24:12 | temp | semmle.label | temp | -| Test.java:29:21:29:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:30:9:30:12 | temp | semmle.label | temp | | Test.java:35:21:35:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:36:9:36:12 | temp : File | semmle.label | temp : File | +| Test.java:36:9:36:12 | temp : File | semmle.label | temp : File | | Test.java:37:13:37:16 | temp | semmle.label | temp | | Test.java:45:21:45:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:46:9:46:12 | temp : File | semmle.label | temp : File | +| Test.java:46:9:46:12 | temp : File | semmle.label | temp : File | | Test.java:47:29:47:32 | temp | semmle.label | temp | | Test.java:56:21:56:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:57:9:57:12 | temp : File | semmle.label | temp : File | +| Test.java:57:9:57:12 | temp : File | semmle.label | temp : File | | Test.java:58:15:58:18 | temp | semmle.label | temp | | Test.java:66:21:66:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:67:20:67:23 | temp : File | semmle.label | temp : File | +| Test.java:67:20:67:23 | temp : File | semmle.label | temp : File | | Test.java:68:20:68:23 | temp | semmle.label | temp | | Test.java:77:21:77:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:78:20:78:23 | temp : File | semmle.label | temp : File | +| Test.java:78:20:78:23 | temp : File | semmle.label | temp : File | | Test.java:79:20:79:23 | temp | semmle.label | temp | | Test.java:88:21:88:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:89:13:89:16 | temp : File | semmle.label | temp : File | +| Test.java:89:13:89:16 | temp : File | semmle.label | temp : File | | Test.java:89:30:89:33 | temp | semmle.label | temp | | Test.java:97:21:97:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:98:14:98:17 | temp : File | semmle.label | temp : File | +| Test.java:98:14:98:17 | temp : File | semmle.label | temp : File | | Test.java:98:32:98:35 | temp | semmle.label | temp | | Test.java:109:21:109:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:110:30:110:33 | temp : File | semmle.label | temp : File | +| Test.java:110:30:110:33 | temp : File | semmle.label | temp : File | | Test.java:110:48:110:51 | temp | semmle.label | temp | | Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:119:14:119:17 | temp : File | semmle.label | temp : File | +| Test.java:119:14:119:17 | temp : File | semmle.label | temp : File | | Test.java:122:14:122:17 | temp | semmle.label | temp | +| Test.java:129:21:129:108 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:129:62:129:107 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:129:71:129:106 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:130:9:130:12 | temp : File | semmle.label | temp : File | +| Test.java:130:9:130:12 | temp : File | semmle.label | temp : File | | Test.java:131:9:131:12 | temp | semmle.label | temp | +| Test.java:143:21:147:9 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:146:13:146:58 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File | | Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:148:9:148:12 | temp : File | semmle.label | temp : File | +| Test.java:148:9:148:12 | temp : File | semmle.label | temp : File | | Test.java:149:9:149:12 | temp | semmle.label | temp | | Test.java:157:24:157:63 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:158:21:158:24 | temp : File | semmle.label | temp : File | +| Test.java:158:21:158:24 | temp : File | semmle.label | temp : File | | Test.java:158:38:158:41 | temp | semmle.label | temp | | Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | semmle.label | this <.field> [post update] [safe12temp] : File | | Test.java:170:30:170:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File | +| Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File | | Test.java:171:21:171:30 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File | | Test.java:171:44:171:53 | safe12temp | semmle.label | safe12temp | -| Test.java:171:44:171:53 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File | | Test.java:180:21:180:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:181:9:181:12 | temp : File | semmle.label | temp : File | +| Test.java:181:9:181:12 | temp : File | semmle.label | temp : File | | Test.java:182:13:182:16 | temp | semmle.label | temp | | Test.java:190:21:190:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:191:9:191:12 | temp : File | semmle.label | temp : File | +| Test.java:191:9:191:12 | temp : File | semmle.label | temp : File | | Test.java:192:13:192:16 | temp | semmle.label | temp | | Test.java:200:21:200:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:201:9:201:12 | temp : File | semmle.label | temp : File | +| Test.java:201:9:201:12 | temp : File | semmle.label | temp : File | | Test.java:202:13:202:16 | temp | semmle.label | temp | +| Test.java:211:21:211:66 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:211:30:211:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:213:24:213:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:213:65:213:68 | temp : File | semmle.label | temp : File | +| Test.java:214:14:214:20 | workDir : File | semmle.label | workDir : File | +| Test.java:214:14:214:20 | workDir : File | semmle.label | workDir : File | +| Test.java:217:25:217:31 | workDir : File | semmle.label | workDir : File | +| Test.java:221:41:221:48 | dir : File | semmle.label | dir : File | +| Test.java:222:14:222:16 | dir | semmle.label | dir | subpaths #select | Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here | @@ -82,3 +167,4 @@ subpaths | Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here | | Test.java:158:38:158:41 | temp | Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:158:21:158:24 | temp | delete here | Test.java:163:16:163:19 | temp | here | | Test.java:171:44:171:53 | safe12temp | Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:171:21:171:30 | safe12temp | delete here | Test.java:176:16:176:25 | safe12temp | here | +| Test.java:222:14:222:16 | dir | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:222:14:222:16 | dir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:214:14:214:20 | workDir | delete here | Test.java:222:31:222:33 | dir | here | From 325d0e1f67e1db48ef5abf95d87a6c5d733f1716 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Fri, 18 Mar 2022 17:51:20 -0400 Subject: [PATCH 19/28] Add `NullLiteral` flow check for `File.createTempFile` --- .../java/dataflow/internal/DataFlowUtil.qll | 24 +++++++++++++++ .../code/java/environment/SystemProperty.qll | 30 +++---------------- .../TempDirLocalInformationDisclosure.ql | 2 +- .../CWE-378/TempDirHijackingVulnerability.ql | 12 ++++++-- .../security/CWE-378/semmle/tests/Test.java | 7 +++++ 5 files changed, 46 insertions(+), 29 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll index 02f4dae50384..38f535ff4abc 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll @@ -84,6 +84,30 @@ predicate localFlow(Node node1, Node node2) { localFlowStep*(node1, node2) } pragma[inline] predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2)) } +/** + * Holds if data can flow from `e1` to `e2` in zero or more + * local (intra-procedural) steps or via local variable intializers + * for final variables. + */ +pragma[inline] +predicate localExprFlowPlusInitializers(Expr e1, Expr e2) { + localFlowPlusInitializers(exprNode(e1), exprNode(e2)) +} + +/** + * Holds if data can flow from `pred` to `succ` in zero or more + * local (intra-procedural) steps or via instance or static variable intializers + * for final variables. + */ +pragma[inline] +predicate localFlowPlusInitializers(Node pred, Node succ) { + exists(Variable v | v.isFinal() and pred.asExpr() = v.getInitializer() | + localFlow(exprNode(v.getAnAccess()), succ) + ) + or + localFlow(pred, succ) +} + /** * Holds if the `FieldRead` is not completely determined by explicit SSA * updates. diff --git a/java/ql/lib/semmle/code/java/environment/SystemProperty.qll b/java/ql/lib/semmle/code/java/environment/SystemProperty.qll index 6a3ffde76ebe..36927070af51 100644 --- a/java/ql/lib/semmle/code/java/environment/SystemProperty.qll +++ b/java/ql/lib/semmle/code/java/environment/SystemProperty.qll @@ -42,7 +42,7 @@ private MethodAccess getSystemPropertyFromSystemGetProperties(string propertyNam result.getMethod() = getMethod ) and result.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName and - localExprFlowPlusInitializers(any(MethodAccess m | + DataFlow::localExprFlowPlusInitializers(any(MethodAccess m | m.getMethod().getDeclaringType() instanceof TypeSystem and m.getMethod().hasName("getProperties") ), result.getQualifier()) @@ -172,15 +172,15 @@ private MethodAccess getSystemPropertyFromGuava(string propertyName) { ec.getDeclaringType().hasQualifiedName("com.google.common.base", "StandardSystemProperty") and // Example: `StandardSystemProperty.JAVA_IO_TMPDIR.value()` ( - localExprFlowPlusInitializers(ec.getAnAccess(), result.getQualifier()) and + DataFlow::localExprFlowPlusInitializers(ec.getAnAccess(), result.getQualifier()) and result.getMethod().hasName("value") ) or // Example: `System.getProperty(StandardSystemProperty.JAVA_IO_TMPDIR.key())` exists(MethodAccess keyMa | - localExprFlowPlusInitializers(ec.getAnAccess(), keyMa.getQualifier()) and + DataFlow::localExprFlowPlusInitializers(ec.getAnAccess(), keyMa.getQualifier()) and keyMa.getMethod().hasName("key") and - localExprFlowPlusInitializers(keyMa, result.(MethodAccessSystemGetProperty).getArgument(0)) + DataFlow::localExprFlowPlusInitializers(keyMa, result.(MethodAccessSystemGetProperty).getArgument(0)) ) | ec.hasName("JAVA_VERSION") and propertyName = "java.version" @@ -262,25 +262,3 @@ private MethodAccess getSystemPropertyFromSpringProperties(string propertyName) ) and result.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName } - -/** - * Holds if data can flow from `e1` to `e2` in zero or more - * local (intra-procedural) steps or via local variable intializers - * for final variables. - */ -private predicate localExprFlowPlusInitializers(Expr e1, Expr e2) { - localFlowPlusInitializers(DataFlow::exprNode(e1), DataFlow::exprNode(e2)) -} - -/** - * Holds if data can flow from `pred` to `succ` in zero or more - * local (intra-procedural) steps or via instance or static variable intializers - * for final variables. - */ -private predicate localFlowPlusInitializers(DataFlow::Node pred, DataFlow::Node succ) { - exists(Variable v | v.isFinal() and pred.asExpr() = v.getInitializer() | - DataFlow::localFlow(DataFlow::exprNode(v.getAnAccess()), succ) - ) - or - DataFlow::localFlow(pred, succ) -} diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index 5fb0add69bbc..c282922a0d6a 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -205,7 +205,7 @@ class MethodAccessInsecureFileCreateTempFile extends MethodAccessInsecureFileCre this.getNumArgument() = 2 or // The default temporary directory is used when the last argument of `File.createTempFile(string, string, File)` is `null` - DataFlow::localExprFlow(any(NullLiteral n), this.getArgument(2)) + DataFlow::localExprFlowPlusInitializers(any(NullLiteral n), this.getArgument(2)) ) } diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 01c5bc62a8b0..fa8828a716fa 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -71,7 +71,15 @@ private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTain FileCreateTempFileDirHijackingTaintSource() { this.asExpr() = any(MethodAccess ma | - ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2 + // The two argument variant of this method uses the system property `java.io.tmpdir` as the base directory. + ma.getMethod() instanceof MethodFileCreateTempFile and + ( + // The two argument variant of this method uses the system property `java.io.tmpdir` as the base directory. + ma.getNumArgument() = 2 + or + // The three argument variant of this method uses the system property `java.io.tmpdir` as the base directory when `null`. + DataFlow::localExprFlowPlusInitializers(any(NullLiteral n), ma.getArgument(2)) + ) ) } @@ -90,7 +98,7 @@ private class SystemPropertyDirHijackingTaintSource extends DirHijackingTaintSou private predicate isTaintStepFileCreateTempFile(DataFlow::Node node1, DataFlow::Node node2) { node2.asExpr() = any(MethodAccess ma | - // Argument 2 is the target directory. + // Argument two is the target directory. // This vulnerability exists when the system property `java.io.tmpdir` is used as the target directory. ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() ) diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index c5d2a1fee8a2..6d03542ffc96 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -223,4 +223,11 @@ private static void ensureDirectory(File dir) throws IOException { throw new IOException("Mkdirs failed to create " + dir.toString()); } } + + static File vulnerable5() throws IOException { + File temp = File.createTempFile("test", "directory", null); + temp.delete(); + temp.mkdir(); + return temp; + } } From 71f5fc550c2a80e73656fa467c56a649489302d2 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Tue, 29 Mar 2022 19:05:38 -0400 Subject: [PATCH 20/28] Add additional tests and better tracking of 'unsafe use' --- .../CWE-378/TempDirHijackingVulnerability.ql | 183 +++++++++++++++--- .../TempDirHijackingVulnerability.expected | 93 +++++---- .../security/CWE-378/semmle/tests/Test.java | 49 ++++- 3 files changed, 260 insertions(+), 65 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index fa8828a716fa..37f7c42c5cf4 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -44,18 +44,93 @@ private class FromDeleteFileFlowState extends DataFlow::FlowState { FromDeleteFileFlowState() { this = "FromDeleteFileFlowState" } } +/** + * The flow state after the `File::mkdir(s)` method has been called on tracked taint. + * This is the final state, and is only used when computing the 'unsafe' call site. + */ +private class FromMkdirsFileFlowState extends DataFlow::FlowState { + FromMkdirsFileFlowState() { this = "FromMkdirsFileFlowState" } +} + +/** + * A guard that is checking if a directory exists, throwing if it does not exist. + */ +private Guard directoryExistsGuard(ThrowStmt s) { + result = + any(MethodAccess existanceCheck | + ( + existanceCheck.getMethod() instanceof MethodFileExists + or + existanceCheck.getMethod() instanceof MethodFileIsDirectory + ) + ) and + result.directlyControls(s.getEnclosingStmt(), false) +} + +/** + * A guard that is verifying that the directory is sucsessfully created, throwing when it is not created. + */ +private Guard directoryCreationGuard(ThrowStmt s) { + result = any(MethodAccess creationCheck | + creationCheck.getMethod() instanceof MethodFileCreatesDirs + ) and + result.directlyControls(s.getEnclosingStmt(), false) +} + +/** + * A guard that is safely verfying that a directory is created. + * + * 'Safe' means that the directory is guarnteed to have been created, and the directory did not already exist. + */ +private Guard safeDirectoryCreationGuard(ThrowStmt s) { + result = directoryCreationGuard(s) and + // This guard is not 'safe' if a `directoryExistsGuard` is also guarding this throw statement + not exists(directoryExistsGuard(s)) +} + /** * An expression that will create a directory without throwing an exception if a file/directory already exists. */ -private predicate isNonThrowingDirectoryCreationExpression(Expr qualifier, MethodAccess creationCall) { +private predicate isNonThrowingDirectoryCreationExpression( + Expr fileInstanceExpr, MethodAccess creationCall +) { creationCall.getMethod() instanceof MethodFileCreatesDirs and - creationCall.getQualifier() = qualifier + creationCall.getQualifier() = fileInstanceExpr and + // Check for 'throwing creations' and ensure that this call is not used in a throwing case. + if creationCall.(Guard).directlyControls(_, _) then // The creation call is used in a guard + not creationCall = safeDirectoryCreationGuard(_) // Ensure that the guard is insufficient. + else any() // The creation call is not used in a guard. Thus is not a 'throwing creation'. + or + // Recursively look for methods that encapsulate the above. + // Thus, the use of 'helper directory creation methods' are still considered + // when assessing if an `unsafeUse` is present or not. + exists(Method m, int argumentIndex, Expr arg | + arg.(VarAccess).getVariable() = m.getParameter(argumentIndex) and + isNonThrowingDirectoryCreationExpression(any(Expr e2 | DataFlow::localExprFlow(arg, e2)), + creationCall) + | + m.getAReference().getArgument(argumentIndex) = fileInstanceExpr + ) } private class MethodFileDelete extends Method { MethodFileDelete() { - getDeclaringType() instanceof TypeFile and - hasName("delete") + this.getDeclaringType() instanceof TypeFile and + this.hasName("delete") + } +} + +private class MethodFileIsDirectory extends Method { + MethodFileIsDirectory() { + this.getDeclaringType() instanceof TypeFile and + this.hasName("isDirectory") + } +} + +private class MethodFileExists extends Method { + MethodFileExists() { + this.getDeclaringType() instanceof TypeFile and + this.hasName("exists") } } @@ -143,7 +218,13 @@ private class TempDirHijackingFromDirectoryCreateToUnsafeUseConfig extends DataF } private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard { - DirectoryCreationBarrierGuard() { isNonThrowingDirectoryCreationExpression(_, this) } + Expr fileInstanceExpr; + + DirectoryCreationBarrierGuard() { + isNonThrowingDirectoryCreationExpression(fileInstanceExpr, this) + } + + Expr getFileInstanceExpr() { result = fileInstanceExpr } override predicate checks(Expr e, boolean branch) { this.controls(e, branch) } } @@ -153,13 +234,18 @@ private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard { * is not guarded by a check of the return value of `File::mkdir(s)`. */ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) { - exists(Guard g, MethodAccess ma | + exists(DirectoryCreationBarrierGuard g, MethodAccess ma | any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to `mkdir` or `mkdirs` sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink g = ma and // The guard is the method access - any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c) - .hasFlow(sink, DataFlow::exprNode(unsafeUse)) and // There is some flow from the sink to an unsafe use of the File + ( // TODO: Consider replacing this entire bit of logic with `TempDirHijackingFullPathInlcudingUnsafeUse`. + any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c) + .hasFlow(sink, DataFlow::exprNode(unsafeUse)) // There is some flow from the sink to an unsafe use of the File + or + DataFlow::localExprFlow(g.getFileInstanceExpr(), unsafeUse) // There is some local flow from the passed file instance to an unsafe use + ) and unsafeUse != sink.asExpr() and // The unsafe use is not the sink itself + not g.getFileInstanceExpr() = unsafeUse and // The unsafe use is not the file instance not safeUse(unsafeUse) and // The unsafe use is not a safe use not g.controls(unsafeUse.getBasicBlock(), true) and not booleanVarAccessGuardsGuard(g) // The guard is not guarded by a boolean variable access guard @@ -242,29 +328,39 @@ private predicate safeUse(Expr e) { not DataFlow::localExprFlow(getStringAccessOnFile(e), any(Expr sink | not safeUse(sink))) } +private predicate isSourceUnified(DataFlow::Node source, DataFlow::FlowState state) { + exists(DirHijackingTaintSource taintSource | + source = taintSource and state = taintSource.getFlowState() + ) +} + +private predicate isAdditionalTaintStepUnified( + DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2 +) { + // From a temporary directory system property access to `File.createTempFile(_, _, flow)` call + isTaintStepFileCreateTempFile(node1, node2) and + state1 instanceof FromSystemPropertyFlowState and + state2 instanceof FromFileCreateTempFileFlowState + or + // From `File.createTempFile(_, _, flow)` to a call to `File::delete` + node1 = node2 and + isDeleteFileExpr(node1.asExpr()) and + state1 instanceof FromFileCreateTempFileFlowState and + state2 instanceof FromDeleteFileFlowState +} + private class TempDirHijackingFullPath extends TaintTracking::Configuration { TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" } override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { - exists(DirHijackingTaintSource taintSource | - source = taintSource and state = taintSource.getFlowState() - ) + isSourceUnified(source, state) } override predicate isAdditionalTaintStep( DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2 ) { - // From a temporary directory system property access to `File.createTempFile(_, _, flow)` call - isTaintStepFileCreateTempFile(node1, node2) and - state1 instanceof FromSystemPropertyFlowState and - state2 instanceof FromFileCreateTempFileFlowState - or - // From `File.createTempFile(_, _, flow)` to a call to `File::delete` - node1 = node2 and - isDeleteFileExpr(node1.asExpr()) and - state1 instanceof FromFileCreateTempFileFlowState and - state2 instanceof FromDeleteFileFlowState + isAdditionalTaintStepUnified(node1, state1, node2, state2) } override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { @@ -274,14 +370,57 @@ private class TempDirHijackingFullPath extends TaintTracking::Configuration { } } +/** + * A configuration that tracks the full path of the temporary directory taint all the way to an + * `unsafeUse` of the potentially hijacked temporary directory. + * + * This is intentionally not used in the the generation of the displayed path; + * this is because there may not exist an explicit path from the call to `File::mkdir(s)` call to the the `unsafeUse`. + */ +class TempDirHijackingFullPathInlcudingUnsafeUse extends TaintTracking2::Configuration { + TempDirHijackingFullPathInlcudingUnsafeUse() { + this = "TempDirHijackingFullPathInlcudingUnsafeUse" + } + + override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { + isSourceUnified(source, state) + } + + override predicate isAdditionalTaintStep( + DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, + DataFlow::FlowState state2 + ) { + isAdditionalTaintStepUnified(node1, state1, node2, state2) + or // `File::mkdir(s)` is not an end-state when looking for an unsafe use + isNonThrowingDirectoryCreationExpression(node1.asExpr(), _) and + node1.asExpr() = node2.asExpr() and + state1 instanceof FromDeleteFileFlowState and + state2 instanceof FromMkdirsFileFlowState + } + + override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { + // From a `File::delete` call, to a call to `File::mkdir(s)` + isUnsafeUseUnconstrainedByIfCheck(_, sink.asExpr()) and + state instanceof FromMkdirsFileFlowState + } +} + from DataFlow::PathNode pathSource, DataFlow::PathNode pathSink, DataFlow::Node deleteCheckpoint, MethodAccess creationCall, Expr unsafe where - exists(TempDirHijackingFullPath c | c.hasFlowPath(pathSource, pathSink)) and + // Generate the full path to display to the user + any(TempDirHijackingFullPath c).hasFlowPath(pathSource, pathSink) and + // Find the delete checkpoint to display to the user any(TempDirHijackingToDeleteConfig c).hasFlow(pathSource.getNode(), deleteCheckpoint) and + // Find the delete checkpoint to display to the user any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and + // Find a full path where an `unsafe` use is present + any(TempDirHijackingFullPathInlcudingUnsafeUse c) + .hasFlow(pathSource.getNode(), DataFlow::exprNode(unsafe)) and + // Verify the unsafe use is not constrained by an if check isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and + // Verfy the creation call expression is the expected sink isNonThrowingDirectoryCreationExpression(pathSink.getNode().asExpr(), creationCall) select pathSink, pathSource, pathSink, "Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user.", diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index 442892ae14a9..5c3fa961def2 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -5,15 +5,9 @@ edges | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp : File | | Test.java:23:9:23:12 | temp : File | Test.java:23:9:23:12 | temp : File | | Test.java:23:9:23:12 | temp : File | Test.java:24:9:24:12 | temp | -| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:36:9:36:12 | temp : File | -| Test.java:36:9:36:12 | temp : File | Test.java:36:9:36:12 | temp : File | -| Test.java:36:9:36:12 | temp : File | Test.java:37:13:37:16 | temp | | Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:46:9:46:12 | temp : File | | Test.java:46:9:46:12 | temp : File | Test.java:46:9:46:12 | temp : File | | Test.java:46:9:46:12 | temp : File | Test.java:47:29:47:32 | temp | -| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:57:9:57:12 | temp : File | -| Test.java:57:9:57:12 | temp : File | Test.java:57:9:57:12 | temp : File | -| Test.java:57:9:57:12 | temp : File | Test.java:58:15:58:18 | temp | | Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:67:20:67:23 | temp : File | | Test.java:67:20:67:23 | temp : File | Test.java:67:20:67:23 | temp : File | | Test.java:67:20:67:23 | temp : File | Test.java:68:20:68:23 | temp | @@ -29,9 +23,6 @@ edges | Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp : File | | Test.java:110:30:110:33 | temp : File | Test.java:110:30:110:33 | temp : File | | Test.java:110:30:110:33 | temp : File | Test.java:110:48:110:51 | temp | -| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp : File | -| Test.java:119:14:119:17 | temp : File | Test.java:119:14:119:17 | temp : File | -| Test.java:119:14:119:17 | temp : File | Test.java:122:14:122:17 | temp | | Test.java:129:21:129:108 | createTempFile(...) : File | Test.java:130:9:130:12 | temp : File | | Test.java:129:62:129:107 | new File(...) : File | Test.java:129:21:129:108 | createTempFile(...) : File | | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:129:62:129:107 | new File(...) : File | @@ -52,23 +43,36 @@ edges | Test.java:171:21:171:30 | safe12temp : File | Test.java:171:21:171:30 | safe12temp : File | | Test.java:171:21:171:30 | safe12temp : File | Test.java:171:44:171:53 | safe12temp | | Test.java:171:21:171:30 | this <.field> [safe12temp] : File | Test.java:171:21:171:30 | safe12temp : File | -| Test.java:180:21:180:60 | createTempFile(...) : File | Test.java:181:9:181:12 | temp : File | -| Test.java:181:9:181:12 | temp : File | Test.java:181:9:181:12 | temp : File | -| Test.java:181:9:181:12 | temp : File | Test.java:182:13:182:16 | temp | -| Test.java:190:21:190:60 | createTempFile(...) : File | Test.java:191:9:191:12 | temp : File | -| Test.java:191:9:191:12 | temp : File | Test.java:191:9:191:12 | temp : File | -| Test.java:191:9:191:12 | temp : File | Test.java:192:13:192:16 | temp | -| Test.java:200:21:200:60 | createTempFile(...) : File | Test.java:201:9:201:12 | temp : File | -| Test.java:201:9:201:12 | temp : File | Test.java:201:9:201:12 | temp : File | -| Test.java:201:9:201:12 | temp : File | Test.java:202:13:202:16 | temp | | Test.java:211:21:211:66 | new File(...) : File | Test.java:213:65:213:68 | temp : File | | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:211:21:211:66 | new File(...) : File | | Test.java:213:24:213:69 | createTempFile(...) : File | Test.java:214:14:214:20 | workDir : File | | Test.java:213:65:213:68 | temp : File | Test.java:213:24:213:69 | createTempFile(...) : File | | Test.java:214:14:214:20 | workDir : File | Test.java:214:14:214:20 | workDir : File | +| Test.java:214:14:214:20 | workDir : File | Test.java:217:25:217:31 | workDir | | Test.java:214:14:214:20 | workDir : File | Test.java:217:25:217:31 | workDir : File | | Test.java:217:25:217:31 | workDir : File | Test.java:221:41:221:48 | dir : File | | Test.java:221:41:221:48 | dir : File | Test.java:222:14:222:16 | dir | +| Test.java:228:21:228:66 | createTempFile(...) : File | Test.java:229:9:229:12 | temp : File | +| Test.java:229:9:229:12 | temp : File | Test.java:229:9:229:12 | temp : File | +| Test.java:229:9:229:12 | temp : File | Test.java:230:9:230:12 | temp | +| Test.java:235:21:235:66 | new File(...) : File | Test.java:237:65:237:68 | temp : File | +| Test.java:235:30:235:65 | getProperty(...) : String | Test.java:235:21:235:66 | new File(...) : File | +| Test.java:237:24:237:69 | createTempFile(...) : File | Test.java:238:14:238:20 | workDir : File | +| Test.java:237:65:237:68 | temp : File | Test.java:237:24:237:69 | createTempFile(...) : File | +| Test.java:238:14:238:20 | workDir : File | Test.java:238:14:238:20 | workDir : File | +| Test.java:238:14:238:20 | workDir : File | Test.java:241:33:241:39 | workDir | +| Test.java:238:14:238:20 | workDir : File | Test.java:241:33:241:39 | workDir : File | +| Test.java:241:33:241:39 | workDir : File | Test.java:245:49:245:56 | dir : File | +| Test.java:245:49:245:56 | dir : File | Test.java:248:36:248:38 | dir | +| Test.java:254:21:254:66 | new File(...) : File | Test.java:256:65:256:68 | temp : File | +| Test.java:254:30:254:65 | getProperty(...) : String | Test.java:254:21:254:66 | new File(...) : File | +| Test.java:256:24:256:69 | createTempFile(...) : File | Test.java:257:9:257:15 | workDir : File | +| Test.java:256:65:256:68 | temp : File | Test.java:256:24:256:69 | createTempFile(...) : File | +| Test.java:257:9:257:15 | workDir : File | Test.java:257:9:257:15 | workDir : File | +| Test.java:257:9:257:15 | workDir : File | Test.java:258:23:258:29 | workDir | +| Test.java:257:9:257:15 | workDir : File | Test.java:258:23:258:29 | workDir : File | +| Test.java:258:23:258:29 | workDir : File | Test.java:262:31:262:39 | file : File | +| Test.java:262:31:262:39 | file : File | Test.java:263:9:263:12 | file | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:12:13:12:16 | temp : File | semmle.label | temp : File | @@ -78,18 +82,10 @@ nodes | Test.java:23:9:23:12 | temp : File | semmle.label | temp : File | | Test.java:23:9:23:12 | temp : File | semmle.label | temp : File | | Test.java:24:9:24:12 | temp | semmle.label | temp | -| Test.java:35:21:35:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:36:9:36:12 | temp : File | semmle.label | temp : File | -| Test.java:36:9:36:12 | temp : File | semmle.label | temp : File | -| Test.java:37:13:37:16 | temp | semmle.label | temp | | Test.java:45:21:45:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:46:9:46:12 | temp : File | semmle.label | temp : File | | Test.java:46:9:46:12 | temp : File | semmle.label | temp : File | | Test.java:47:29:47:32 | temp | semmle.label | temp | -| Test.java:56:21:56:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:57:9:57:12 | temp : File | semmle.label | temp : File | -| Test.java:57:9:57:12 | temp : File | semmle.label | temp : File | -| Test.java:58:15:58:18 | temp | semmle.label | temp | | Test.java:66:21:66:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:67:20:67:23 | temp : File | semmle.label | temp : File | | Test.java:67:20:67:23 | temp : File | semmle.label | temp : File | @@ -110,10 +106,6 @@ nodes | Test.java:110:30:110:33 | temp : File | semmle.label | temp : File | | Test.java:110:30:110:33 | temp : File | semmle.label | temp : File | | Test.java:110:48:110:51 | temp | semmle.label | temp | -| Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:119:14:119:17 | temp : File | semmle.label | temp : File | -| Test.java:119:14:119:17 | temp : File | semmle.label | temp : File | -| Test.java:122:14:122:17 | temp | semmle.label | temp | | Test.java:129:21:129:108 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:129:62:129:107 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:129:71:129:106 | getProperty(...) : String | semmle.label | getProperty(...) : String | @@ -137,27 +129,40 @@ nodes | Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File | | Test.java:171:21:171:30 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File | | Test.java:171:44:171:53 | safe12temp | semmle.label | safe12temp | -| Test.java:180:21:180:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:181:9:181:12 | temp : File | semmle.label | temp : File | -| Test.java:181:9:181:12 | temp : File | semmle.label | temp : File | -| Test.java:182:13:182:16 | temp | semmle.label | temp | -| Test.java:190:21:190:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:191:9:191:12 | temp : File | semmle.label | temp : File | -| Test.java:191:9:191:12 | temp : File | semmle.label | temp : File | -| Test.java:192:13:192:16 | temp | semmle.label | temp | -| Test.java:200:21:200:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:201:9:201:12 | temp : File | semmle.label | temp : File | -| Test.java:201:9:201:12 | temp : File | semmle.label | temp : File | -| Test.java:202:13:202:16 | temp | semmle.label | temp | | Test.java:211:21:211:66 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:211:30:211:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | | Test.java:213:24:213:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:213:65:213:68 | temp : File | semmle.label | temp : File | | Test.java:214:14:214:20 | workDir : File | semmle.label | workDir : File | | Test.java:214:14:214:20 | workDir : File | semmle.label | workDir : File | +| Test.java:217:25:217:31 | workDir | semmle.label | workDir | | Test.java:217:25:217:31 | workDir : File | semmle.label | workDir : File | | Test.java:221:41:221:48 | dir : File | semmle.label | dir : File | | Test.java:222:14:222:16 | dir | semmle.label | dir | +| Test.java:228:21:228:66 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:229:9:229:12 | temp : File | semmle.label | temp : File | +| Test.java:229:9:229:12 | temp : File | semmle.label | temp : File | +| Test.java:230:9:230:12 | temp | semmle.label | temp | +| Test.java:235:21:235:66 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:235:30:235:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:237:24:237:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:237:65:237:68 | temp : File | semmle.label | temp : File | +| Test.java:238:14:238:20 | workDir : File | semmle.label | workDir : File | +| Test.java:238:14:238:20 | workDir : File | semmle.label | workDir : File | +| Test.java:241:33:241:39 | workDir | semmle.label | workDir | +| Test.java:241:33:241:39 | workDir : File | semmle.label | workDir : File | +| Test.java:245:49:245:56 | dir : File | semmle.label | dir : File | +| Test.java:248:36:248:38 | dir | semmle.label | dir | +| Test.java:254:21:254:66 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:254:30:254:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:256:24:256:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:256:65:256:68 | temp : File | semmle.label | temp : File | +| Test.java:257:9:257:15 | workDir : File | semmle.label | workDir : File | +| Test.java:257:9:257:15 | workDir : File | semmle.label | workDir : File | +| Test.java:258:23:258:29 | workDir | semmle.label | workDir | +| Test.java:258:23:258:29 | workDir : File | semmle.label | workDir : File | +| Test.java:262:31:262:39 | file : File | semmle.label | file : File | +| Test.java:263:9:263:12 | file | semmle.label | file | subpaths #select | Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here | @@ -167,4 +172,8 @@ subpaths | Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here | | Test.java:158:38:158:41 | temp | Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:158:21:158:24 | temp | delete here | Test.java:163:16:163:19 | temp | here | | Test.java:171:44:171:53 | safe12temp | Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:171:21:171:30 | safe12temp | delete here | Test.java:176:16:176:25 | safe12temp | here | +| Test.java:222:14:222:16 | dir | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:222:14:222:16 | dir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:214:14:214:20 | workDir | delete here | Test.java:218:16:218:22 | workDir | here | | Test.java:222:14:222:16 | dir | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:222:14:222:16 | dir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:214:14:214:20 | workDir | delete here | Test.java:222:31:222:33 | dir | here | +| Test.java:230:9:230:12 | temp | Test.java:228:21:228:66 | createTempFile(...) : File | Test.java:230:9:230:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:229:9:229:12 | temp | delete here | Test.java:231:16:231:19 | temp | here | +| Test.java:248:36:248:38 | dir | Test.java:235:30:235:65 | getProperty(...) : String | Test.java:248:36:248:38 | dir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:238:14:238:20 | workDir | delete here | Test.java:242:16:242:22 | workDir | here | +| Test.java:263:9:263:12 | file | Test.java:254:30:254:65 | getProperty(...) : String | Test.java:263:9:263:12 | file | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:257:9:257:15 | workDir | delete here | Test.java:259:16:259:22 | workDir | here | diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index 6d03542ffc96..ceb7797380b4 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -215,7 +215,7 @@ File vulnerable4() throws IOException { throw new IOException("Could not delete temp file: " + workDir.getAbsolutePath()); } ensureDirectory(workDir); - return temp; + return workDir; } private static void ensureDirectory(File dir) throws IOException { @@ -230,4 +230,51 @@ static File vulnerable5() throws IOException { temp.mkdir(); return temp; } + + File vulnerable6() throws IOException { + File temp = new File(System.getProperty("java.io.tmpdir")); + ensureDirectoryReversed(temp); + File workDir = File.createTempFile("test", "directory", temp); + if (!workDir.delete()) { + throw new IOException("Could not delete temp file: " + workDir.getAbsolutePath()); + } + ensureDirectoryReversed(workDir); + return workDir; + } + + private static void ensureDirectoryReversed(File dir) throws IOException { + // If the directory already exists, don't create it. If it's not, create it. + // Note: this is still vulnerable because the race condition still exists + if (!(dir.isDirectory() || dir.mkdirs())) { + throw new IOException("Mkdirs failed to create " + dir.toString()); + } + } + + File vulnerable7() throws IOException { + File temp = new File(System.getProperty("java.io.tmpdir")); + mkdirsWrapper(temp); + File workDir = File.createTempFile("test", "directory", temp); + workDir.delete(); + mkdirsWrapper(workDir); + return workDir; + } + + static void mkdirsWrapper(File file) { + file.mkdirs(); + } + + File safe16() throws IOException { + File temp = new File(System.getProperty("java.io.tmpdir")); + mkdirsWrapperSafe(temp); + File workDir = File.createTempFile("test", "directory", temp); + workDir.delete(); + mkdirsWrapperSafe(workDir); + return workDir; + } + + static void mkdirsWrapperSafe(File file) throws IOException { + if(!file.mkdirs()) { + throw new IOException("Mkdirs failed to create " + file.toString()); + } + } } From 140c66e6b4130e9ef8607bb7c7714fc0d79e48ce Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 30 Mar 2022 11:07:08 -0400 Subject: [PATCH 21/28] Add additional tests cases for mkdir wrapper method checking --- .../CWE-378/TempDirHijackingVulnerability.ql | 19 +++++---- .../TempDirHijackingVulnerability.expected | 40 +++++++++++++++++++ .../security/CWE-378/semmle/tests/Test.java | 30 ++++++++++++++ 3 files changed, 81 insertions(+), 8 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 37f7c42c5cf4..49b147dcb820 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -64,22 +64,21 @@ private Guard directoryExistsGuard(ThrowStmt s) { existanceCheck.getMethod() instanceof MethodFileIsDirectory ) ) and - result.directlyControls(s.getEnclosingStmt(), false) + result.directlyControls(s.getEnclosingStmt(), false) } /** * A guard that is verifying that the directory is sucsessfully created, throwing when it is not created. */ private Guard directoryCreationGuard(ThrowStmt s) { - result = any(MethodAccess creationCheck | - creationCheck.getMethod() instanceof MethodFileCreatesDirs - ) and + result = + any(MethodAccess creationCheck | creationCheck.getMethod() instanceof MethodFileCreatesDirs) and result.directlyControls(s.getEnclosingStmt(), false) } /** * A guard that is safely verfying that a directory is created. - * + * * 'Safe' means that the directory is guarnteed to have been created, and the directory did not already exist. */ private Guard safeDirectoryCreationGuard(ThrowStmt s) { @@ -97,7 +96,9 @@ private predicate isNonThrowingDirectoryCreationExpression( creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = fileInstanceExpr and // Check for 'throwing creations' and ensure that this call is not used in a throwing case. - if creationCall.(Guard).directlyControls(_, _) then // The creation call is used in a guard + if creationCall.(Guard).directlyControls(_, _) + then + // The creation call is used in a guard not creationCall = safeDirectoryCreationGuard(_) // Ensure that the guard is insufficient. else any() // The creation call is not used in a guard. Thus is not a 'throwing creation'. or @@ -238,7 +239,8 @@ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to `mkdir` or `mkdirs` sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink g = ma and // The guard is the method access - ( // TODO: Consider replacing this entire bit of logic with `TempDirHijackingFullPathInlcudingUnsafeUse`. + ( + // TODO: Consider replacing this entire bit of logic with `TempDirHijackingFullPathInlcudingUnsafeUse`. any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c) .hasFlow(sink, DataFlow::exprNode(unsafeUse)) // There is some flow from the sink to an unsafe use of the File or @@ -391,7 +393,8 @@ class TempDirHijackingFullPathInlcudingUnsafeUse extends TaintTracking2::Configu DataFlow::FlowState state2 ) { isAdditionalTaintStepUnified(node1, state1, node2, state2) - or // `File::mkdir(s)` is not an end-state when looking for an unsafe use + or + // `File::mkdir(s)` is not an end-state when looking for an unsafe use isNonThrowingDirectoryCreationExpression(node1.asExpr(), _) and node1.asExpr() = node2.asExpr() and state1 instanceof FromDeleteFileFlowState and diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index 5c3fa961def2..05072f2763c6 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -73,6 +73,24 @@ edges | Test.java:257:9:257:15 | workDir : File | Test.java:258:23:258:29 | workDir : File | | Test.java:258:23:258:29 | workDir : File | Test.java:262:31:262:39 | file : File | | Test.java:262:31:262:39 | file : File | Test.java:263:9:263:12 | file | +| Test.java:282:21:282:66 | new File(...) : File | Test.java:284:65:284:68 | temp : File | +| Test.java:282:30:282:65 | getProperty(...) : String | Test.java:282:21:282:66 | new File(...) : File | +| Test.java:284:24:284:69 | createTempFile(...) : File | Test.java:285:9:285:15 | workDir : File | +| Test.java:284:65:284:68 | temp : File | Test.java:284:24:284:69 | createTempFile(...) : File | +| Test.java:285:9:285:15 | workDir : File | Test.java:285:9:285:15 | workDir : File | +| Test.java:285:9:285:15 | workDir : File | Test.java:286:24:286:30 | workDir | +| Test.java:285:9:285:15 | workDir : File | Test.java:286:24:286:30 | workDir : File | +| Test.java:286:24:286:30 | workDir : File | Test.java:290:32:290:40 | file : File | +| Test.java:290:32:290:40 | file : File | Test.java:292:9:292:12 | file | +| Test.java:296:21:296:66 | new File(...) : File | Test.java:298:65:298:68 | temp : File | +| Test.java:296:30:296:65 | getProperty(...) : String | Test.java:296:21:296:66 | new File(...) : File | +| Test.java:298:24:298:69 | createTempFile(...) : File | Test.java:299:9:299:15 | workDir : File | +| Test.java:298:65:298:68 | temp : File | Test.java:298:24:298:69 | createTempFile(...) : File | +| Test.java:299:9:299:15 | workDir : File | Test.java:299:9:299:15 | workDir : File | +| Test.java:299:9:299:15 | workDir : File | Test.java:300:24:300:30 | workDir | +| Test.java:299:9:299:15 | workDir : File | Test.java:300:24:300:30 | workDir : File | +| Test.java:300:24:300:30 | workDir : File | Test.java:304:32:304:40 | file : File | +| Test.java:304:32:304:40 | file : File | Test.java:306:14:306:17 | file | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:12:13:12:16 | temp : File | semmle.label | temp : File | @@ -163,6 +181,26 @@ nodes | Test.java:258:23:258:29 | workDir : File | semmle.label | workDir : File | | Test.java:262:31:262:39 | file : File | semmle.label | file : File | | Test.java:263:9:263:12 | file | semmle.label | file | +| Test.java:282:21:282:66 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:282:30:282:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:284:24:284:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:284:65:284:68 | temp : File | semmle.label | temp : File | +| Test.java:285:9:285:15 | workDir : File | semmle.label | workDir : File | +| Test.java:285:9:285:15 | workDir : File | semmle.label | workDir : File | +| Test.java:286:24:286:30 | workDir | semmle.label | workDir | +| Test.java:286:24:286:30 | workDir : File | semmle.label | workDir : File | +| Test.java:290:32:290:40 | file : File | semmle.label | file : File | +| Test.java:292:9:292:12 | file | semmle.label | file | +| Test.java:296:21:296:66 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:296:30:296:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:298:24:298:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:298:65:298:68 | temp : File | semmle.label | temp : File | +| Test.java:299:9:299:15 | workDir : File | semmle.label | workDir : File | +| Test.java:299:9:299:15 | workDir : File | semmle.label | workDir : File | +| Test.java:300:24:300:30 | workDir | semmle.label | workDir | +| Test.java:300:24:300:30 | workDir : File | semmle.label | workDir : File | +| Test.java:304:32:304:40 | file : File | semmle.label | file : File | +| Test.java:306:14:306:17 | file | semmle.label | file | subpaths #select | Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here | @@ -177,3 +215,5 @@ subpaths | Test.java:230:9:230:12 | temp | Test.java:228:21:228:66 | createTempFile(...) : File | Test.java:230:9:230:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:229:9:229:12 | temp | delete here | Test.java:231:16:231:19 | temp | here | | Test.java:248:36:248:38 | dir | Test.java:235:30:235:65 | getProperty(...) : String | Test.java:248:36:248:38 | dir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:238:14:238:20 | workDir | delete here | Test.java:242:16:242:22 | workDir | here | | Test.java:263:9:263:12 | file | Test.java:254:30:254:65 | getProperty(...) : String | Test.java:263:9:263:12 | file | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:257:9:257:15 | workDir | delete here | Test.java:259:16:259:22 | workDir | here | +| Test.java:292:9:292:12 | file | Test.java:282:30:282:65 | getProperty(...) : String | Test.java:292:9:292:12 | file | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:285:9:285:15 | workDir | delete here | Test.java:287:16:287:22 | workDir | here | +| Test.java:306:14:306:17 | file | Test.java:296:30:296:65 | getProperty(...) : String | Test.java:306:14:306:17 | file | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:299:9:299:15 | workDir | delete here | Test.java:301:16:301:22 | workDir | here | diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index ceb7797380b4..f96a44b2dcbb 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -277,4 +277,34 @@ static void mkdirsWrapperSafe(File file) throws IOException { throw new IOException("Mkdirs failed to create " + file.toString()); } } + + File vulnerable8() throws IOException { + File temp = new File(System.getProperty("java.io.tmpdir")); + mkdirsWrapper2(temp); + File workDir = File.createTempFile("test", "directory", temp); + workDir.delete(); + mkdirsWrapper2(workDir); + return workDir; + } + + static void mkdirsWrapper2(File file) { + if (file.exists()) return; + file.mkdirs(); + } + + File vulnerable9() throws IOException { + File temp = new File(System.getProperty("java.io.tmpdir")); + mkdirsWrapper3(temp); + File workDir = File.createTempFile("test", "directory", temp); + workDir.delete(); + mkdirsWrapper3(workDir); + return workDir; + } + + static void mkdirsWrapper3(File file) throws IOException { + if (file.exists()) return; // Vulnerable path + if (!file.mkdirs()) { + throw new IOException("Mkdirs failed to create " + file.toString()); + } + } } From 21bef99f8ed50a574cfcfb985beaa1de062cdcf8 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 30 Mar 2022 11:23:01 -0400 Subject: [PATCH 22/28] Add release notes --- .../CWE/CWE-378/TempDirHijackingVulnerability.ql | 10 ++++++++-- .../2022-03-30-local-temporary-directory-hijacking.md | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-) create mode 100644 java/ql/src/change-notes/2022-03-30-local-temporary-directory-hijacking.md diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 49b147dcb820..c08cf964bcd1 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -1,10 +1,16 @@ /** - * @name Temporary Directory Hijacking Vulnerability disclosure - * @description Detect temporary directory hijack vulnerability + * @name Temporary Directory Hijacking Vulnerability + * @description Detect temporary directory hijacking vulnerability due too file creation race condition. * @kind path-problem * @problem.severity error * @precision very-high * @id java/temp-directory-hijacking + * @security-severity 7.8 + * @tags security + * external/cwe/cwe-378 + * external/cwe/cwe-379 + * external/cwe/cwe-552 + * external/cwe/cwe-732 */ import java diff --git a/java/ql/src/change-notes/2022-03-30-local-temporary-directory-hijacking.md b/java/ql/src/change-notes/2022-03-30-local-temporary-directory-hijacking.md new file mode 100644 index 000000000000..db784534e31a --- /dev/null +++ b/java/ql/src/change-notes/2022-03-30-local-temporary-directory-hijacking.md @@ -0,0 +1,5 @@ +--- +category: newQuery +--- + * A new query titled "Temporary Directory Hijacking Vulnerability" (`java/ava/temp-directory-hijacking`) has been added. + This query finds chains of calls that can cause temporary directory hijacking. \ No newline at end of file From 407dd05a97bdf027baefc63e24f6e2cc9b5cad11 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 30 Mar 2022 13:28:20 -0400 Subject: [PATCH 23/28] Rename localExprFlowPlusInitializers to localExprOrInitializerFlow --- .../semmle/code/java/dataflow/internal/DataFlowUtil.qll | 6 +++--- .../lib/semmle/code/java/environment/SystemProperty.qll | 9 +++++---- .../CWE/CWE-200/TempDirLocalInformationDisclosure.ql | 2 +- .../CWE/CWE-378/TempDirHijackingVulnerability.ql | 2 +- 4 files changed, 10 insertions(+), 9 deletions(-) diff --git a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll index 38f535ff4abc..cbb09e7365c1 100644 --- a/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll +++ b/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll @@ -90,8 +90,8 @@ predicate localExprFlow(Expr e1, Expr e2) { localFlow(exprNode(e1), exprNode(e2) * for final variables. */ pragma[inline] -predicate localExprFlowPlusInitializers(Expr e1, Expr e2) { - localFlowPlusInitializers(exprNode(e1), exprNode(e2)) +predicate localExprOrInitializerFlow(Expr e1, Expr e2) { + localOrInitializerFlow(exprNode(e1), exprNode(e2)) } /** @@ -100,7 +100,7 @@ predicate localExprFlowPlusInitializers(Expr e1, Expr e2) { * for final variables. */ pragma[inline] -predicate localFlowPlusInitializers(Node pred, Node succ) { +predicate localOrInitializerFlow(Node pred, Node succ) { exists(Variable v | v.isFinal() and pred.asExpr() = v.getInitializer() | localFlow(exprNode(v.getAnAccess()), succ) ) diff --git a/java/ql/lib/semmle/code/java/environment/SystemProperty.qll b/java/ql/lib/semmle/code/java/environment/SystemProperty.qll index 36927070af51..2634505b5227 100644 --- a/java/ql/lib/semmle/code/java/environment/SystemProperty.qll +++ b/java/ql/lib/semmle/code/java/environment/SystemProperty.qll @@ -42,7 +42,7 @@ private MethodAccess getSystemPropertyFromSystemGetProperties(string propertyNam result.getMethod() = getMethod ) and result.getArgument(0).(CompileTimeConstantExpr).getStringValue() = propertyName and - DataFlow::localExprFlowPlusInitializers(any(MethodAccess m | + DataFlow::localExprOrInitializerFlow(any(MethodAccess m | m.getMethod().getDeclaringType() instanceof TypeSystem and m.getMethod().hasName("getProperties") ), result.getQualifier()) @@ -172,15 +172,16 @@ private MethodAccess getSystemPropertyFromGuava(string propertyName) { ec.getDeclaringType().hasQualifiedName("com.google.common.base", "StandardSystemProperty") and // Example: `StandardSystemProperty.JAVA_IO_TMPDIR.value()` ( - DataFlow::localExprFlowPlusInitializers(ec.getAnAccess(), result.getQualifier()) and + DataFlow::localExprOrInitializerFlow(ec.getAnAccess(), result.getQualifier()) and result.getMethod().hasName("value") ) or // Example: `System.getProperty(StandardSystemProperty.JAVA_IO_TMPDIR.key())` exists(MethodAccess keyMa | - DataFlow::localExprFlowPlusInitializers(ec.getAnAccess(), keyMa.getQualifier()) and + DataFlow::localExprOrInitializerFlow(ec.getAnAccess(), keyMa.getQualifier()) and keyMa.getMethod().hasName("key") and - DataFlow::localExprFlowPlusInitializers(keyMa, result.(MethodAccessSystemGetProperty).getArgument(0)) + DataFlow::localExprOrInitializerFlow(keyMa, + result.(MethodAccessSystemGetProperty).getArgument(0)) ) | ec.hasName("JAVA_VERSION") and propertyName = "java.version" diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index c282922a0d6a..10060d0b1091 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -205,7 +205,7 @@ class MethodAccessInsecureFileCreateTempFile extends MethodAccessInsecureFileCre this.getNumArgument() = 2 or // The default temporary directory is used when the last argument of `File.createTempFile(string, string, File)` is `null` - DataFlow::localExprFlowPlusInitializers(any(NullLiteral n), this.getArgument(2)) + DataFlow::localExprOrInitializerFlow(any(NullLiteral n), this.getArgument(2)) ) } diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index c08cf964bcd1..86a16c800f13 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -160,7 +160,7 @@ private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTain ma.getNumArgument() = 2 or // The three argument variant of this method uses the system property `java.io.tmpdir` as the base directory when `null`. - DataFlow::localExprFlowPlusInitializers(any(NullLiteral n), ma.getArgument(2)) + DataFlow::localExprOrInitializerFlow(any(NullLiteral n), ma.getArgument(2)) ) ) } From 0f5a1e7aa429348f584f46f2982d762b4b243f38 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Thu, 31 Mar 2022 11:38:57 -0400 Subject: [PATCH 24/28] Expand isDeleteFileExpr to include delete method wrappers --- .../CWE-378/TempDirHijackingVulnerability.ql | 32 +++++++++++++---- .../TempDirHijackingVulnerability.expected | 36 ++++++++----------- .../security/CWE-378/semmle/tests/Test.java | 29 ++++++++++----- 3 files changed, 61 insertions(+), 36 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 86a16c800f13..744c10b930d9 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -101,12 +101,14 @@ private predicate isNonThrowingDirectoryCreationExpression( ) { creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = fileInstanceExpr and - // Check for 'throwing creations' and ensure that this call is not used in a throwing case. - if creationCall.(Guard).directlyControls(_, _) - then - // The creation call is used in a guard + ( + // Check for 'throwing creations' and ensure that this call is not used in a throwing case. + // If the creation call is used in a guard: + creationCall.(Guard).directlyControls(_, _) + implies + // Then it must not be a 'safe' creation guard. Thus `creationCall` is not a 'throwing creation'. not creationCall = safeDirectoryCreationGuard(_) // Ensure that the guard is insufficient. - else any() // The creation call is not used in a guard. Thus is not a 'throwing creation'. + ) or // Recursively look for methods that encapsulate the above. // Thus, the use of 'helper directory creation methods' are still considered @@ -142,7 +144,23 @@ private class MethodFileExists extends Method { } predicate isDeleteFileExpr(Expr expr) { - expr = any(MethodFileDelete m).getAReference().getQualifier() + exists(Expr deleteMethodQualifier | + deleteMethodQualifier = any(MethodFileDelete m).getAReference().getQualifier() + | + // The expression is the qualifier of the `delete` method call. + expr = deleteMethodQualifier + or + // Any wrapper method that calls the `delete` method on a file instance argument. + expr = + any(Method m, int argIndex, Expr arg | + arg.(VarAccess).getVariable() = m.getParameter(argIndex) and + // We intentionally don't call this method recursively as it will increase the false positive rate. + // A delete call at a depth of one call is enough to cover most cases. + DataFlow::localExprFlow(arg, deleteMethodQualifier) + | + m.getAReference().getArgument(argIndex) + ) + ) } abstract private class DirHijackingTaintSource extends DataFlow::Node { @@ -402,7 +420,7 @@ class TempDirHijackingFullPathInlcudingUnsafeUse extends TaintTracking2::Configu or // `File::mkdir(s)` is not an end-state when looking for an unsafe use isNonThrowingDirectoryCreationExpression(node1.asExpr(), _) and - node1.asExpr() = node2.asExpr() and + node1 = node2 and state1 instanceof FromDeleteFileFlowState and state2 instanceof FromMkdirsFileFlowState } diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index 05072f2763c6..d3506f041157 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -34,15 +34,12 @@ edges | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File | | Test.java:148:9:148:12 | temp : File | Test.java:148:9:148:12 | temp : File | | Test.java:148:9:148:12 | temp : File | Test.java:149:9:149:12 | temp | -| Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:21:158:24 | temp : File | -| Test.java:158:21:158:24 | temp : File | Test.java:158:21:158:24 | temp : File | -| Test.java:158:21:158:24 | temp : File | Test.java:158:38:158:41 | temp | -| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:21:171:30 | this <.field> [safe12temp] : File | -| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | -| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:21:171:30 | safe12temp : File | -| Test.java:171:21:171:30 | safe12temp : File | Test.java:171:21:171:30 | safe12temp : File | -| Test.java:171:21:171:30 | safe12temp : File | Test.java:171:44:171:53 | safe12temp | -| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | Test.java:171:21:171:30 | safe12temp : File | +| Test.java:156:20:156:59 | createTempFile(...) : File | Test.java:157:17:157:20 | temp : File | +| Test.java:157:17:157:20 | temp : File | Test.java:157:17:157:20 | temp : File | +| Test.java:157:17:157:20 | temp : File | Test.java:157:34:157:37 | temp | +| Test.java:169:24:169:63 | createTempFile(...) : File | Test.java:170:21:170:24 | safe : File | +| Test.java:170:21:170:24 | safe : File | Test.java:170:21:170:24 | safe : File | +| Test.java:170:21:170:24 | safe : File | Test.java:170:38:170:41 | safe | | Test.java:211:21:211:66 | new File(...) : File | Test.java:213:65:213:68 | temp : File | | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:211:21:211:66 | new File(...) : File | | Test.java:213:24:213:69 | createTempFile(...) : File | Test.java:214:14:214:20 | workDir : File | @@ -137,16 +134,14 @@ nodes | Test.java:148:9:148:12 | temp : File | semmle.label | temp : File | | Test.java:148:9:148:12 | temp : File | semmle.label | temp : File | | Test.java:149:9:149:12 | temp | semmle.label | temp | -| Test.java:157:24:157:63 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:158:21:158:24 | temp : File | semmle.label | temp : File | -| Test.java:158:21:158:24 | temp : File | semmle.label | temp : File | -| Test.java:158:38:158:41 | temp | semmle.label | temp | -| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | semmle.label | this <.field> [post update] [safe12temp] : File | -| Test.java:170:30:170:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | -| Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File | -| Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File | -| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File | -| Test.java:171:44:171:53 | safe12temp | semmle.label | safe12temp | +| Test.java:156:20:156:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:157:17:157:20 | temp : File | semmle.label | temp : File | +| Test.java:157:17:157:20 | temp : File | semmle.label | temp : File | +| Test.java:157:34:157:37 | temp | semmle.label | temp | +| Test.java:169:24:169:63 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:170:21:170:24 | safe : File | semmle.label | safe : File | +| Test.java:170:21:170:24 | safe : File | semmle.label | safe : File | +| Test.java:170:38:170:41 | safe | semmle.label | safe | | Test.java:211:21:211:66 | new File(...) : File | semmle.label | new File(...) : File | | Test.java:211:30:211:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | | Test.java:213:24:213:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | @@ -208,8 +203,7 @@ subpaths | Test.java:24:9:24:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:23:9:23:12 | temp | delete here | Test.java:25:16:25:19 | temp | here | | Test.java:131:9:131:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:131:9:131:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:130:9:130:12 | temp | delete here | Test.java:132:16:132:19 | temp | here | | Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here | -| Test.java:158:38:158:41 | temp | Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:158:21:158:24 | temp | delete here | Test.java:163:16:163:19 | temp | here | -| Test.java:171:44:171:53 | safe12temp | Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:171:21:171:30 | safe12temp | delete here | Test.java:176:16:176:25 | safe12temp | here | +| Test.java:170:38:170:41 | safe | Test.java:169:24:169:63 | createTempFile(...) : File | Test.java:170:38:170:41 | safe | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:170:21:170:24 | safe | delete here | Test.java:176:16:176:25 | safe12temp | here | | Test.java:222:14:222:16 | dir | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:222:14:222:16 | dir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:214:14:214:20 | workDir | delete here | Test.java:218:16:218:22 | workDir | here | | Test.java:222:14:222:16 | dir | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:222:14:222:16 | dir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:214:14:214:20 | workDir | delete here | Test.java:222:31:222:33 | dir | here | | Test.java:230:9:230:12 | temp | Test.java:228:21:228:66 | createTempFile(...) : File | Test.java:230:9:230:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:229:9:229:12 | temp | delete here | Test.java:231:16:231:19 | temp | here | diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java index f96a44b2dcbb..d09bcf4b8c8f 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -152,12 +152,10 @@ static File vulnerable3() throws IOException { static File safe11() throws IOException { File temp = null; - if (temp == null) { - while (true) { - temp = File.createTempFile("test", "directory"); - if (temp.delete() && temp.mkdir()) { - break; - } + while (true) { + temp = File.createTempFile("test", "directory"); + if (temp.delete() && temp.mkdir()) { + break; } } return temp; @@ -166,12 +164,14 @@ static File safe11() throws IOException { File safe12temp; File safe12() throws IOException { if (safe12temp == null) { + File safe = null; while (true) { - safe12temp = File.createTempFile("test", "directory"); - if (safe12temp.delete() && safe12temp.mkdir()) { + safe = File.createTempFile("test", "directory"); + if (safe.delete() && safe.mkdir()) { break; } } + safe12temp = safe; } return safe12temp; } @@ -307,4 +307,17 @@ static void mkdirsWrapper3(File file) throws IOException { throw new IOException("Mkdirs failed to create " + file.toString()); } } + + File vulnerable10() throws IOException { + File temp = new File(System.getProperty("java.io.tmpdir")); + temp.mkdirs(); + File workDir = File.createTempFile("test", "directory", temp); + deleteWrapper(workDir); + workDir.mkdirs(); + return workDir; + } + + static void deleteWrapper(File file) { + file.delete(); + } } From e7f016e299364ed8de1face6bf2a05cf112ec5cc Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Mon, 25 Apr 2022 11:46:31 -0400 Subject: [PATCH 25/28] Apply suggestions from code review Co-authored-by: Chris Smowton --- java/ql/lib/semmle/code/java/security/TempFileLib.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/TempFileLib.qll b/java/ql/lib/semmle/code/java/security/TempFileLib.qll index 229ec1846801..d44ca261277b 100644 --- a/java/ql/lib/semmle/code/java/security/TempFileLib.qll +++ b/java/ql/lib/semmle/code/java/security/TempFileLib.qll @@ -4,7 +4,7 @@ import java import semmle.code.java.dataflow.ExternalFlow /** - * All `java.io.File::createTempFile` methods. + * A `java.io.File::createTempFile` method. */ class MethodFileCreateTempFile extends Method { MethodFileCreateTempFile() { @@ -14,7 +14,7 @@ class MethodFileCreateTempFile extends Method { } /** - * All methods on the class `java.io.File` that create directories. + * A method on the class `java.io.File` that create directories. */ class MethodFileCreatesDirs extends Method { MethodFileCreatesDirs() { From 3a5025309295b3d25650cff1980bdc3463120202 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Thu, 28 Apr 2022 13:23:45 -0400 Subject: [PATCH 26/28] Fix implicit 'this' use in TempFileLib --- java/ql/lib/semmle/code/java/security/TempFileLib.qll | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/TempFileLib.qll b/java/ql/lib/semmle/code/java/security/TempFileLib.qll index d44ca261277b..35b4768b8fb6 100644 --- a/java/ql/lib/semmle/code/java/security/TempFileLib.qll +++ b/java/ql/lib/semmle/code/java/security/TempFileLib.qll @@ -18,8 +18,8 @@ class MethodFileCreateTempFile extends Method { */ class MethodFileCreatesDirs extends Method { MethodFileCreatesDirs() { - getDeclaringType() instanceof TypeFile and - hasName(["mkdir", "mkdirs"]) + this.getDeclaringType() instanceof TypeFile and + this.hasName(["mkdir", "mkdirs"]) } } From cd3662c4350d3458ffc2af69bb2d1b9c56a28b6e Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Tue, 3 May 2022 11:51:07 -0400 Subject: [PATCH 27/28] Cleanup after rebase on `main` --- .../lib/semmle/code/java/security/TempFileLib.qll | 10 ---------- .../tests/TempDirHijackingVulnerability.expected | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/java/ql/lib/semmle/code/java/security/TempFileLib.qll b/java/ql/lib/semmle/code/java/security/TempFileLib.qll index 35b4768b8fb6..761d2a8a495a 100644 --- a/java/ql/lib/semmle/code/java/security/TempFileLib.qll +++ b/java/ql/lib/semmle/code/java/security/TempFileLib.qll @@ -1,7 +1,6 @@ /** Provides classes to reason about temporary directory vulnerabilities. */ import java -import semmle.code.java.dataflow.ExternalFlow /** * A `java.io.File::createTempFile` method. @@ -22,12 +21,3 @@ class MethodFileCreatesDirs extends Method { this.hasName(["mkdir", "mkdirs"]) } } - -private class TemporaryFileFlow extends SummaryModelCsv { - override predicate row(string row) { - // qualifier to return - row = - "java.io;File;true;" + ["getAbsoluteFile", "getCanonicalFile"] + - ";;;Argument[-1];ReturnValue;taint" - } -} diff --git a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected index d3506f041157..94c653032fba 100644 --- a/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -88,6 +88,12 @@ edges | Test.java:299:9:299:15 | workDir : File | Test.java:300:24:300:30 | workDir : File | | Test.java:300:24:300:30 | workDir : File | Test.java:304:32:304:40 | file : File | | Test.java:304:32:304:40 | file : File | Test.java:306:14:306:17 | file | +| Test.java:312:21:312:66 | new File(...) : File | Test.java:314:65:314:68 | temp : File | +| Test.java:312:30:312:65 | getProperty(...) : String | Test.java:312:21:312:66 | new File(...) : File | +| Test.java:314:24:314:69 | createTempFile(...) : File | Test.java:315:23:315:29 | workDir : File | +| Test.java:314:65:314:68 | temp : File | Test.java:314:24:314:69 | createTempFile(...) : File | +| Test.java:315:23:315:29 | workDir : File | Test.java:315:23:315:29 | workDir : File | +| Test.java:315:23:315:29 | workDir : File | Test.java:316:9:316:15 | workDir | nodes | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | | Test.java:12:13:12:16 | temp : File | semmle.label | temp : File | @@ -196,6 +202,13 @@ nodes | Test.java:300:24:300:30 | workDir : File | semmle.label | workDir : File | | Test.java:304:32:304:40 | file : File | semmle.label | file : File | | Test.java:306:14:306:17 | file | semmle.label | file | +| Test.java:312:21:312:66 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:312:30:312:65 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:314:24:314:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File | +| Test.java:314:65:314:68 | temp : File | semmle.label | temp : File | +| Test.java:315:23:315:29 | workDir : File | semmle.label | workDir : File | +| Test.java:315:23:315:29 | workDir : File | semmle.label | workDir : File | +| Test.java:316:9:316:15 | workDir | semmle.label | workDir | subpaths #select | Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here | @@ -211,3 +224,4 @@ subpaths | Test.java:263:9:263:12 | file | Test.java:254:30:254:65 | getProperty(...) : String | Test.java:263:9:263:12 | file | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:257:9:257:15 | workDir | delete here | Test.java:259:16:259:22 | workDir | here | | Test.java:292:9:292:12 | file | Test.java:282:30:282:65 | getProperty(...) : String | Test.java:292:9:292:12 | file | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:285:9:285:15 | workDir | delete here | Test.java:287:16:287:22 | workDir | here | | Test.java:306:14:306:17 | file | Test.java:296:30:296:65 | getProperty(...) : String | Test.java:306:14:306:17 | file | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:299:9:299:15 | workDir | delete here | Test.java:301:16:301:22 | workDir | here | +| Test.java:316:9:316:15 | workDir | Test.java:312:30:312:65 | getProperty(...) : String | Test.java:316:9:316:15 | workDir | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:315:23:315:29 | workDir | delete here | Test.java:317:16:317:22 | workDir | here | From a2a7c735bbdd4dcaa793768e87fda93782ea0dcc Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Mon, 9 May 2022 20:30:07 +0100 Subject: [PATCH 28/28] Clean up function naming, documentation, and to some degree code without changing behaviour --- .../CWE-378/TempDirHijackingVulnerability.ql | 266 ++++++++++-------- 1 file changed, 156 insertions(+), 110 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql index 744c10b930d9..c2eaaae55f87 100644 --- a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -59,42 +59,55 @@ private class FromMkdirsFileFlowState extends DataFlow::FlowState { } /** - * A guard that is checking if a directory exists, throwing if it does not exist. + * Holds if `s` is executed if a `File.exists` or `File.isDirectory` check fails. */ -private Guard directoryExistsGuard(ThrowStmt s) { - result = - any(MethodAccess existanceCheck | - ( - existanceCheck.getMethod() instanceof MethodFileExists - or - existanceCheck.getMethod() instanceof MethodFileIsDirectory - ) - ) and - result.directlyControls(s.getEnclosingStmt(), false) +private predicate throwsIfDirectoryDoesNotExist(ThrowStmt s) { + exists(Guard g | + g = + any(MethodAccess existenceCheck | + ( + existenceCheck.getMethod() instanceof MethodFileExists + or + existenceCheck.getMethod() instanceof MethodFileIsDirectory + ) + ) and + g.directlyControls(s.getEnclosingStmt(), false) + ) } /** - * A guard that is verifying that the directory is sucsessfully created, throwing when it is not created. + * Holds if `test` checks if a `File.mkdir` or `mkdirs` operation failed, in which case throw statement `s` is executed. */ -private Guard directoryCreationGuard(ThrowStmt s) { - result = +private predicate throwsIfMkdirFailed(Guard test, ThrowStmt s) { + test = any(MethodAccess creationCheck | creationCheck.getMethod() instanceof MethodFileCreatesDirs) and - result.directlyControls(s.getEnclosingStmt(), false) + test.directlyControls(s.getEnclosingStmt(), false) } /** - * A guard that is safely verfying that a directory is created. + * Holds if `test` checks if a `File.mkdir` or `mkdirs` operation failed, in which case an exception is thrown, + * and that same throw isn't also reachable from a failing `exists` or `isDirectory` check. * - * 'Safe' means that the directory is guarnteed to have been created, and the directory did not already exist. + * For example, gets the test expression in `if(!f.mkdir()) { throw ... }` but not `if(!f.mkdir() && !f.exists()) { throw ... }`, + * since the latter accepts the case where `f` already exists. */ -private Guard safeDirectoryCreationGuard(ThrowStmt s) { - result = directoryCreationGuard(s) and - // This guard is not 'safe' if a `directoryExistsGuard` is also guarding this throw statement - not exists(directoryExistsGuard(s)) +private predicate throwsIfMkdirFailedExcludingExistenceChecks(Guard test) { + exists(ThrowStmt s | + throwsIfMkdirFailed(test, s) and + // The same throw statement must not result from a failing `f.exists()` or similar call. + not throwsIfDirectoryDoesNotExist(s) + ) } /** - * An expression that will create a directory without throwing an exception if a file/directory already exists. + * Holds if `creationCall` is of the form `fileInstanceExpr.mkdir()`, + * and it is not clear that failure to create a fresh directory will result in throwing an exception. + * + * Note `fileInstanceExpr` may be passed to the actual `mkdir` via a wrapper function: in this case + * `fileInstanceExpr` is the argument to the outer wrapper function, but `creationCall` is still the + * inner `mkdir` call. + * + * TODO: that may warrant changing. */ private predicate isNonThrowingDirectoryCreationExpression( Expr fileInstanceExpr, MethodAccess creationCall @@ -102,23 +115,22 @@ private predicate isNonThrowingDirectoryCreationExpression( creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = fileInstanceExpr and ( - // Check for 'throwing creations' and ensure that this call is not used in a throwing case. - // If the creation call is used in a guard: - creationCall.(Guard).directlyControls(_, _) - implies - // Then it must not be a 'safe' creation guard. Thus `creationCall` is not a 'throwing creation'. - not creationCall = safeDirectoryCreationGuard(_) // Ensure that the guard is insufficient. + // The result of `mkdir` is not directly tested at all... + not creationCall.(Guard).directlyControls(_, _) + or + // ... or it is tested, but that test doesn't clearly have the form `if(!f.mkdir()) throw ...`. + not throwsIfMkdirFailedExcludingExistenceChecks(creationCall) ) or // Recursively look for methods that encapsulate the above. // Thus, the use of 'helper directory creation methods' are still considered // when assessing if an `unsafeUse` is present or not. - exists(Method m, int argumentIndex, Expr arg | - arg.(VarAccess).getVariable() = m.getParameter(argumentIndex) and - isNonThrowingDirectoryCreationExpression(any(Expr e2 | DataFlow::localExprFlow(arg, e2)), - creationCall) + exists(Method m, int argumentIndex, Expr fileInstanceParam | + DataFlow::localExprFlow(m.getParameter(pragma[only_bind_into](argumentIndex)).getAnAccess(), + fileInstanceParam) and + isNonThrowingDirectoryCreationExpression(fileInstanceParam, creationCall) | - m.getAReference().getArgument(argumentIndex) = fileInstanceExpr + m.getAReference().getArgument(pragma[only_bind_into](argumentIndex)) = fileInstanceExpr ) } @@ -143,32 +155,34 @@ private class MethodFileExists extends Method { } } -predicate isDeleteFileExpr(Expr expr) { +/** + * Holds if `file` is deleted, by a call to `file.delete()` or a method that wraps the same. + */ +predicate isDeletedFile(Expr file) { exists(Expr deleteMethodQualifier | deleteMethodQualifier = any(MethodFileDelete m).getAReference().getQualifier() | // The expression is the qualifier of the `delete` method call. - expr = deleteMethodQualifier + file = deleteMethodQualifier or // Any wrapper method that calls the `delete` method on a file instance argument. - expr = - any(Method m, int argIndex, Expr arg | - arg.(VarAccess).getVariable() = m.getParameter(argIndex) and + file = + any(Method m, int argIndex | // We intentionally don't call this method recursively as it will increase the false positive rate. // A delete call at a depth of one call is enough to cover most cases. - DataFlow::localExprFlow(arg, deleteMethodQualifier) + DataFlow::localExprFlow(m.getParameter(argIndex).getAnAccess(), deleteMethodQualifier) | m.getAReference().getArgument(argIndex) ) ) } -abstract private class DirHijackingTaintSource extends DataFlow::Node { +abstract private class SystemTempDirNode extends DataFlow::Node { abstract DataFlow::FlowState getFlowState(); } -private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTaintSource { - FileCreateTempFileDirHijackingTaintSource() { +private class TempFileInSystemTempDirNode extends SystemTempDirNode { + TempFileInSystemTempDirNode() { this.asExpr() = any(MethodAccess ma | // The two argument variant of this method uses the system property `java.io.tmpdir` as the base directory. @@ -186,46 +200,64 @@ private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTain override DataFlow::FlowState getFlowState() { result instanceof FromFileCreateTempFileFlowState } } -private class SystemPropertyDirHijackingTaintSource extends DirHijackingTaintSource { - SystemPropertyDirHijackingTaintSource() { this.asExpr() = getSystemProperty("java.io.tmpdir") } +private class SystemTempDirPropertyNode extends SystemTempDirNode { + SystemTempDirPropertyNode() { this.asExpr() = getSystemProperty("java.io.tmpdir") } override DataFlow::FlowState getFlowState() { result instanceof FromSystemPropertyFlowState } } /** - * Holds when for a call to `File.createTempFile(_, _, node2)` where `node1` is the `MethodAccess`. + * Holds when `createdTempFile` = `File.createTempFile(_, _, createInDir)`. */ -private predicate isTaintStepFileCreateTempFile(DataFlow::Node node1, DataFlow::Node node2) { - node2.asExpr() = +private predicate isTaintStepFileCreateTempFile( + DataFlow::Node createInDir, DataFlow::Node createdTempFile +) { + createdTempFile.asExpr() = any(MethodAccess ma | - // Argument two is the target directory. - // This vulnerability exists when the system property `java.io.tmpdir` is used as the target directory. - ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr() + ma.getMethod() instanceof MethodFileCreateTempFile and + ma.getArgument(2) = createInDir.asExpr() ) } +/** + * Tracks taint from references to the system global temporary directory (`java.io.tmpdir`), + * either directly through `System.getProperty()` or indirectly using `File.createTempFile`, to + * a `file.delete()` call. + */ private class TempDirHijackingToDeleteConfig extends TaintTracking2::Configuration { TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" } - override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource } + override predicate isSource(DataFlow::Node source) { source instanceof SystemTempDirNode } override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) { + // Propagates taint from the directory a temporary file is created in, to the created file. isTaintStepFileCreateTempFile(node1, node2) } - override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) } + override predicate isSink(DataFlow::Node sink) { isDeletedFile(sink.asExpr()) } } +/** + * Tracks taint from deleted files to an attempt to create the same file, where that creation attempt + * does not appear to throw an exception on failure. + * + * For example, we would flag the path from one use of `f` to the next in `f.delete(); f.mkdir();`, + * but not in `f.delete(); if(!f.mkdir()) throw ...` because the latter case checks that the `mkdir` + * worked as expected and throws otherwise. + */ private class TempDirHijackingFromDeleteConfig extends DataFlow3::Configuration { TempDirHijackingFromDeleteConfig() { this = "TempDirHijackingFromDeleteConfig" } - override predicate isSource(DataFlow::Node source) { isDeleteFileExpr(source.asExpr()) } + override predicate isSource(DataFlow::Node source) { isDeletedFile(source.asExpr()) } override predicate isSink(DataFlow::Node sink) { isNonThrowingDirectoryCreationExpression(sink.asExpr(), _) } } +/** + * Tracks taint from a file that is created without throwing on failure to an unsafe use of that file. + */ private class TempDirHijackingFromDirectoryCreateToUnsafeUseConfig extends DataFlow4::Configuration { TempDirHijackingFromDirectoryCreateToUnsafeUseConfig() { this = "TempDirHijackingFromDirectoryCreateToUnsafeUseConfig" @@ -236,45 +268,34 @@ private class TempDirHijackingFromDirectoryCreateToUnsafeUseConfig extends DataF } override predicate isSink(DataFlow::Node sink) { not safeUse(sink.asExpr()) } - - override predicate isBarrierGuard(DataFlow::BarrierGuard guard) { - guard instanceof DirectoryCreationBarrierGuard - } -} - -private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard { - Expr fileInstanceExpr; - - DirectoryCreationBarrierGuard() { - isNonThrowingDirectoryCreationExpression(fileInstanceExpr, this) - } - - Expr getFileInstanceExpr() { result = fileInstanceExpr } - - override predicate checks(Expr e, boolean branch) { this.controls(e, branch) } } /** - * Holds when there is an expression `unsafeUse` which is an unsafe use of the file that + * Holds when there is an expression `unsafeUse` which is an unsafe use of the file `createdFileNode` that * is not guarded by a check of the return value of `File::mkdir(s)`. + * + * TODO: this is confusing, due to the fact that `isNonThrowingDirectoryCreationExpression` may identify a `mkdir` + * method call inside a wrapper, in whcih case `fileInstanceExpr` is the argument to the wrapper but `createdFileNode` + * is the direct qualifier to `mkdir`. Hence the global flow from the direct `mkdir` qualifier and the local flow from + * a wrapper function's argument are not redundant as they may seem, but this was probably an accident. */ -predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) { - exists(DirectoryCreationBarrierGuard g, MethodAccess ma | - any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to `mkdir` or `mkdirs` - sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink - g = ma and // The guard is the method access +predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node createdFileNode, Expr unsafeUse) { + exists(Expr fileInstanceExpr, MethodAccess creationCall | + // Note `fileInstanceExpr` may be an argument to method wrapping `mkdir`, and therefore not the same as `createdFileNode.asExpr()` + isNonThrowingDirectoryCreationExpression(fileInstanceExpr, creationCall) and + createdFileNode.asExpr() = creationCall.getQualifier() and // `createdFileNode` is the direct qualifier to `creationCall` ( - // TODO: Consider replacing this entire bit of logic with `TempDirHijackingFullPathInlcudingUnsafeUse`. + // TODO: Consider replacing this entire bit of logic with `TempDirHijackingFullPathIncludingUnsafeUse`. any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c) - .hasFlow(sink, DataFlow::exprNode(unsafeUse)) // There is some flow from the sink to an unsafe use of the File + .hasFlow(createdFileNode, DataFlow::exprNode(unsafeUse)) // There is some flow from the created file to an unsafe use of the File or - DataFlow::localExprFlow(g.getFileInstanceExpr(), unsafeUse) // There is some local flow from the passed file instance to an unsafe use + DataFlow::localExprFlow(fileInstanceExpr, unsafeUse) // There is some local flow from the passed file instance to an unsafe use ) and - unsafeUse != sink.asExpr() and // The unsafe use is not the sink itself - not g.getFileInstanceExpr() = unsafeUse and // The unsafe use is not the file instance + unsafeUse != createdFileNode.asExpr() and // The unsafe use is not the sink itself + not fileInstanceExpr = unsafeUse and // The unsafe use is not the file instance not safeUse(unsafeUse) and // The unsafe use is not a safe use - not g.controls(unsafeUse.getBasicBlock(), true) and - not booleanVarAccessGuardsGuard(g) // The guard is not guarded by a boolean variable access guard + not creationCall.(Guard).controls(unsafeUse.getBasicBlock(), true) and + not booleanVarAccessGuardsGuard(creationCall) // The guard is not guarded by a boolean variable access guard ) } @@ -282,6 +303,11 @@ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) * Holds for any guard `g` that is itself guarded by a boolean variable access guard. * * For example, the following code: `if (isDirectory && !file.mkdir()) { ... }`. + * + * TODO: This is an unreasoned heuristic -- there's no reason to prefer the other check passing vs. failing, + * and generally speaking indiscriminately eliminating anything guarded by any other boolean regardless of context + * would be confusing to a user wondering why `if (itsMonday && !file.mkdir()) { ... }` is being treated differently + * to `if (!file.mkdir()) { ... }` */ private predicate booleanVarAccessGuardsGuard(Guard g) { exists(Guard g2 | g2 = any(VarAccess va | va.getType() instanceof BooleanType) | @@ -290,27 +316,46 @@ private predicate booleanVarAccessGuardsGuard(Guard g) { } /** - * Gets any `MethodAccess` that access string returning methods on the expression `e` of type `File`. + * Gets the result of a call `file.method()`, where `file` is a `java.io.File` and `method` returns `String`. + * + * These are all path elements of `file`. */ -private MethodAccess getStringAccessOnFile(Expr e) { +private MethodAccess getAStringAccessOnFile(Expr file) { result = any(MethodAccess fileMethodAccess | fileMethodAccess.getMethod().getDeclaringType() instanceof TypeFile and - fileMethodAccess.getQualifier() = e and + fileMethodAccess.getQualifier() = file and fileMethodAccess.getMethod().getReturnType() instanceof TypeString ) } -private Argument getThrowableConstructorParam() { - result = - any(Argument a | - exists(ConstructorCall c | - c.getConstructor().getDeclaringType().getAnAncestor() instanceof TypeThrowable and - c.getAnArgument() = a - ) - ) +/** + * Gets an argument to a constructor of a throwable type (e.g. the `e` in `new Exception(e)`). + */ +private Argument getThrowableConstructorArgument() { + exists(ConstructorCall c | + c.getConstructor().getDeclaringType().getAnAncestor() instanceof TypeThrowable and + c.getAnArgument() = result + ) } +/** + * Holds if `e` is considered a safe way to use a potentially hijacked `File` instance: + * + * This can be any of the contexts _: + * + * _ && _ + * x &= _ + * str + _ + * str + _.getPath() and similar + * _.getPath() + " message" // FIXME -- the file itself could be used here, not necessarily `getPath` or similar + * new Exception(_) and similar + * _.deleteOnExit() + * var = _ where all uses of var are safe + * ^^ TODO is this redundant with the local-flow case? + * Any expression where it has at least one local flow sink, and all of those sinks match one of the above cases (redundant with the var case) + * ^^ TODO why is the local-flow case limited to `getPath()` and similar? Why not the file itself? + */ private predicate safeUse(Expr e) { exists(AndLogicalExpr andExp | andExp.getType() instanceof BooleanType and andExp.getAnOperand() = e @@ -324,19 +369,20 @@ private predicate safeUse(Expr e) { exists(AddExpr addExp | addExp.getType() instanceof TypeString and ( + // TODO this is an unreasoned heuristic -- there is no reason why `file + " couldn't be created"` should be treated differently to `"Can't create " + file`. addExp.getRightOperand() = e or // A method call, like `File.getAbsolutePath()` is being called and concatenated to the end of a a string - addExp.getRightOperand() = getStringAccessOnFile(e) + addExp.getRightOperand() = getAStringAccessOnFile(e) or // A method call, like `File.getAbsolutePath()` is being called and prepended to another string with a leading space character. - addExp.getLeftOperand() = getStringAccessOnFile(e) and + addExp.getLeftOperand() = getAStringAccessOnFile(e) and addExp.getRightOperand().(CompileTimeConstantExpr).getStringValue().matches(" %") ) ) or // File is being used to construct an exception message - e = getThrowableConstructorParam() + e = getThrowableConstructorArgument() or // A call to `File::deleteOnExit` exists(MethodAccess ma | @@ -350,17 +396,17 @@ private predicate safeUse(Expr e) { not e = any(VariableAssign assign | not safeUse(assign.getDestVar().getAnAccess())).getSource() or // Data flow exists exclusively to locations that are known to be safe - DataFlow::localExprFlow(getStringAccessOnFile(e), any(Expr sink | safeUse(sink))) and - not DataFlow::localExprFlow(getStringAccessOnFile(e), any(Expr sink | not safeUse(sink))) + DataFlow::localExprFlow(getAStringAccessOnFile(e), any(Expr sink | safeUse(sink))) and + not DataFlow::localExprFlow(getAStringAccessOnFile(e), any(Expr sink | not safeUse(sink))) } -private predicate isSourceUnified(DataFlow::Node source, DataFlow::FlowState state) { - exists(DirHijackingTaintSource taintSource | +private predicate isSystemTempDirSource(DataFlow::Node source, DataFlow::FlowState state) { + exists(SystemTempDirNode taintSource | source = taintSource and state = taintSource.getFlowState() ) } -private predicate isAdditionalTaintStepUnified( +private predicate isAdditionalTaintStepCommon( DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2 ) { // From a temporary directory system property access to `File.createTempFile(_, _, flow)` call @@ -370,7 +416,7 @@ private predicate isAdditionalTaintStepUnified( or // From `File.createTempFile(_, _, flow)` to a call to `File::delete` node1 = node2 and - isDeleteFileExpr(node1.asExpr()) and + isDeletedFile(node1.asExpr()) and state1 instanceof FromFileCreateTempFileFlowState and state2 instanceof FromDeleteFileFlowState } @@ -379,14 +425,14 @@ private class TempDirHijackingFullPath extends TaintTracking::Configuration { TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" } override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { - isSourceUnified(source, state) + isSystemTempDirSource(source, state) } override predicate isAdditionalTaintStep( DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2 ) { - isAdditionalTaintStepUnified(node1, state1, node2, state2) + isAdditionalTaintStepCommon(node1, state1, node2, state2) } override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) { @@ -403,20 +449,20 @@ private class TempDirHijackingFullPath extends TaintTracking::Configuration { * This is intentionally not used in the the generation of the displayed path; * this is because there may not exist an explicit path from the call to `File::mkdir(s)` call to the the `unsafeUse`. */ -class TempDirHijackingFullPathInlcudingUnsafeUse extends TaintTracking2::Configuration { - TempDirHijackingFullPathInlcudingUnsafeUse() { - this = "TempDirHijackingFullPathInlcudingUnsafeUse" +class TempDirHijackingFullPathIncludingUnsafeUse extends TaintTracking2::Configuration { + TempDirHijackingFullPathIncludingUnsafeUse() { + this = "TempDirHijackingFullPathIncludingUnsafeUse" } override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { - isSourceUnified(source, state) + isSystemTempDirSource(source, state) } override predicate isAdditionalTaintStep( DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2 ) { - isAdditionalTaintStepUnified(node1, state1, node2, state2) + isAdditionalTaintStepCommon(node1, state1, node2, state2) or // `File::mkdir(s)` is not an end-state when looking for an unsafe use isNonThrowingDirectoryCreationExpression(node1.asExpr(), _) and @@ -443,7 +489,7 @@ where // Find the delete checkpoint to display to the user any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and // Find a full path where an `unsafe` use is present - any(TempDirHijackingFullPathInlcudingUnsafeUse c) + any(TempDirHijackingFullPathIncludingUnsafeUse c) .hasFlow(pathSource.getNode(), DataFlow::exprNode(unsafe)) and // Verify the unsafe use is not constrained by an if check isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and