Add additional tests and better tracking of 'unsafe use'

github · JLLeitschuh · Oct 14, 2020 · Oct 14, 2020 · Oct 15, 2020 · Jan 19, 2022
commit 71f5fc550c2a80e73656fa467c56a649489302d2
@@ -44,18 +44,93 @@ private class FromDeleteFileFlowState extends DataFlow::FlowState {
   FromDeleteFileFlowState() { this = "FromDeleteFileFlowState" }
 }
 
+/**
+ * The flow state after the `File::mkdir(s)` method has been called on tracked taint.
+ * This is the final state, and is only used when computing the 'unsafe' call site.
+ */
+private class FromMkdirsFileFlowState extends DataFlow::FlowState {
+  FromMkdirsFileFlowState() { this = "FromMkdirsFileFlowState" }
+}
+
+/**
+ * A guard that is checking if a directory exists, throwing if it does not exist.
+ */
+private Guard directoryExistsGuard(ThrowStmt s) {
+  result =
+    any(MethodAccess existanceCheck |
+      (
+        existanceCheck.getMethod() instanceof MethodFileExists
+        or
+        existanceCheck.getMethod() instanceof MethodFileIsDirectory
+      )
+    ) and
+    result.directlyControls(s.getEnclosingStmt(), false)
+}
+
+/**
+ * A guard that is verifying that the directory is sucsessfully created, throwing when it is not created.
+ */
+private Guard directoryCreationGuard(ThrowStmt s) {
+  result = any(MethodAccess creationCheck |
+    creationCheck.getMethod() instanceof MethodFileCreatesDirs
+  ) and
+  result.directlyControls(s.getEnclosingStmt(), false)
+}
+
+/**
+ * A guard that is safely verfying that a directory is created.
+ * 
+ * 'Safe' means that the directory is guarnteed to have been created, and the directory did not already exist.
+ */
+private Guard safeDirectoryCreationGuard(ThrowStmt s) {
+  result = directoryCreationGuard(s) and
+  // This guard is not 'safe' if a `directoryExistsGuard` is also guarding this throw statement
+  not exists(directoryExistsGuard(s))
+}
+
 /**
  * An expression that will create a directory without throwing an exception if a file/directory already exists.
  */
-private predicate isNonThrowingDirectoryCreationExpression(Expr qualifier, MethodAccess creationCall) {
+private predicate isNonThrowingDirectoryCreationExpression(
+  Expr fileInstanceExpr, MethodAccess creationCall
+) {
   creationCall.getMethod() instanceof MethodFileCreatesDirs and
-  creationCall.getQualifier() = qualifier
+  creationCall.getQualifier() = fileInstanceExpr and
+  // Check for 'throwing creations' and ensure that this call is not used in a throwing case.
+  if creationCall.(Guard).directlyControls(_, _) then // The creation call is used in a guard
+    not creationCall = safeDirectoryCreationGuard(_) // Ensure that the guard is insufficient.
+  else any() // The creation call is not used in a guard. Thus is not a 'throwing creation'.
+  or
+  // Recursively look for methods that encapsulate the above.
+  // Thus, the use of 'helper directory creation methods' are still considered
+  // when assessing if an `unsafeUse` is present or not.
+  exists(Method m, int argumentIndex, Expr arg |
+    arg.(VarAccess).getVariable() = m.getParameter(argumentIndex) and
+    isNonThrowingDirectoryCreationExpression(any(Expr e2 | DataFlow::localExprFlow(arg, e2)),
+      creationCall)
+  |
+    m.getAReference().getArgument(argumentIndex) = fileInstanceExpr
+  )
 }
 
 private class MethodFileDelete extends Method {
   MethodFileDelete() {
-    getDeclaringType() instanceof TypeFile and
-    hasName("delete")
+    this.getDeclaringType() instanceof TypeFile and
+    this.hasName("delete")
+  }
+}
+
+private class MethodFileIsDirectory extends Method {
+  MethodFileIsDirectory() {
+    this.getDeclaringType() instanceof TypeFile and
+    this.hasName("isDirectory")
+  }
+}
+
+private class MethodFileExists extends Method {
+  MethodFileExists() {
+    this.getDeclaringType() instanceof TypeFile and
+    this.hasName("exists")
   }
 }
 
@@ -143,7 +218,13 @@ private class TempDirHijackingFromDirectoryCreateToUnsafeUseConfig extends DataF
 }
 
 private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard {
-  DirectoryCreationBarrierGuard() { isNonThrowingDirectoryCreationExpression(_, this) }
+  Expr fileInstanceExpr;
+
+  DirectoryCreationBarrierGuard() {
+    isNonThrowingDirectoryCreationExpression(fileInstanceExpr, this)
+  }
+
+  Expr getFileInstanceExpr() { result = fileInstanceExpr }
 
   override predicate checks(Expr e, boolean branch) { this.controls(e, branch) }
 }
