Refactor TempDirHijacking to show complete path

github · Mar 16, 2022 · 5a9ed32 · 5a9ed32
1 parent 3e2beb0
commit 5a9ed32
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 52 deletions.
diff --git a/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql b/java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql
@@ -11,6 +11,9 @@ import java
 import semmle.code.java.controlflow.Guards
 import semmle.code.java.dataflow.FlowSources
 import semmle.code.java.security.TempFileLib
+import semmle.code.java.dataflow.DataFlow3
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.TaintTracking2
 import DataFlow::PathGraph
 
 /**
@@ -31,33 +34,41 @@ predicate isDeleteFileExpr(Expr expr) {
   expr = any(MethodFileDelete m).getAReference().getQualifier()
 }
 
-private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuration {
-  TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" }
-
-  override predicate isSource(DataFlow::Node source) {
-    source.asExpr() =
+private class DirHijackingTaintSource extends DataFlow::Node {
+  DirHijackingTaintSource() {
+    this.asExpr() =
       any(MethodAccess ma |
         ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2
       )
     or
     // TODO: Replace with getSystemProperty("java.io.tmpdir")
-    source.asExpr() =
+    this.asExpr() =
       any(MethodAccessSystemGetProperty maSgp |
         maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir")
       )
   }
+}
+
+private predicate isAdditionalTaintStepCommon(DataFlow::Node node1, DataFlow::Node node2) {
+  node2.asExpr() =
+    any(MethodAccess ma |
+      ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr()
+    )
+}
+
+private class TempDirHijackingToDeleteConfig extends TaintTracking2::Configuration {
+  TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource }
 
   override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
-    node2.asExpr() =
-      any(MethodAccess ma |
-        ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr()
-      )
+    isAdditionalTaintStepCommon(node1, node2)
   }
 
   override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) }
 }
 
-private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration {
+private class TempDirHijackingFromDeleteConfig extends DataFlow3::Configuration {
   TempDirHijackingFromDeleteConfig() { this = "TempDirHijackingFromDeleteConfig" }
 
   override predicate isSource(DataFlow::Node source) { isDeleteFileExpr(source.asExpr()) }
@@ -127,15 +138,31 @@ private predicate safeUse(Expr e) {
   )
 }
 
+private class TempDirHijackingFullPath extends TaintTracking::Configuration {
+  TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    isNonThrowingDirectoryCreationExpression(sink.asExpr(), _)
+  }
+
+  override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
+    isAdditionalTaintStepCommon(node1, node2)
+    or
+    node1.asExpr() = node2.asExpr() and isDeleteFileExpr(node1.asExpr())
+  }
+}
+
 from
-  DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2,
-  DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe
+  DataFlow::PathNode pathSource, DataFlow::PathNode pathSink, DataFlow::Node deleteCheckpoint,
+  MethodAccess creationCall, Expr unsafe
 where
-  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)
-select deleteCheckpoint.getNode(), source, deleteCheckpoint,
-  "Local temporary directory hijacking race condition $@, file $@ may have been hijacked",
-  creationCall, "here", unsafe, "here"
+  any(TempDirHijackingFullPath c).hasFlowPath(pathSource, pathSink) and
+  any(TempDirHijackingToDeleteConfig c).hasFlow(pathSource.getNode(), deleteCheckpoint) and
+  any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and
+  isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and
+  isNonThrowingDirectoryCreationExpression(pathSink.getNode().asExpr(), creationCall)
+select pathSink, pathSource, pathSink,
+  "Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user.",
+  deleteCheckpoint.asExpr(), "delete here", unsafe, "here"
diff --git a/.../ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected b/.../ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected
@@ -1,54 +1,57 @@
 edges
-| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp |
-| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp |
-| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:36:9:36:12 | temp |
-| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:46:9:46:12 | temp |
-| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:57:9:57:12 | temp |
-| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:67:20:67:23 | temp |
-| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:78:20:78:23 | temp |
-| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:13:89:16 | temp |
-| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:14:98:17 | temp |
-| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp |
-| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp |
-| Test.java:129:62:129:107 | new File(...) : File | Test.java:130:9:130:12 | temp |
+| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp |
+| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp |
+| Test.java:29:21:29:60 | createTempFile(...) : File | Test.java:30:9:30:12 | temp |
+| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:37:13:37:16 | temp |
+| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:47:29:47:32 | temp |
+| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:58:15:58:18 | temp |
+| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:68:20:68:23 | temp |
+| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:79:20:79:23 | temp |
+| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:30:89:33 | temp |
+| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:32:98:35 | temp |
+| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:48:110:51 | temp |
+| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:122:14:122:17 | temp |
+| Test.java:129:62:129:107 | new File(...) : File | Test.java:131:9:131:12 | temp |
 | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:129:62:129:107 | new File(...) : File |
 | Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File |
-| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:148:9:148:12 | temp |
+| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:149:9:149:12 | temp |
 | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File |
 nodes
 | Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:12:13:12:16 | temp | semmle.label | temp |
+| Test.java:13:13:13:16 | temp | semmle.label | temp |
 | Test.java:22:21:22:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:23:9:23:12 | temp | semmle.label | temp |
+| Test.java:24:9:24:12 | temp | semmle.label | temp |
+| Test.java:29:21:29:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
+| Test.java:30:9:30:12 | temp | semmle.label | temp |
 | Test.java:35:21:35:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:36:9:36:12 | temp | semmle.label | temp |
+| Test.java:37:13:37:16 | temp | semmle.label | temp |
 | Test.java:45:21:45:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:46:9:46:12 | temp | semmle.label | temp |
+| Test.java:47:29:47:32 | temp | semmle.label | temp |
 | Test.java:56:21:56:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:57:9:57:12 | temp | semmle.label | temp |
+| Test.java:58:15:58:18 | temp | semmle.label | temp |
 | Test.java:66:21:66:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:67:20:67:23 | temp | semmle.label | temp |
+| Test.java:68:20:68:23 | temp | semmle.label | temp |
 | Test.java:77:21:77:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:78:20:78:23 | temp | semmle.label | temp |
+| Test.java:79:20:79:23 | temp | semmle.label | temp |
 | Test.java:88:21:88:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:89:13:89:16 | temp | semmle.label | temp |
+| Test.java:89:30:89:33 | temp | semmle.label | temp |
 | Test.java:97:21:97:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:98:14:98:17 | temp | semmle.label | temp |
+| Test.java:98:32:98:35 | temp | semmle.label | temp |
 | Test.java:109:21:109:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:110:30:110:33 | temp | semmle.label | temp |
+| Test.java:110:48:110:51 | temp | semmle.label | temp |
 | Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
-| Test.java:119:14:119:17 | temp | semmle.label | temp |
+| Test.java:122:14:122:17 | temp | semmle.label | temp |
 | Test.java:129:62:129:107 | new File(...) : File | semmle.label | new File(...) : File |
 | Test.java:129:71:129:106 | getProperty(...) : String | semmle.label | getProperty(...) : String |
-| Test.java:130:9:130:12 | temp | semmle.label | temp |
+| Test.java:131:9:131:12 | temp | semmle.label | temp |
 | Test.java:146:13:146:58 | new File(...) : File | semmle.label | new File(...) : File |
 | Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File |
 | Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String |
-| Test.java:148:9:148:12 | temp | semmle.label | temp |
+| Test.java:149:9:149:12 | temp | semmle.label | temp |
 subpaths
 #select
-| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:9:18:33 | ...=... | here |
-| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:30:18:33 | temp | here |
-| Test.java:23:9:23:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:24:9:24:20 | mkdir(...) | here | Test.java:25:16:25:19 | temp | here |
-| Test.java:130:9:130:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:130:9:130:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:131:9:131:20 | mkdir(...) | here | Test.java:132:16:132:19 | temp | here |
-| Test.java:148:9:148:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:148:9:148:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:149:9:149:20 | mkdir(...) | here | Test.java:150:16:150:19 | temp | here |
+| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here |
+| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:30:18:33 | temp | here |
+| Test.java:24:9:24:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:23:9:23:12 | temp | delete here | Test.java:25:16:25:19 | temp | here |
+| Test.java:131:9:131:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:131:9:131:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:130:9:130:12 | temp | delete here | Test.java:132:16:132:19 | temp | here |
+| Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here |