revamp weak file permissions query

alexrford · alexrford · commit d1f2258d45ae · 2021-09-21T19:02:11.000+01:00
diff --git a/ql/lib/codeql/ruby/Concepts.qll b/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.
+   */
+  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.
+ */
+class FileSystemReadAccess extends FileSystemAccess {
+  override FileSystemReadAccess::Range range;
+
+  /**
+   * Gets a node that represents data read from the file system access.
    */
-  DataFlow::Node getRootPathArgument() { none() }
+  DataFlow::Node getADataNode() { result = range.getADataNode() }
+}
 
+/** Provides a class for modeling new file system reads. */
+module FileSystemReadAccess {
   /**
-   * Holds if this file system access will reject paths containing upward navigation
-   * segments (`../`).
+   * A data flow node that reads data from the file system.
    *
-   * `argument` should refer to the relevant path argument or root path argument.
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `FileSystemReadAccess` instead.
    */
-  predicate isUpwardNavigationRejected(DataFlow::Node argument) { none() }
+  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 reads data from the file system.
+ * 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.
  */
-abstract class FileSystemReadAccess extends FileSystemAccess {
-  /** Gets a node that represents data from the file system. */
-  abstract DataFlow::Node getADataNode();
+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.
diff --git a/ql/lib/codeql/ruby/frameworks/Files.qll b/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
-    )
+/**
+ * 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"]) }
+
+    override DataFlow::Node getAPathArgument() { result = this.getArgument(0) }
+
+    override DataFlow::Node getADataNode() { result = this }
   }
 
-  MethodCall getCall() { result = call.asExpr().getExpr() }
-}
-*/
+  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;
 
-class StdLibFileNameSource extends FileNameSource {
-  StdLibFileNameSource() {
-    this = API::getTopLevelMember("File").getAMethodCall(["join", "path", "to_path", "readlink"])
+    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 }
   }
 }
 
-/**
- * Classes and predicates for modelling the `File` module from the standard
- * library.
- */
-private module File {
+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;
 
-  class FileModuleReader extends FileSystemReadAccess, DataFlow::CallNode {
-    FileModuleReader() {
-      this = API::getTopLevelMember("File").getAMethodCall(["new", "open"])
+    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 { }
diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
@@ -11,6 +11,7 @@
  */
 
 import ruby
+import codeql.ruby.Concepts
 import codeql.ruby.DataFlow
 import DataFlow::PathGraph
 import codeql.ruby.ApiGraphs
@@ -43,42 +44,21 @@ class PermissivePermissionsExpr extends Expr {
   }
 }
 
-/** 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 PermissivePermissionsConfig extends DataFlow::Configuration {
   PermissivePermissionsConfig() { this = "PermissivePermissionsConfig" }
 
   override predicate isSource(DataFlow::Node source) {
     exists(PermissivePermissionsExpr ppe | source.asExpr().getExpr() = ppe)
   }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof PermissionArgument }
+  override predicate isSink(DataFlow::Node sink) {
+    exists(FileSystemPermissionModification mod | mod.getAPermissionNode() = sink)
+  }
 }
 
 from
   DataFlow::PathNode source, DataFlow::PathNode sink, PermissivePermissionsConfig conf,
-  PermissionArgument arg
-where conf.hasFlowPath(source, sink) and arg = sink.getNode()
-select source.getNode(), source, sink, "Overly permissive mask in $@ sets file to $@.",
-  arg.getCall(), arg.getCall().toString(), source.getNode(), source.getNode().toString()
+  FileSystemPermissionModification mod
+where conf.hasFlowPath(source, sink) and mod.getAPermissionNode() = sink.getNode()
+select source.getNode(), source, sink, "Overly permissive mask in $@ sets file to $@.", mod,
+  mod.toString(), source.getNode(), source.getNode().toString()
diff --git a/ql/test/query-tests/security/cwe-732/FilePermissions.rb b/ql/test/query-tests/security/cwe-732/FilePermissions.rb
@@ -1,9 +1,13 @@
 require "fileutils"
 
 def run_chmod_1(filename)
+  # BAD: sets file as world writable
   FileUtils.chmod 0222, filename
+  # BAD: sets file as world writable
   FileUtils.chmod 0622, filename
+  # BAD: sets file as world readable
   FileUtils.chmod 0755, filename
+  # BAD: sets file as world readable + writable
   FileUtils.chmod 0777, filename
 end
 
@@ -14,45 +18,56 @@ def chmod(mode, list, options = {} )
 end
 
 def run_chmod_2(filename)
-  foo = FileUtils
+  foo = File
   bar = foo
-  baz = Dummy
-  # "safe"
+  baz = DummyModule
+  # GOOD: DummyModule is not a known class that performs file permission modifications
   baz.chmod 0755, filename
   baz = bar
-  # unsafe
+  # BAD: sets file as world readable
   baz.chmod 0755, filename
 end
 
 def run_chmod_3(filename)
   # TODO: we currently miss this
   foo = FileUtils
   bar, baz = foo, 7
+  # BAD: sets file as world readable
   bar.chmod 0755, filename
 end
 
 def run_chmod_4(filename)
-  # safe permissions
+  # GOOD: no group/world access
   FileUtils.chmod 0700, filename
+  # GOOD: group/world execute bit only
   FileUtils.chmod 0711, filename
+  # GOOD: world execute bit only
   FileUtils.chmod 0701, filename
+  # GOOD: group execute bit only
   FileUtils.chmod 0710, filename
 end
 
 def run_chmod_5(filename)
   perm = 0777
+  # BAD: sets world rwx
   FileUtils.chmod perm, filename
   perm2 = perm
+  # BAD: sets world rwx
   FileUtils.chmod perm2, filename
 
   perm = "u=wrx,g=rwx,o=x"
   perm2 = perm
+  # BAD: sets group rwx
   FileUtils.chmod perm2, filename
+  # BAD: sets file as world readable
   FileUtils.chmod "u=rwx,o+r", filename
+  # GOOD: sets file as group/world unreadable
   FileUtils.chmod "u=rwx,go-r", filename
+  # BAD: sets group/world as +rw
   FileUtils.chmod "a+rw", filename
 end
 
 def run_chmod_R(filename)
-  File.chmod_R 0755, filename
+  # BAD: sets file as world readable
+  FileUtils.chmod_R 0755, filename
 end
diff --git a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
