revamp weak file permissions query

ql/lib/codeql/ruby/Concepts.qll

@@ -37,45 +37,107 @@ module SqlExecution {
 }
 
 /**
- * A data flow node that performs a file system access (read, write, copy, permissions, stats, etc).
+ * A data flow node that performs a file system access, including reading and writing data,
+ * creating and deleting files and folders, checking and updating permissions, and so on.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `FileSystemAccess::Range` instead.
  */
-abstract class FileSystemAccess extends DataFlow::Node {
+class FileSystemAccess extends DataFlow::Node {
+  FileSystemAccess::Range range;
+
+  FileSystemAccess() { this = range }
+
   /** Gets an argument to this file system access that is interpreted as a path. */
-  abstract DataFlow::Node getAPathArgument();
+  DataFlow::Node getAPathArgument() { result = range.getAPathArgument() }
+}
 
+/** Provides a class for modeling new file system access APIs. */
+module FileSystemAccess {
   /**
-   * Gets an argument to this file system access that is interpreted as a root folder
-   * in which the path arguments are constrained.
+   * A data-flow node that performs a file system access, including reading and writing data,
+   * creating and deleting files and folders, checking and updating permissions, and so on.
    *
-   * In other words, if a root argument is provided, the underlying file access does its own
-   * sanitization to prevent the path arguments from traversing outside the root folder.
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `FileSystemAccess` instead.
    */
-  DataFlow::Node getRootPathArgument() { none() }
-
-  /**
-   * Holds if this file system access will reject paths containing upward navigation
-   * segments (`../`).
-   *
-   * `argument` should refer to the relevant path argument or root path argument.
-   */
-  predicate isUpwardNavigationRejected(DataFlow::Node argument) { none() }
+  abstract class Range extends DataFlow::Node {
+    /** Gets an argument to this file system access that is interpreted as a path. */
+    abstract DataFlow::Node getAPathArgument();
+  }
 }
 
 /**
  * A data flow node that reads data from the file system.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `FileSystemReadAccess::Range` instead.
  */
-abstract class FileSystemReadAccess extends FileSystemAccess {
-  /** Gets a node that represents data from the file system. */
-  abstract DataFlow::Node getADataNode();
+class FileSystemReadAccess extends FileSystemAccess {
+  override FileSystemReadAccess::Range range;
+
+  /**
+   * Gets a node that represents data read from the file system access.
+   */
+  DataFlow::Node getADataNode() { result = range.getADataNode() }
 }
 
+/** Provides a class for modeling new file system reads. */
+module FileSystemReadAccess {
+  /**
+   * A data flow node that reads data from the file system.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `FileSystemReadAccess` instead.
+   */
+  abstract class Range extends FileSystemAccess::Range {
+    /**
+     * Gets a node that represents data read from the file system.
+     */
+    abstract DataFlow::Node getADataNode();
+  }
+}
+
+/**
+ * A data flow node that sets the permissions for one or more files.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `FileSystemPermissionModification::Range` instead.
+ */
+class FileSystemPermissionModification extends DataFlow::Node {
+  FileSystemPermissionModification::Range range;
+
+  FileSystemPermissionModification() { this = range }
+
+  /**
+   * Gets an argument to this permission modification that is interpreted as a
+   * set of permissions.
+   */
+  DataFlow::Node getAPermissionNode() { result = range.getAPermissionNode() }
+}
+
+/** Provides a class for modeling new file system permission modifications. */
+module FileSystemPermissionModification {
+  /**
+   * A data-flow node that sets permissions for a one or more files.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `FileSystemPermissionModification` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Gets an argument to this permission modification that is interpreted as a
+     * set of permissions.
+     */
+    abstract DataFlow::Node getAPermissionNode();
+  }
+}
 
 /**
  * A data flow node that contains a file name or an array of file names from the local file system.
  */
 abstract class FileNameSource extends DataFlow::Node { }
 
-
 /**
  * A data-flow node that escapes meta-characters, which could be used to prevent
  * injection attacks.

ql/lib/codeql/ruby/frameworks/Files.qll

@@ -6,49 +6,84 @@ private import ruby
 private import codeql.ruby.Concepts
 private import codeql.ruby.ApiGraphs
 
-/** A permissions argument of a call to a File/FileUtils method that may modify file permissions */
-/*
-class PermissionArgument extends DataFlow::Node {
-  private DataFlow::CallNode call;
-
-  PermissionArgument() {
-    exists(string methodName |
-      call = API::getTopLevelMember(["File", "FileUtils"]).getAMethodCall(methodName)
-    |
-      methodName in ["chmod", "chmod_R", "lchmod"] and this = call.getArgument(0)
-      or
-      methodName = "mkfifo" and this = call.getArgument(1)
-      or
-      methodName in ["new", "open"] and this = call.getArgument(2)
-      or
-      methodName in ["install", "makedirs", "mkdir", "mkdir_p", "mkpath"] and
-      this = call.getKeywordArgument("mode")
-      // TODO: defaults for optional args? This may depend on the umask
-    )
-  }
-
-  MethodCall getCall() { result = call.asExpr().getExpr() }
-}
-*/
-
-
-class StdLibFileNameSource extends FileNameSource {
-  StdLibFileNameSource() {
-    this = API::getTopLevelMember("File").getAMethodCall(["join", "path", "to_path", "readlink"])
-
-  }
-}
-
 /**
  * Classes and predicates for modelling the `File` module from the standard
  * library.
  */
 private module File {
+  private class FileModuleReader extends FileSystemReadAccess::Range, DataFlow::CallNode {
+    FileModuleReader() { this = API::getTopLevelMember("File").getAMethodCall(["new", "open"]) }
 
-  class FileModuleReader extends FileSystemReadAccess, DataFlow::CallNode {
-    FileModuleReader() {
-      this = API::getTopLevelMember("File").getAMethodCall(["new", "open"])
+    override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
+
+    override DataFlow::Node getADataNode() { result = this }
+  }
+
+  private class FileModuleFilenameSource extends FileNameSource {
+    FileModuleFilenameSource() {
+      // Class methods
+      this =
+        API::getTopLevelMember("File")
+            .getAMethodCall([
+                "absolute_path", "basename", "expand_path", "join", "path", "readlink",
+                "realdirpath", "realpath"
+              ])
     }
   }
 
-}
+  private class FileModulePermissionModification extends FileSystemPermissionModification::Range,
+    DataFlow::CallNode {
+    private DataFlow::Node permissionArg;
+
+    FileModulePermissionModification() {
+      exists(string methodName | this = API::getTopLevelMember("File").getAMethodCall(methodName) |
+        methodName in ["chmod", "lchmod"] and permissionArg = this.getArgument(0)
+        or
+        methodName = "mkfifo" and permissionArg = this.getArgument(1)
+        or
+        methodName in ["new", "open"] and permissionArg = this.getArgument(2)
+        // TODO: defaults for optional args? This may depend on the umask
+      )
+    }
+
+    override DataFlow::Node getAPermissionNode() { result = permissionArg }
+  }
+}
+
+private module FileUtils {
+  private class FileUtilsFilenameSource extends FileNameSource {
+    FileUtilsFilenameSource() {
+      // Note that many methods in FileUtils accept a `noop` option that will
+      // perform a dry run of the command. This means that, for instance, `rm`
+      // and similar methods may not actually delete/unlink a file.
+      this =
+        API::getTopLevelMember("FileUtils")
+            .getAMethodCall([
+                "chmod", "chmod_R", "chown", "chown_R", "getwd", "makedirs", "mkdir", "mkdir_p",
+                "mkpath", "remove", "remove_dir", "remove_entry", "rm", "rm_f", "rm_r", "rm_rf",
+                "rmdir", "rmtree", "safe_unlink", "touch"
+              ])
+    }
+  }
+
+  private class FileUtilsPermissionModification extends FileSystemPermissionModification::Range,
+    DataFlow::CallNode {
+    private DataFlow::Node permissionArg;
+
+    FileUtilsPermissionModification() {
+      exists(string methodName |
+        this = API::getTopLevelMember("FileUtils").getAMethodCall(methodName)
+      |
+        methodName in ["chmod", "chmod_R"] and permissionArg = this.getArgument(0)
+        or
+        methodName in ["install", "makedirs", "mkdir", "mkdir_p", "mkpath"] and
+        permissionArg = this.getKeywordArgument("mode")
+        // TODO: defaults for optional args? This may depend on the umask
+      )
+    }
+
+    override DataFlow::Node getAPermissionNode() { result = permissionArg }
+  }
+}
+
+private module IO { }