Fix FP from mkdirs call on exact temp directory

github · Feb 9, 2022 · 49a7367 · 49a7367
1 parent 787e3da
commit 49a7367
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 9 deletions.
diff --git 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
@@ -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
@@ -269,4 +269,14 @@ void safeFileCreationWithPermissions() throws IOException {
         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();
+    }
 }