@@ -153,13 +234,18 @@ private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard {
  * is not guarded by a check of the return value of `File::mkdir(s)`.
  */
 predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) {
-  exists(Guard g, MethodAccess ma |
+  exists(DirectoryCreationBarrierGuard g, MethodAccess ma |
     any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to `mkdir` or `mkdirs`
     sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink
     g = ma and // The guard is the method access
-    any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c)
-        .hasFlow(sink, DataFlow::exprNode(unsafeUse)) and // There is some flow from the sink to an unsafe use of the File
+    ( // TODO: Consider replacing this entire bit of logic with `TempDirHijackingFullPathInlcudingUnsafeUse`.
+      any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c)
+          .hasFlow(sink, DataFlow::exprNode(unsafeUse)) // There is some flow from the sink to an unsafe use of the File
+      or
+      DataFlow::localExprFlow(g.getFileInstanceExpr(), unsafeUse) // There is some local flow from the passed file instance to an unsafe use
+    ) and
     unsafeUse != sink.asExpr() and // The unsafe use is not the sink itself
+    not g.getFileInstanceExpr() = unsafeUse and // The unsafe use is not the file instance
     not safeUse(unsafeUse) and // The unsafe use is not a safe use
     not g.controls(unsafeUse.getBasicBlock(), true) and
     not booleanVarAccessGuardsGuard(g) // The guard is not guarded by a boolean variable access guard
@@ -242,29 +328,39 @@ private predicate safeUse(Expr e) {
   not DataFlow::localExprFlow(getStringAccessOnFile(e), any(Expr sink | not safeUse(sink)))
 }
 
+private predicate isSourceUnified(DataFlow::Node source, DataFlow::FlowState state) {
+  exists(DirHijackingTaintSource taintSource |
+    source = taintSource and state = taintSource.getFlowState()
+  )
+}
+
+private predicate isAdditionalTaintStepUnified(
+  DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2, DataFlow::FlowState state2
+) {
+  // From a temporary directory system property access to `File.createTempFile(_, _, flow)` call
+  isTaintStepFileCreateTempFile(node1, node2) and
+  state1 instanceof FromSystemPropertyFlowState and
+  state2 instanceof FromFileCreateTempFileFlowState
+  or
+  // From `File.createTempFile(_, _, flow)` to a call to `File::delete`
+  node1 = node2 and
+  isDeleteFileExpr(node1.asExpr()) and
+  state1 instanceof FromFileCreateTempFileFlowState and
+  state2 instanceof FromDeleteFileFlowState
+}
+
 private class TempDirHijackingFullPath extends TaintTracking::Configuration {
   TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" }
 
   override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
-    exists(DirHijackingTaintSource taintSource |
-      source = taintSource and state = taintSource.getFlowState()
-    )
+    isSourceUnified(source, state)
   }
 
   override predicate isAdditionalTaintStep(
     DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
     DataFlow::FlowState state2
   ) {
-    // From a temporary directory system property access to `File.createTempFile(_, _, flow)` call
-    isTaintStepFileCreateTempFile(node1, node2) and
-    state1 instanceof FromSystemPropertyFlowState and
-    state2 instanceof FromFileCreateTempFileFlowState
-    or
-    // From `File.createTempFile(_, _, flow)` to a call to `File::delete`
-    node1 = node2 and
-    isDeleteFileExpr(node1.asExpr()) and
-    state1 instanceof FromFileCreateTempFileFlowState and
-    state2 instanceof FromDeleteFileFlowState
+    isAdditionalTaintStepUnified(node1, state1, node2, state2)
   }
 
   override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
@@ -274,14 +370,57 @@ private class TempDirHijackingFullPath extends TaintTracking::Configuration {
   }
 }
 
+/**
+ * A configuration that tracks the full path of the temporary directory taint all the way to an
+ * `unsafeUse` of the potentially hijacked temporary directory.
+ *
+ * This is intentionally not used in the the generation of the displayed path;
+ * this is because there may not exist an explicit path from the call to `File::mkdir(s)` call to the the `unsafeUse`.
+ */
+class TempDirHijackingFullPathInlcudingUnsafeUse extends TaintTracking2::Configuration {
+  TempDirHijackingFullPathInlcudingUnsafeUse() {
+    this = "TempDirHijackingFullPathInlcudingUnsafeUse"
+  }
+
+  override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
+    isSourceUnified(source, state)
+  }
+
+  override predicate isAdditionalTaintStep(
+    DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
+    DataFlow::FlowState state2
+  ) {
+    isAdditionalTaintStepUnified(node1, state1, node2, state2)
+    or // `File::mkdir(s)` is not an end-state when looking for an unsafe use
+    isNonThrowingDirectoryCreationExpression(node1.asExpr(), _) and
+    node1.asExpr() = node2.asExpr() and
+    state1 instanceof FromDeleteFileFlowState and
+    state2 instanceof FromMkdirsFileFlowState
+  }
+
+  override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+    // From a `File::delete` call, to a call to `File::mkdir(s)`
+    isUnsafeUseUnconstrainedByIfCheck(_, sink.asExpr()) and
+    state instanceof FromMkdirsFileFlowState
+  }
+}
+
 from
   DataFlow::PathNode pathSource, DataFlow::PathNode pathSink, DataFlow::Node deleteCheckpoint,
   MethodAccess creationCall, Expr unsafe
 where
-  exists(TempDirHijackingFullPath c | c.hasFlowPath(pathSource, pathSink)) and
+  // Generate the full path to display to the user
+  any(TempDirHijackingFullPath c).hasFlowPath(pathSource, pathSink) and
+  // Find the delete checkpoint to display to the user
   any(TempDirHijackingToDeleteConfig c).hasFlow(pathSource.getNode(), deleteCheckpoint) and
+  // Find the delete checkpoint to display to the user
   any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and
+  // Find a full path where an `unsafe` use is present
+  any(TempDirHijackingFullPathInlcudingUnsafeUse c)
+      .hasFlow(pathSource.getNode(), DataFlow::exprNode(unsafe)) and
+  // Verify the unsafe use is not constrained by an if check
   isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and
+  // Verfy the creation call expression is the expected sink
   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.",