Merge pull request #170 from github/weak-file-permissions

Add `rb/overly-permissive-file` query
2026-04-24 00:05:14 +02:00 · 2021-05-14 17:04:15 +01:00
parent 6c382ccd4b b2f2f786ac
commit 3c0f20cec8
6 changed files with 250 additions and 0 deletions
--- a/ql/src/codeql_ruby/ast/Literal.qll
+++ b/ql/src/codeql_ruby/ast/Literal.qll
@@ -47,6 +47,42 @@ class IntegerLiteral extends NumericLiteral, TIntegerLiteral {

  final override string getValueText() { result = g.getValue() }

+  /** Gets the numerical value of this integer literal. */
+  final int getValue() {
+    exists(string s, string values, string str |
+      s = this.getValueText().toLowerCase() and
+      (
+        s.matches("0b%") and
+        values = "01" and
+        str = s.suffix(2)
+        or
+        s.matches("0x%") and
+        values = "0123456789abcdef" and
+        str = s.suffix(2)
+        or
+        s.charAt(0) = "0" and
+        not s.charAt(1) = ["b", "x", "o"] and
+        values = "01234567" and
+        str = s.suffix(1)
+        or
+        s.matches("0o%") and
+        values = "01234567" and
+        str = s.suffix(2)
+        or
+        s.charAt(0) != "0" and values = "0123456789" and str = s
+      )
+    |
+      result =
+        sum(int index, string c, int v, int exp |
+          c = str.replaceAll("_", "").charAt(index) and
+          v = values.indexOf(c.toLowerCase()) and
+          exp = str.replaceAll("_", "").length() - index - 1
+        |
+          v * values.length().pow(exp)
+        )
+    )
+  }
+
  final override string toString() { result = this.getValueText() }

  final override string getAPrimaryQlClass() { result = "IntegerLiteral" }
--- a/ql/src/queries/security/cwe-732/WeakFilePermissions.qhelp
+++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.qhelp
@@ -0,0 +1,26 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+
+<overview>
+<p>
+When creating a file, POSIX systems allow permissions to be specified
+for owner, group and others separately. Permissions should be kept as
+strict as possible, preventing access to the files contents by other users.
+</p>
+
+</overview>
+
+<recommendation>
+<p>
+Restrict the file permissions of files to prevent any but the owner being able to read or write to that file
+</p>
+</recommendation>
+
+<references>
+<li>
+Wikipedia:
+<a href="https://en.wikipedia.org/wiki/File_system_permissions">File system permissions</a>.
+</li>
+</references>
+
+</qhelp>
--- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
+++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
@@ -0,0 +1,99 @@
+/**
+ * @name Overly permissive file permissions
+ * @description Allowing files to be readable or writable by users other than the owner may allow sensitive information to be accessed.
+ * @kind path-problem
+ * @problem.severity warning
+ * @id rb/overly-permissive-file
+ * @tags external/cwe/cwe-732
+ *       security
+ * @precision low
+ */
+
+import ruby
+import codeql_ruby.DataFlow
+import DataFlow::PathGraph
+private import codeql_ruby.dataflow.SSA
+
+// TODO: account for flows through tuple assignments
+/** An expression referencing the File or FileUtils module */
+class FileModuleAccess extends Expr {
+  FileModuleAccess() {
+    this.(ConstantAccess).getName() = "File"
+    or
+    this.(ConstantAccess).getName() = "FileUtils"
+    or
+    exists(FileModuleAccess fma, Ssa::WriteDefinition def |
+      def.getARead() = this.getAControlFlowNode() and
+      def.getWriteAccess().getParent().(Assignment).getRightOperand() = fma
+    )
+  }
+}
+
+bindingset[p]
+int world_permission(int p) { result = p.bitAnd(7) }
+
+// 70 oct = 56 dec
+bindingset[p]
+int group_permission(int p) { result = p.bitAnd(56) }
+
+bindingset[p]
+string access(int p) {
+  p.bitAnd(2) != 0 and result = "writable"
+  or
+  p.bitAnd(4) != 0 and result = "readable"
+}
+
+/** An expression specifing a file permission that allows group/others read or write access */
+class PermissivePermissionsExpr extends Expr {
+  // TODO: non-literal expressions?
+  PermissivePermissionsExpr() {
+    exists(int perm, string acc |
+      perm = this.(IntegerLiteral).getValue() and
+      (acc = access(world_permission(perm)) or acc = access(group_permission(perm)))
+    )
+    or
+    // adding/setting read or write permissions for all/group/other
+    this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=][xXst]*[rw].*")
+  }
+}
+
+/** A call to a method of File or FileUtils that may modify file permissions */
+class PermissionSettingMethodCall extends MethodCall {
+  private string methodName;
+  private Expr permArg;
+
+  PermissionSettingMethodCall() {
+    this.getReceiver() instanceof FileModuleAccess and
+    this.getMethodName() = methodName and
+    (
+      methodName in ["chmod", "chmod_R", "lchmod"] and permArg = this.getArgument(0)
+      or
+      methodName = "mkfifo" and permArg = this.getArgument(1)
+      or
+      methodName in ["new", "open"] and permArg = this.getArgument(2)
+      or
+      methodName in ["install", "makedirs", "mkdir", "mkdir_p", "mkpath"] and
+      permArg = this.getKeywordArgument("mode")
+      // TODO: defaults for optional args? This may depend on the umask
+    )
+  }
+
+  Expr getPermissionArgument() { result = permArg }
+}
+
+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) {
+    exists(PermissionSettingMethodCall c | sink.asExpr().getExpr() = c.getPermissionArgument())
+  }
+}
+
+from DataFlow::PathNode source, DataFlow::PathNode sink, PermissivePermissionsConfig conf
+where conf.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "Overly permissive mask sets file to $@.", source.getNode(),
+  source.getNode().toString()
--- a/ql/test/query-tests/security/cwe-732/FilePermissions.rb
+++ b/ql/test/query-tests/security/cwe-732/FilePermissions.rb
@@ -0,0 +1,58 @@
+require "fileutils"
+
+def run_chmod_1(filename)
+  FileUtils.chmod 0222, filename
+  FileUtils.chmod 0622, filename
+  FileUtils.chmod 0755, filename
+  FileUtils.chmod 0777, filename
+end
+
+module DummyModule
+  def chmod(mode, list, options = {} )
+    list
+  end
+end
+
+def run_chmod_2(filename)
+  foo = FileUtils
+  bar = foo
+  baz = Dummy
+  # "safe"
+  baz.chmod 0755, filename
+  baz = bar
+  # unsafe
+  baz.chmod 0755, filename
+end
+
+def run_chmod_3(filename)
+  # TODO: we currently miss this
+  foo = FileUtils
+  bar, baz = foo, 7
+  bar.chmod 0755, filename
+end
+
+def run_chmod_4(filename)
+  # safe permissions
+  FileUtils.chmod 0700, filename
+  FileUtils.chmod 0711, filename
+  FileUtils.chmod 0701, filename
+  FileUtils.chmod 0710, filename
+end
+
+def run_chmod_5(filename)
+  perm = 0777
+  FileUtils.chmod perm, filename
+  perm2 = perm
+  FileUtils.chmod perm2, filename
+
+  perm = "u=wrx,g=rwx,o=x"
+  perm2 = perm
+  FileUtils.chmod perm2, filename
+  FileUtils.chmod "u=rwx,o+r", filename
+  FileUtils.chmod "u=rwx,go-r", filename
+  FileUtils.chmod "a+rw", filename
+end
+
+def run_chmod_R(filename)
+  File.chmod_R 0755, filename
+end
--- a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
+++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
@@ -0,0 +1,30 @@
+edges
+| FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:44:19:44:22 | perm |
+| FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:46:19:46:23 | perm2 |
+| FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" :  | FilePermissions.rb:50:19:50:23 | perm2 |
+nodes
+| FilePermissions.rb:4:19:4:22 | 0222 | semmle.label | 0222 |
+| FilePermissions.rb:5:19:5:22 | 0622 | semmle.label | 0622 |
+| FilePermissions.rb:6:19:6:22 | 0755 | semmle.label | 0755 |
+| FilePermissions.rb:7:19:7:22 | 0777 | semmle.label | 0777 |
+| FilePermissions.rb:24:13:24:16 | 0755 | semmle.label | 0755 |
+| FilePermissions.rb:43:10:43:13 | 0777 :  | semmle.label | 0777 :  |
+| FilePermissions.rb:44:19:44:22 | perm | semmle.label | perm |
+| FilePermissions.rb:46:19:46:23 | perm2 | semmle.label | perm2 |
+| FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" :  | semmle.label | "u=wrx,g=rwx,o=x" :  |
+| FilePermissions.rb:50:19:50:23 | perm2 | semmle.label | perm2 |
+| FilePermissions.rb:51:19:51:29 | "u=rwx,o+r" | semmle.label | "u=rwx,o+r" |
+| FilePermissions.rb:53:19:53:24 | "a+rw" | semmle.label | "a+rw" |
+| FilePermissions.rb:57:16:57:19 | 0755 | semmle.label | 0755 |
+#select
+| FilePermissions.rb:4:19:4:22 | 0222 | FilePermissions.rb:4:19:4:22 | 0222 | FilePermissions.rb:4:19:4:22 | 0222 | Overly permissive mask sets file to $@. | FilePermissions.rb:4:19:4:22 | 0222 | 0222 |
+| FilePermissions.rb:5:19:5:22 | 0622 | FilePermissions.rb:5:19:5:22 | 0622 | FilePermissions.rb:5:19:5:22 | 0622 | Overly permissive mask sets file to $@. | FilePermissions.rb:5:19:5:22 | 0622 | 0622 |
+| FilePermissions.rb:6:19:6:22 | 0755 | FilePermissions.rb:6:19:6:22 | 0755 | FilePermissions.rb:6:19:6:22 | 0755 | Overly permissive mask sets file to $@. | FilePermissions.rb:6:19:6:22 | 0755 | 0755 |
+| FilePermissions.rb:7:19:7:22 | 0777 | FilePermissions.rb:7:19:7:22 | 0777 | FilePermissions.rb:7:19:7:22 | 0777 | Overly permissive mask sets file to $@. | FilePermissions.rb:7:19:7:22 | 0777 | 0777 |
+| FilePermissions.rb:24:13:24:16 | 0755 | FilePermissions.rb:24:13:24:16 | 0755 | FilePermissions.rb:24:13:24:16 | 0755 | Overly permissive mask sets file to $@. | FilePermissions.rb:24:13:24:16 | 0755 | 0755 |
+| FilePermissions.rb:44:19:44:22 | perm | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:44:19:44:22 | perm | Overly permissive mask sets file to $@. | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
+| FilePermissions.rb:46:19:46:23 | perm2 | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:46:19:46:23 | perm2 | Overly permissive mask sets file to $@. | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
+| FilePermissions.rb:50:19:50:23 | perm2 | FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" :  | FilePermissions.rb:50:19:50:23 | perm2 | Overly permissive mask sets file to $@. | FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" | "u=wrx,g=rwx,o=x" |
+| FilePermissions.rb:51:19:51:29 | "u=rwx,o+r" | FilePermissions.rb:51:19:51:29 | "u=rwx,o+r" | FilePermissions.rb:51:19:51:29 | "u=rwx,o+r" | Overly permissive mask sets file to $@. | FilePermissions.rb:51:19:51:29 | "u=rwx,o+r" | "u=rwx,o+r" |
+| FilePermissions.rb:53:19:53:24 | "a+rw" | FilePermissions.rb:53:19:53:24 | "a+rw" | FilePermissions.rb:53:19:53:24 | "a+rw" | Overly permissive mask sets file to $@. | FilePermissions.rb:53:19:53:24 | "a+rw" | "a+rw" |
+| FilePermissions.rb:57:16:57:19 | 0755 | FilePermissions.rb:57:16:57:19 | 0755 | FilePermissions.rb:57:16:57:19 | 0755 | Overly permissive mask sets file to $@. | FilePermissions.rb:57:16:57:19 | 0755 | 0755 |
--- a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.qlref
+++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.qlref
@@ -0,0 +1 @@
+queries/security/cwe-732/WeakFilePermissions.ql
				`@@ -0,0 +1 @@`
				`queries/security/cwe-732/WeakFilePermissions.ql`