Start taint hijacking tracking with java.io.tmpdir

github · JLLeitschuh · fbecfdd8184f0f7892211f739f3f8648cdd8dc37 · Marcono1234 · JLLeitschuh · Marcono1234
commit fbecfdd8184f0f7892211f739f3f8648cdd8dc37
@@ -64,7 +64,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 |

@@ -46,20 +46,26 @@ private class MethodFileDelete extends Method {
 }
 
 predicate isDeleteFileExpr(Expr expr) {
-  exists(MethodAccess ma |
-    expr = ma.getQualifier() and
-    ma.getMethod() instanceof MethodFileDelete
-  )
+  expr = any(MethodFileDelete m).getAReference().getQualifier()
 }
 
-private class TempDirHijackingToDeleteConfig extends DataFlow::Configuration {
+private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuration {
   TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" }
 
   override predicate isSource(DataFlow::Node source) {
-    exists(MethodAccess ma |
-      ma.getMethod() instanceof MethodFileCreateTempFile and
-      source.asExpr() = ma
-    )
+    source.asExpr() =
+      any(MethodAccess ma |
+        ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2
+      ) or
+      // TODO: Replace with getSystemProperty("java.io.tmpdir")
+    source.asExpr() = any(MethodAccessSystemGetProperty maSgp | maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir"))
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    node2.asExpr() =
+      any(MethodAccess ma |
+        ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr()
+      )
   }
 
   override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) }
@@ -137,11 +143,10 @@ private predicate safeUse(Expr e) {
 
 from
   DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2,
-  DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe,
-  TempDirHijackingToDeleteConfig toDeleteConfig, TempDirHijackingFromDeleteConfig fromDeleteConfig
+  DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe
 where
-  toDeleteConfig.hasFlowPath(source, deleteCheckpoint) and
-  fromDeleteConfig.hasFlow(deleteCheckpoint2, sink) and
+  any(TempDirHijackingToDeleteConfig c).hasFlowPath(source, deleteCheckpoint) and
+  any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint2, sink) and
   deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and
   isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and
   isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall)

@@ -124,5 +124,19 @@ static File safe9() throws IOException {
         }
         return temp;
     }
+
+    static File vulnerable2() throws IOException {
+        File temp = File.createTempFile("test", "directory", new File(System.getProperty("java.io.tmpdir")));
+        temp.delete();
+        temp.mkdir();
+        return temp;
+    }
+
+    static File safe10() throws IOException {
+        File temp = File.createTempFile("test", "directory", new File(System.getProperty("user.dir")));
+        temp.delete();
+        temp.mkdir();
+        return temp;
+    }
 
 }