diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index 60721dbf7f2..9a3fbfe55fb 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -13,12 +13,18 @@ import java import TempDirUtils import DataFlow::PathGraph +import semmle.code.java.dataflow.TaintTracking2 -private class MethodFileSystemFileCreation extends Method { - MethodFileSystemFileCreation() { - this.getDeclaringType() instanceof TypeFile and - this.hasName(["mkdir", "mkdirs", "createNewFile"]) - } +abstract private class MethodFileSystemFileCreation extends Method { + MethodFileSystemFileCreation() { this.getDeclaringType() instanceof TypeFile } +} + +private class MethodFileDirectoryCreation extends MethodFileSystemFileCreation { + MethodFileDirectoryCreation() { this.hasName(["mkdir", "mkdirs"]) } +} + +private class MethodFileFileCreation extends MethodFileSystemFileCreation { + MethodFileFileCreation() { this.hasName(["createNewFile"]) } } abstract private class FileCreationSink extends DataFlow::Node { } @@ -113,7 +119,10 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf isAdditionalFileTaintStep(node1, node2) } - override predicate isSink(DataFlow::Node sink) { sink instanceof FileCreationSink } + override predicate isSink(DataFlow::Node sink) { + sink instanceof FileCreationSink and + exists(TempDirSystemGetPropertyDirectlyToMkdirConfig config | not config.hasFlowTo(sink)) + } override predicate isSanitizer(DataFlow::Node sanitizer) { exists(FilesSanitizingCreationMethodAccess sanitisingMethodAccess | @@ -122,6 +131,42 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf } } +/** + * Configuration that tracks calls to to `mkdir` or `mkdirs` that are are directly on the temp directory system property. + * Examples: + * - `File tempDir = new File(System.getProperty("java.io.tmpdir")); tempDir.mkdir();` + * - `File tempDir = new File(System.getProperty("java.io.tmpdir")); tempDir.mkdirs();` + * + * These are examples of code that is simply verifying that the temp directory exists. + * As such, this code pattern is filtered out as an explicit vulnerability in + * `TempDirSystemGetPropertyToCreateConfig::isSink`. + */ +private class TempDirSystemGetPropertyDirectlyToMkdirConfig extends TaintTracking2::Configuration { + TempDirSystemGetPropertyDirectlyToMkdirConfig() { + this = "TempDirSystemGetPropertyDirectlyToMkdirConfig" + } + + override predicate isSource(DataFlow::Node node) { + exists( + MethodAccessSystemGetPropertyTempDirTainted propertyGetMethodAccess, DataFlow::Node callSite + | + DataFlow::localFlow(DataFlow::exprNode(propertyGetMethodAccess), callSite) + | + isFileConstructorArgument(callSite.asExpr(), node.asExpr(), 1) + ) + } + + override predicate isSink(DataFlow::Node node) { + exists(MethodAccess ma | ma.getMethod() instanceof MethodFileDirectoryCreation | + ma.getQualifier() = node.asExpr() + ) + } + + override predicate isSanitizer(DataFlow::Node sanitizer) { + isFileConstructorArgument(sanitizer.asExpr(), _, _) + } +} + // // Begin configuration for tracking single-method calls that are vulnerable. // diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll index c6df9549dc6..0c2409c0408 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll +++ b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll @@ -46,11 +46,12 @@ class MethodFileCreateTempFile extends Method { /** * Holds if `expDest` is some constructor call `new java.io.File(x)` and `expSource` is `x`. */ -private predicate isFileConstructorArgument(Expr expSource, Expr exprDest) { +predicate isFileConstructorArgument(Expr expSource, Expr exprDest, int paramCount) { exists(ConstructorCall construtorCall | construtorCall.getConstructedType() instanceof TypeFile and construtorCall.getArgument(0) = expSource and - construtorCall = exprDest + construtorCall = exprDest and + construtorCall.getConstructor().getNumberOfParameters() = paramCount ) } @@ -77,7 +78,7 @@ private predicate isTaintPropagatingFileTransformation(Expr expSource, Expr expr * For example, `taintedFile.getCanonicalFile()` is itself tainted. */ predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2) { - isFileConstructorArgument(node1.asExpr(), node2.asExpr()) or + isFileConstructorArgument(node1.asExpr(), node2.asExpr(), _) or isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr()) } diff --git a/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java index fc1e9eb0904..b5b708692f1 100644 --- a/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java @@ -269,4 +269,14 @@ public class Test { tempFile.setReadable(false, false); tempFile.setReadable(true, true); } + + void notVulnerableCreateOnSystemPropertyDir() throws IOException { + File tempDir = new File(System.getProperty("java.io.tmpdir")); + tempDir.mkdir(); + } + + void notVulnerableCreateOnSystemPropertyDirs() throws IOException { + File tempDir = new File(System.getProperty("java.io.tmpdir")); + tempDir.mkdirs(); + } }