From 7f466401762f8ec2884246fe7f5913ff29a9030a Mon Sep 17 00:00:00 2001
From: Jonathan Leitschuh <Jonathan.Leitschuh@gmail.com>
Date: Tue, 8 Feb 2022 17:57:10 -0500
Subject: [PATCH] Consider calls to setReadable(false, false) then
 setReadable(true, true) to be safe

---
 .../TempDirLocalInformationDisclosure.ql      | 23 +++++----
 .../src/Security/CWE/CWE-200/TempDirUtils.qll | 51 ++++++++++++++++++-
 .../security/CWE-200/semmle/tests/Test.java   |  6 +++
 3 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
index be14d44b576..7d0a7ecca3e 100644
--- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
+++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
@@ -197,14 +197,17 @@ class InsecureMethodPseudoConfiguration extends DataFlow::Configuration {
 
 from DataFlow::PathNode source, DataFlow::PathNode sink, string message
 where
-  any(TempDirSystemGetPropertyToCreateConfig conf).hasFlowPath(source, sink) and
-  message =
-    "Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users."
-  or
-  any(InsecureMethodPseudoConfiguration conf).hasFlowPath(source, sink) and
-  // Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used.
-  message =
-    "Local information disclosure vulnerability due to use of " +
-      source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() +
-      " readable by other local users."
+  (
+    any(TempDirSystemGetPropertyToCreateConfig conf).hasFlowPath(source, sink) and
+    message =
+      "Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users."
+    or
+    any(InsecureMethodPseudoConfiguration conf).hasFlowPath(source, sink) and
+    // Note this message has no "$@" placeholder, so the "system temp directory" template parameter below is not used.
+    message =
+      "Local information disclosure vulnerability due to use of " +
+        source.getNode().asExpr().(MethodAccessInsecureFileCreation).getFileSystemEntityType() +
+        " readable by other local users."
+  ) and
+  not isPermissionsProtectedTempDirUse(sink.getNode())
 select source.getNode(), source, sink, message, source.getNode(), "system temp directory"
diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll
index 0613006f71f..c6df9549dc6 100644
--- a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll
+++ b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll
@@ -59,8 +59,8 @@ private predicate isFileConstructorArgument(Expr expSource, Expr exprDest) {
  */
 private class TaintFollowingFileMethod extends Method {
   TaintFollowingFileMethod() {
-    getDeclaringType() instanceof TypeFile and
-    hasName(["getAbsoluteFile", "getCanonicalFile"])
+    this.getDeclaringType() instanceof TypeFile and
+    this.hasName(["getAbsoluteFile", "getCanonicalFile"])
   }
 }
 
@@ -80,3 +80,50 @@ predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2)
   isFileConstructorArgument(node1.asExpr(), node2.asExpr()) or
   isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr())
 }
+
+/**
+ * A method call to `java.io.File::setReadable`.
+ */
+private class FileSetRedableMethodAccess extends MethodAccess {
+  FileSetRedableMethodAccess() {
+    exists(Method m | this.getMethod() = m |
+      m.getDeclaringType() instanceof TypeFile and
+      m.hasName("setReadable")
+    )
+  }
+
+  predicate isCallWithArguments(boolean arg1, boolean arg2) {
+    this.isCallWithArgument(0, arg1) and this.isCallToSecondArgumentWithValue(arg2)
+  }
+
+  private predicate isCallToSecondArgumentWithValue(boolean value) {
+    this.getMethod().getNumberOfParameters() = 1 and value = true
+    or
+    isCallWithArgument(1, value)
+  }
+
+  private predicate isCallWithArgument(int index, boolean arg) {
+    DataFlow::localExprFlow(any(CompileTimeConstantExpr e | e.getBooleanValue() = arg),
+      this.getArgument(index))
+  }
+}
+
+/**
+ * Hold's if temporary directory's use is protected if there is an explicit call to
+ * `setReadable(false, false)`, then `setRedabale(true, true)`.
+ */
+predicate isPermissionsProtectedTempDirUse(DataFlow::Node sink) {
+  exists(FileSetRedableMethodAccess setReadable1, FileSetRedableMethodAccess setReadable2 |
+    setReadable1.isCallWithArguments(false, false) and
+    setReadable2.isCallWithArguments(true, true)
+  |
+    exists(DataFlow::Node setReadableNode1, DataFlow::Node setReadableNode2 |
+      setReadableNode1.asExpr() = setReadable1.getQualifier() and
+      setReadableNode2.asExpr() = setReadable2.getQualifier()
+    |
+      DataFlow::localFlow(sink, setReadableNode1) and // Flow from sink to setReadable(false, false)
+      DataFlow::localFlow(sink, setReadableNode2) and // Flow from sink to setReadable(true, true)
+      DataFlow::localFlow(setReadableNode1, setReadableNode2) // Flow from setReadable(false, false) to setReadable(true, true)
+    )
+  )
+}
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 44b8210e658..fc1e9eb0904 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
@@ -263,4 +263,10 @@ public class Test {
         // TO MAKE SAFE REWRITE TO:
         Files.createDirectories(tempDirChild.toPath(), PosixFilePermissions.asFileAttribute(EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)));
     }
+
+    void safeFileCreationWithPermissions() throws IOException {
+        File tempFile = File.createTempFile("temp", "file.txt");
+        tempFile.setReadable(false, false);
+        tempFile.setReadable(true, true);
+    }
 }