From b2d36babc4c293bd97e4b638ddc6fe04f5ada0e6 Mon Sep 17 00:00:00 2001
From: Alex Ford <alexrford@github.com>
Date: Fri, 4 Jun 2021 13:08:41 +0100
Subject: [PATCH 1/2] report rb/weak-file-permission alerts at source rather
 than sink and improve alert message

---
 .../security/cwe-732/WeakFilePermissions.ql   | 34 ++++++++++---------
 .../cwe-732/WeakFilePermissions.expected      | 22 ++++++------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
index 41c0edceb16..c001ce36bb9 100644
--- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
+++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
@@ -57,28 +57,28 @@ class PermissivePermissionsExpr extends Expr {
   }
 }
 
-/** A call to a method of File or FileUtils that may modify file permissions */
-class PermissionSettingMethodCall extends MethodCall {
+/** A permissions argument of a call to a File/FileUtils method that may modify file permissions */
+class PermissionArgument extends Expr {
+  private MethodCall call;
   private string methodName;
-  private Expr permArg;
 
-  PermissionSettingMethodCall() {
-    this.getReceiver() instanceof FileModuleAccess and
-    this.getMethodName() = methodName and
+  PermissionArgument() {
+    call.getReceiver() instanceof FileModuleAccess and
+    call.getMethodName() = methodName and
     (
-      methodName in ["chmod", "chmod_R", "lchmod"] and permArg = this.getArgument(0)
+      methodName in ["chmod", "chmod_R", "lchmod"] and this = call.getArgument(0)
       or
-      methodName = "mkfifo" and permArg = this.getArgument(1)
+      methodName = "mkfifo" and this = call.getArgument(1)
       or
-      methodName in ["new", "open"] and permArg = this.getArgument(2)
+      methodName in ["new", "open"] and this = call.getArgument(2)
       or
       methodName in ["install", "makedirs", "mkdir", "mkdir_p", "mkpath"] and
-      permArg = this.getKeywordArgument("mode")
+      this = call.getKeywordArgument("mode")
       // TODO: defaults for optional args? This may depend on the umask
     )
   }
 
-  Expr getPermissionArgument() { result = permArg }
+  MethodCall getCall() { result = call }
 }
 
 class PermissivePermissionsConfig extends DataFlow::Configuration {
@@ -89,11 +89,13 @@ class PermissivePermissionsConfig extends DataFlow::Configuration {
   }
 
   override predicate isSink(DataFlow::Node sink) {
-    exists(PermissionSettingMethodCall c | sink.asExpr().getExpr() = c.getPermissionArgument())
+    exists(PermissionArgument arg | sink.asExpr().getExpr() = arg)
   }
 }
 
-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()
+from
+  DataFlow::PathNode source, DataFlow::PathNode sink, PermissivePermissionsConfig conf,
+  PermissionArgument arg
+where conf.hasFlowPath(source, sink) and arg = sink.getNode().asExpr().getExpr()
+select source.getNode(), source, sink, "Overly permissive mask in $@ sets file to $@.",
+  arg.getCall(), arg.getCall(), source.getNode(), source.getNode().toString()
diff --git a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
index 965851448ff..30f3b9fddbe 100644
--- a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
+++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
@@ -17,14 +17,14 @@ nodes
 | 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 |
+| FilePermissions.rb:4:19:4:22 | 0222 | FilePermissions.rb:4:19:4:22 | 0222 | FilePermissions.rb:4:19:4:22 | 0222 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:4:3:4:32 | call to chmod | FilePermissions.rb:4:3:4:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:5:3:5:32 | call to chmod | FilePermissions.rb:5:3:5:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:6:3:6:32 | call to chmod | FilePermissions.rb:6:3:6:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:7:3:7:32 | call to chmod | FilePermissions.rb:7:3:7:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:24:3:24:26 | call to chmod | FilePermissions.rb:24:3:24:26 | call to chmod | FilePermissions.rb:24:13:24:16 | 0755 | 0755 |
+| FilePermissions.rb:43:10:43:13 | 0777 | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:44:19:44:22 | perm | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:44:3:44:32 | call to chmod | FilePermissions.rb:44:3:44:32 | call to chmod | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
+| FilePermissions.rb:43:10:43:13 | 0777 | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:46:19:46:23 | perm2 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:46:3:46:33 | call to chmod | FilePermissions.rb:46:3:46:33 | call to chmod | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
+| FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" | FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" :  | FilePermissions.rb:50:19:50:23 | perm2 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:50:3:50:33 | call to chmod | FilePermissions.rb:50:3:50:33 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:51:3:51:39 | call to chmod | FilePermissions.rb:51:3:51:39 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:53:3:53:34 | call to chmod | FilePermissions.rb:53:3:53:34 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:57:3:57:29 | call to chmod_R | FilePermissions.rb:57:3:57:29 | call to chmod_R | FilePermissions.rb:57:16:57:19 | 0755 | 0755 |

From 8a3ffb6dca5f63f15e85844e60686ff8847b0bd6 Mon Sep 17 00:00:00 2001
From: Alex Ford <alexrford@github.com>
Date: Fri, 4 Jun 2021 13:25:03 +0100
Subject: [PATCH 2/2] add missing toString

---
 .../security/cwe-732/WeakFilePermissions.ql   |  2 +-
 .../cwe-732/WeakFilePermissions.expected      | 22 +++++++++----------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
index c001ce36bb9..4f6b5517611 100644
--- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
+++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql
@@ -98,4 +98,4 @@ from
   PermissionArgument arg
 where conf.hasFlowPath(source, sink) and arg = sink.getNode().asExpr().getExpr()
 select source.getNode(), source, sink, "Overly permissive mask in $@ sets file to $@.",
-  arg.getCall(), arg.getCall(), source.getNode(), source.getNode().toString()
+  arg.getCall(), arg.getCall().toString(), source.getNode(), source.getNode().toString()
diff --git a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
index 30f3b9fddbe..2fc1b5e2df4 100644
--- a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
+++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected
@@ -17,14 +17,14 @@ nodes
 | 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 in $@ sets file to $@. | FilePermissions.rb:4:3:4:32 | call to chmod | FilePermissions.rb:4:3:4:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:5:3:5:32 | call to chmod | FilePermissions.rb:5:3:5:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:6:3:6:32 | call to chmod | FilePermissions.rb:6:3:6:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:7:3:7:32 | call to chmod | FilePermissions.rb:7:3:7:32 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:24:3:24:26 | call to chmod | FilePermissions.rb:24:3:24:26 | call to chmod | FilePermissions.rb:24:13:24:16 | 0755 | 0755 |
-| FilePermissions.rb:43:10:43:13 | 0777 | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:44:19:44:22 | perm | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:44:3:44:32 | call to chmod | FilePermissions.rb:44:3:44:32 | call to chmod | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
-| FilePermissions.rb:43:10:43:13 | 0777 | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:46:19:46:23 | perm2 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:46:3:46:33 | call to chmod | FilePermissions.rb:46:3:46:33 | call to chmod | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
-| FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" | FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" :  | FilePermissions.rb:50:19:50:23 | perm2 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:50:3:50:33 | call to chmod | FilePermissions.rb:50:3:50:33 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:51:3:51:39 | call to chmod | FilePermissions.rb:51:3:51:39 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:53:3:53:34 | call to chmod | FilePermissions.rb:53:3:53:34 | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:57:3:57:29 | call to chmod_R | FilePermissions.rb:57:3:57:29 | call to chmod_R | FilePermissions.rb:57:16:57:19 | 0755 | 0755 |
+| FilePermissions.rb:4:19:4:22 | 0222 | FilePermissions.rb:4:19:4:22 | 0222 | FilePermissions.rb:4:19:4:22 | 0222 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:4:3:4:32 | call to chmod | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:5:3:5:32 | call to chmod | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:6:3:6:32 | call to chmod | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:7:3:7:32 | call to chmod | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:24:3:24:26 | call to chmod | call to chmod | FilePermissions.rb:24:13:24:16 | 0755 | 0755 |
+| FilePermissions.rb:43:10:43:13 | 0777 | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:44:19:44:22 | perm | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:44:3:44:32 | call to chmod | call to chmod | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
+| FilePermissions.rb:43:10:43:13 | 0777 | FilePermissions.rb:43:10:43:13 | 0777 :  | FilePermissions.rb:46:19:46:23 | perm2 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:46:3:46:33 | call to chmod | call to chmod | FilePermissions.rb:43:10:43:13 | 0777 | 0777 |
+| FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" | FilePermissions.rb:48:10:48:26 | "u=wrx,g=rwx,o=x" :  | FilePermissions.rb:50:19:50:23 | perm2 | Overly permissive mask in $@ sets file to $@. | FilePermissions.rb:50:3:50:33 | call to chmod | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:51:3:51:39 | call to chmod | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:53:3:53:34 | call to chmod | call to chmod | 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 in $@ sets file to $@. | FilePermissions.rb:57:3:57:29 | call to chmod_R | call to chmod_R | FilePermissions.rb:57:16:57:19 | 0755 | 0755 |