From a2a7c735bbdd4dcaa793768e87fda93782ea0dcc Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Mon, 9 May 2022 20:30:07 +0100 Subject: [PATCH] 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