Java: CWE-378: Temp Directory Hijacking Race Condition Vulnerability

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.

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)
-}

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"])
+  }
+}

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))
     )
   }
 

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 |

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();
+    }
+}

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;
+    }
+}

java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.qhelp

@@ -0,0 +1,45 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Local temporary directory hijacking can occur when files/directories are created within directories that are shared between all users on the system.</p>
+
+<p>On most <a href="https://en.wikipedia.org/wiki/Unix-like">unix-like</a> 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.</p>
+
+<p>In the worst case, where local privilege escalation occurs, this vulnerability can have a
+<a href="https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:L/AC:L/PR:L/UI:N/S:U/C:H/I:H/A:H&amp;version=3.1">CVSS v3.1 base score of 7.8</a>.</p>
+</overview>
+
+<recommendation>
+<p>Use JDK methods that specifically protect against this vulnerability:</p>
+<ul>
+    <li><a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Files.html#createTempDirectory(java.nio.file.Path,java.lang.String,java.nio.file.attribute.FileAttribute...)">java.nio.file.Files.createTempDirectory</a></li>
+    <li><a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Files.html#createTempFile(java.nio.file.Path,java.lang.String,java.lang.String,java.nio.file.attribute.FileAttribute...)">java.nio.file.Files.createTempFile</a></li>
+</ul>
+<p>Otherwise, create the file/directory by manually specifying the expected posix file permissions.
+For example: <code>PosixFilePermissions.asFileAttribute(EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE))</code></p>
+<ul>
+    <li><a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Files.html#createFile(java.nio.file.Path,java.nio.file.attribute.FileAttribute...)">java.nio.file.Files.createFile</a></li>
+    <li><a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Files.html#createDirectory(java.nio.file.Path,java.nio.file.attribute.FileAttribute...)">java.nio.file.Files.createDirectory</a></li>
+    <li><a href="https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/nio/file/Files.html#createDirectories(java.nio.file.Path,java.nio.file.attribute.FileAttribute...)">java.nio.file.Files.createDirectories</a></li>
+</ul>
+</recommendation>
+
+<example>
+    <p>In the following example, the directory that is created can be hijacked during the creation process due to a race condition.</p>
+    <sample src="TempDirCreationVulnerable.java"/>
+
+    <p>In the following example, the <code>Files.createTempDirectory</code> API is used which creates a temporary directory that is guaranteed to be safe.</p>
+    <sample src="TempDirCreationSafe.java"/>
+</example>
+
+<references>
+    <li>OSWAP: <a href="https://owasp.org/www-community/vulnerabilities/Insecure_Temporary_File">Insecure Temporary File</a>.</li>
+    <li>CERT: <a href="https://wiki.sei.cmu.edu/confluence/display/java/FIO00-J.+Do+not+operate+on+files+in+shared+directories">FIO00-J. Do not operate on files in shared directories</a>.</li>
+</references>
+</qhelp>