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..cbb09e7365c1 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 localExprOrInitializerFlow(Expr e1, Expr e2) { + localOrInitializerFlow(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 localOrInitializerFlow(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..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 - 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()` ( - 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 | - localExprFlowPlusInitializers(ec.getAnAccess(), keyMa.getQualifier()) and + DataFlow::localExprOrInitializerFlow(ec.getAnAccess(), keyMa.getQualifier()) and keyMa.getMethod().hasName("key") and - localExprFlowPlusInitializers(keyMa, result.(MethodAccessSystemGetProperty).getArgument(0)) + DataFlow::localExprOrInitializerFlow(keyMa, + result.(MethodAccessSystemGetProperty).getArgument(0)) ) | ec.hasName("JAVA_VERSION") and propertyName = "java.version" @@ -262,25 +263,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/lib/semmle/code/java/security/TempFileLib.qll b/java/ql/lib/semmle/code/java/security/TempFileLib.qll new file mode 100644 index 000000000000..761d2a8a495a --- /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 + +/** + * A `java.io.File::createTempFile` method. + */ +class MethodFileCreateTempFile extends Method { + MethodFileCreateTempFile() { + this.getDeclaringType() instanceof TypeFile and + this.hasName("createTempFile") + } +} + +/** + * A method on the class `java.io.File` that create directories. + */ +class MethodFileCreatesDirs extends Method { + MethodFileCreatesDirs() { + this.getDeclaringType() instanceof TypeFile and + this.hasName(["mkdir", "mkdirs"]) + } +} diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index 16e8bb72c930..10060d0b1091 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 { @@ -206,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::localExprOrInitializerFlow(any(NullLiteral n), this.getArgument(2)) ) } diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll index cb2d8dd7875e..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. */ @@ -64,7 +55,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/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..ae981d93614f --- /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 guaranteed 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 new file mode 100644 index 000000000000..c2eaaae55f87 --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql @@ -0,0 +1,500 @@ +/** + * @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 +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 +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" } +} + +/** + * 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" } +} + +/** + * Holds if `s` is executed if a `File.exists` or `File.isDirectory` check fails. + */ +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) + ) +} + +/** + * Holds if `test` checks if a `File.mkdir` or `mkdirs` operation failed, in which case throw statement `s` is executed. + */ +private predicate throwsIfMkdirFailed(Guard test, ThrowStmt s) { + test = + any(MethodAccess creationCheck | creationCheck.getMethod() instanceof MethodFileCreatesDirs) and + test.directlyControls(s.getEnclosingStmt(), false) +} + +/** + * 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. + * + * 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 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) + ) +} + +/** + * 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 +) { + creationCall.getMethod() instanceof MethodFileCreatesDirs and + creationCall.getQualifier() = fileInstanceExpr and + ( + // 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 fileInstanceParam | + DataFlow::localExprFlow(m.getParameter(pragma[only_bind_into](argumentIndex)).getAnAccess(), + fileInstanceParam) and + isNonThrowingDirectoryCreationExpression(fileInstanceParam, creationCall) + | + m.getAReference().getArgument(pragma[only_bind_into](argumentIndex)) = fileInstanceExpr + ) +} + +private class MethodFileDelete extends Method { + MethodFileDelete() { + 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") + } +} + +/** + * 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. + file = deleteMethodQualifier + or + // Any wrapper method that calls the `delete` method on a file instance argument. + 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(m.getParameter(argIndex).getAnAccess(), deleteMethodQualifier) + | + m.getAReference().getArgument(argIndex) + ) + ) +} + +abstract private class SystemTempDirNode extends DataFlow::Node { + abstract DataFlow::FlowState getFlowState(); +} + +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. + 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::localExprOrInitializerFlow(any(NullLiteral n), ma.getArgument(2)) + ) + ) + } + + override DataFlow::FlowState getFlowState() { result instanceof FromFileCreateTempFileFlowState } +} + +private class SystemTempDirPropertyNode extends SystemTempDirNode { + SystemTempDirPropertyNode() { this.asExpr() = getSystemProperty("java.io.tmpdir") } + + override DataFlow::FlowState getFlowState() { result instanceof FromSystemPropertyFlowState } +} + +/** + * Holds when `createdTempFile` = `File.createTempFile(_, _, createInDir)`. + */ +private predicate isTaintStepFileCreateTempFile( + DataFlow::Node createInDir, DataFlow::Node createdTempFile +) { + createdTempFile.asExpr() = + any(MethodAccess ma | + 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 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) { 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) { 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" + } + + override predicate isSource(DataFlow::Node source) { + isNonThrowingDirectoryCreationExpression(source.asExpr(), _) + } + + override predicate isSink(DataFlow::Node sink) { not safeUse(sink.asExpr()) } +} + +/** + * 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 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 `TempDirHijackingFullPathIncludingUnsafeUse`. + any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c) + .hasFlow(createdFileNode, DataFlow::exprNode(unsafeUse)) // There is some flow from the created file to an unsafe use of the File + or + DataFlow::localExprFlow(fileInstanceExpr, unsafeUse) // There is some local flow from the passed file instance to an unsafe use + ) and + 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 creationCall.(Guard).controls(unsafeUse.getBasicBlock(), true) and + not booleanVarAccessGuardsGuard(creationCall) // The guard is not guarded by 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()) { ... }`. + * + * 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) | + g2.controls(g.getBasicBlock(), true) + ) +} + +/** + * 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 getAStringAccessOnFile(Expr file) { + result = + any(MethodAccess fileMethodAccess | + fileMethodAccess.getMethod().getDeclaringType() instanceof TypeFile and + fileMethodAccess.getQualifier() = file and + fileMethodAccess.getMethod().getReturnType() instanceof TypeString + ) +} + +/** + * 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 + ) + or + 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 + ( + // 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() = getAStringAccessOnFile(e) + or + // A method call, like `File.getAbsolutePath()` is being called and prepended to another string with a leading space character. + addExp.getLeftOperand() = getAStringAccessOnFile(e) and + addExp.getRightOperand().(CompileTimeConstantExpr).getStringValue().matches(" %") + ) + ) + or + // File is being used to construct an exception message + e = getThrowableConstructorArgument() + 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(getAStringAccessOnFile(e), any(Expr sink | safeUse(sink))) and + not DataFlow::localExprFlow(getAStringAccessOnFile(e), any(Expr sink | not safeUse(sink))) +} + +private predicate isSystemTempDirSource(DataFlow::Node source, DataFlow::FlowState state) { + exists(SystemTempDirNode taintSource | + source = taintSource and state = taintSource.getFlowState() + ) +} + +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 + 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 + isDeletedFile(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) { + isSystemTempDirSource(source, state) + } + + override predicate isAdditionalTaintStep( + DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, + DataFlow::FlowState state2 + ) { + isAdditionalTaintStepCommon(node1, state1, node2, state2) + } + + 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 + } +} + +/** + * 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 TempDirHijackingFullPathIncludingUnsafeUse extends TaintTracking2::Configuration { + TempDirHijackingFullPathIncludingUnsafeUse() { + this = "TempDirHijackingFullPathIncludingUnsafeUse" + } + + override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) { + isSystemTempDirSource(source, state) + } + + override predicate isAdditionalTaintStep( + DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, + DataFlow::FlowState 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 + node1 = node2 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 + // 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(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 + // 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.", + deleteCheckpoint.asExpr(), "delete here", unsafe, "here" 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 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..94c653032fba --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected @@ -0,0 +1,227 @@ +edges +| 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: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: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: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: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: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: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 | +| 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 | +| 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 | +| 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 | +| 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: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: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: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: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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | +| 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 | 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..d09bcf4b8c8f --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java @@ -0,0 +1,323 @@ +import java.io.File; +import java.io.IOException; + +public class Test { + + 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() throws IOException { + File temp = File.createTempFile("test", "directory"); + temp.mkdir(); + return temp; + } + + static File safe() throws IOException { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + if (temp.mkdir()) { + return temp; + } else { + throw new RuntimeException("Failed to create directory"); + } + } + + static File safe2() throws IOException { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + boolean didMkdirs = temp.mkdirs(); + if (didMkdirs) { + return temp; + } else { + throw new RuntimeException("Failed to create directory"); + } + } + + static File safe3() throws IOException { + File temp = File.createTempFile("test", "directory"); + temp.delete(); + if (!(temp.mkdirs())) { + throw new RuntimeException("Failed to create directory"); + } + return temp; + } + + static File safe4() throws IOException { + boolean success = true; + File temp = File.createTempFile("test", "directory"); + success &= temp.delete(); + 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 safe6() throws IOException { + File temp = File.createTempFile("test", "directory"); + if (temp.delete() && temp.mkdir()) { + return temp; + } else { + 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; + } + + 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; + } + + 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; + } + + static File safe11() throws IOException { + File 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) { + File safe = null; + while (true) { + safe = File.createTempFile("test", "directory"); + if (safe.delete() && safe.mkdir()) { + break; + } + } + safe12temp = safe; + } + 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); + } + } + + 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 workDir; + } + + private static void ensureDirectory(File dir) throws IOException { + if (!dir.mkdirs() && !dir.isDirectory()) { + 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; + } + + 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()); + } + } + + 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()); + } + } + + 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(); + } +}