Add documentation and additional test cases

Author: JLLeitschuh

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/javase/8/docs/api/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/javase/8/docs/api/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/javase/8/docs/api/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/javase/8/docs/api/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/javase/8/docs/api/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 guarnteed 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>

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

@@ -95,17 +95,32 @@ private predicate safeUse(Expr e) {
   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
+    (
+      addExp.getRightOperand() = e
+      or
+      // A method call, like `File.getAbsolutePath()` is being called and concatenated into a string
+      exists(MethodAccess fileMethodAccess |
+        fileMethodAccess.getQualifier() = e and
+        addExp.getRightOperand() = fileMethodAccess and
+        fileMethodAccess.getMethod().getReturnType() instanceof TypeString
+      )
+    )
+  )
 }
 
 from
   DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2,
-  DataFlow2::Node sink, MethodAccess creationCall, TempDirHijackingToDeleteConfig toDeleteConfig,
+  DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe, TempDirHijackingToDeleteConfig toDeleteConfig,
   TempDirHijackingFromDeleteConfig fromDeleteConfig
 where
   toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and
   fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and
   deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and
-  isUnsafeUseUnconstrainedByIfCheck(sink, _) and
+  isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and
   isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall)
 select deleteCheckpoint.getNode(), source, deleteCheckpoint,
-  "Local temporary directory hijacking race condition $@", creationCall, "here"
+  "Local temporary directory hijacking race condition $@ file $@ may have been hijacked", creationCall, "here", unsafe, "here"

java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java

@@ -92,5 +92,37 @@ static File safe6() throws IOException {
             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;
+    }
 
 }