From e5862a942ffbb1ab29b5b24e94a8503a3e7610f3 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 22 Apr 2021 19:59:04 +0100 Subject: [PATCH 01/17] WIP rb/overly-permissive-file query --- .../security/cwe-732/WeakFilePermissions.ql | 79 +++++++++++++++++++ .../security/cwe-732/FilePermissions.rb | 58 ++++++++++++++ .../cwe-732/WeakFilePermissions.expected | 11 +++ .../cwe-732/WeakFilePermissions.qlref | 1 + 4 files changed, 149 insertions(+) create mode 100644 ql/src/queries/security/cwe-732/WeakFilePermissions.ql create mode 100644 ql/test/query-tests/security/cwe-732/FilePermissions.rb create mode 100644 ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected create mode 100644 ql/test/query-tests/security/cwe-732/WeakFilePermissions.qlref diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql new file mode 100644 index 00000000000..aed1f9f6fa2 --- /dev/null +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -0,0 +1,79 @@ +/** + * @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 problem + * @problem.severity warning + * @id rb/overly-permissive-file + * @tags external/cwe/cwe-732 + * security + * @precision low + */ + +import ruby +private import codeql_ruby.dataflow.SSA + +// TODO: account for flows through tuple assignments +// TODO: full dataflow? +/** 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 + ) + } +} + +/** An expression specifing a file permission that allows group/others read or write access */ +class PermissivePermissionsExpr extends Expr { + PermissivePermissionsExpr() { + this.(IntegerLiteral).getValueText().regexpMatch("0[0-7](([2-7].)|.[2-7])") + or + this.(IntegerLiteral) + .getValueText() + .regexpMatch("0b[01]{3}+((1[01]{5}+)|([01]1[01]{4}+)|([01]{3}+1[01]{2}+)|([01]{4}+1[01]))") + or + // TODO: non-literal expressions? underscores? decimal/hex literals? + // adding/setting read or write permissions for all/group/owner + this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=]*[rwxXst]*[rw].*") + or + exists(PermissivePermissionsExpr ppe, Ssa::WriteDefinition def | + def.getARead() = this.getAControlFlowNode() and + def.getWriteAccess().getParent().(Assignment).getRightOperand() = ppe + ) + } +} + +/** 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 } + + predicate isPermissive() { this.getPermissionArgument() instanceof PermissivePermissionsExpr } +} + +from PermissionSettingMethodCall c +where c.isPermissive() +select c, "Overly permissive mask sets file to " + c.getPermissionArgument() + "." diff --git a/ql/test/query-tests/security/cwe-732/FilePermissions.rb b/ql/test/query-tests/security/cwe-732/FilePermissions.rb new file mode 100644 index 00000000000..7edb567a8e6 --- /dev/null +++ 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 diff --git a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected new file mode 100644 index 00000000000..9ef91fc15bd --- /dev/null +++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected @@ -0,0 +1,11 @@ +| FilePermissions.rb:4:3:4:32 | call to chmod | Overly permissive mask sets file to 0222. | +| FilePermissions.rb:5:3:5:32 | call to chmod | Overly permissive mask sets file to 0622. | +| FilePermissions.rb:6:3:6:32 | call to chmod | Overly permissive mask sets file to 0755. | +| FilePermissions.rb:7:3:7:32 | call to chmod | Overly permissive mask sets file to 0777. | +| FilePermissions.rb:24:3:24:26 | call to chmod | Overly permissive mask sets file to 0755. | +| FilePermissions.rb:44:3:44:32 | call to chmod | Overly permissive mask sets file to perm. | +| FilePermissions.rb:46:3:46:33 | call to chmod | Overly permissive mask sets file to perm2. | +| FilePermissions.rb:50:3:50:33 | call to chmod | Overly permissive mask sets file to perm2. | +| FilePermissions.rb:51:3:51:39 | call to chmod | Overly permissive mask sets file to "u=rwx,o+r". | +| FilePermissions.rb:53:3:53:34 | call to chmod | Overly permissive mask sets file to "a+rw". | +| FilePermissions.rb:57:3:57:29 | call to chmod_R | Overly permissive mask sets file to 0755. | diff --git a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.qlref b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.qlref new file mode 100644 index 00000000000..bf19b31509d --- /dev/null +++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.qlref @@ -0,0 +1 @@ +queries/security/cwe-732/WeakFilePermissions.ql From e3d393b7c10011a52f96374b9863f048fca7f13b Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 28 Apr 2021 15:40:58 +0100 Subject: [PATCH 02/17] use full dataflow for permission args in rb/overly-permissive-file --- .../security/cwe-732/WeakFilePermissions.ql | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index aed1f9f6fa2..0ac0b518aa6 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -1,7 +1,7 @@ /** * @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 problem + * @kind path-problem * @problem.severity warning * @id rb/overly-permissive-file * @tags external/cwe/cwe-732 @@ -11,9 +11,10 @@ import ruby private import codeql_ruby.dataflow.SSA +private import codeql_ruby.dataflow.internal.DataFlowImpl as DataFlow // TODO: account for flows through tuple assignments -// TODO: full dataflow? + /** An expression referencing the File or FileUtils module */ class FileModuleAccess extends Expr { FileModuleAccess() { @@ -40,11 +41,6 @@ class PermissivePermissionsExpr extends Expr { // TODO: non-literal expressions? underscores? decimal/hex literals? // adding/setting read or write permissions for all/group/owner this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=]*[rwxXst]*[rw].*") - or - exists(PermissivePermissionsExpr ppe, Ssa::WriteDefinition def | - def.getARead() = this.getAControlFlowNode() and - def.getWriteAccess().getParent().(Assignment).getRightOperand() = ppe - ) } } @@ -70,10 +66,21 @@ class PermissionSettingMethodCall extends MethodCall { } Expr getPermissionArgument() { result = permArg } - - predicate isPermissive() { this.getPermissionArgument() instanceof PermissivePermissionsExpr } } -from PermissionSettingMethodCall c -where c.isPermissive() -select c, "Overly permissive mask sets file to " + c.getPermissionArgument() + "." +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() \ No newline at end of file From 7a72d8ec2f39f6a72512a0cf5106b8b9e195da4b Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 28 Apr 2021 15:51:08 +0100 Subject: [PATCH 03/17] add qhelp for rb/overly-permissive-file --- .../cwe-732/WeakFilePermissions.qhelp | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 ql/src/queries/security/cwe-732/WeakFilePermissions.qhelp diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.qhelp b/ql/src/queries/security/cwe-732/WeakFilePermissions.qhelp new file mode 100644 index 00000000000..a5f8997e911 --- /dev/null +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.qhelp @@ -0,0 +1,26 @@ + + + + +

+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. +

+ + + + +

+Restrict the file permissions of files to prevent any but the owner being able to read or write to that file +

+ + + +
  • +Wikipedia: +File system permissions. +
    • +     + +     From 0a6dc6f15003cda44eb15eb5588be10c33c5244d Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Wed, 28 Apr 2021 16:31:07 +0100 Subject: [PATCH 04/17] update WeakFilePermissions.expected --- .../cwe-732/WeakFilePermissions.expected | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected index 9ef91fc15bd..1b5d49b2d28 100644 --- a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected +++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected @@ -1,11 +1,11 @@ -| FilePermissions.rb:4:3:4:32 | call to chmod | Overly permissive mask sets file to 0222. | -| FilePermissions.rb:5:3:5:32 | call to chmod | Overly permissive mask sets file to 0622. | -| FilePermissions.rb:6:3:6:32 | call to chmod | Overly permissive mask sets file to 0755. | -| FilePermissions.rb:7:3:7:32 | call to chmod | Overly permissive mask sets file to 0777. | -| FilePermissions.rb:24:3:24:26 | call to chmod | Overly permissive mask sets file to 0755. | -| FilePermissions.rb:44:3:44:32 | call to chmod | Overly permissive mask sets file to perm. | -| FilePermissions.rb:46:3:46:33 | call to chmod | Overly permissive mask sets file to perm2. | -| FilePermissions.rb:50:3:50:33 | call to chmod | Overly permissive mask sets file to perm2. | -| FilePermissions.rb:51:3:51:39 | call to chmod | Overly permissive mask sets file to "u=rwx,o+r". | -| FilePermissions.rb:53:3:53:34 | call to chmod | Overly permissive mask sets file to "a+rw". | -| FilePermissions.rb:57:3:57:29 | call to chmod_R | Overly permissive mask sets file to 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 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 | From 2c0fc7d19378f60d1e137d8ef3ee5eea2ad670de Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 15:34:10 +0100 Subject: [PATCH 05/17] parse integer permission args as ints instead of using regex matches --- .../security/cwe-732/WeakFilePermissions.ql | 53 +++++++++++++++---- 1 file changed, 43 insertions(+), 10 deletions(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index 0ac0b518aa6..d367bede0b2 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -14,7 +14,6 @@ private import codeql_ruby.dataflow.SSA private import codeql_ruby.dataflow.internal.DataFlowImpl as DataFlow // TODO: account for flows through tuple assignments - /** An expression referencing the File or FileUtils module */ class FileModuleAccess extends Expr { FileModuleAccess() { @@ -29,18 +28,52 @@ class FileModuleAccess extends Expr { } } +bindingset[p] +int world_permission(int p) { result = p % 8 } + +bindingset[p] +int group_permission(int p) { result = (p / 8) % 8 } + +bindingset[p] +string access(int p) { + p % 4 >= 2 and result = "writable" + or + p % 8 in [4, 5] and result = "readable" +} + +bindingset[s] +int parseInt(string s) { + exists(string values, string str | + 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"] and values = "01234567" and str = s.suffix(1) + 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) + ) + ) +} + /** An expression specifing a file permission that allows group/others read or write access */ class PermissivePermissionsExpr extends Expr { + // TODO: non-literal expressions? PermissivePermissionsExpr() { - this.(IntegerLiteral).getValueText().regexpMatch("0[0-7](([2-7].)|.[2-7])") + exists(int perm, string acc | + perm = parseInt(this.(IntegerLiteral).getValueText()) and + (acc = access(world_permission(perm)) or acc = access(group_permission(perm))) + ) or - this.(IntegerLiteral) - .getValueText() - .regexpMatch("0b[01]{3}+((1[01]{5}+)|([01]1[01]{4}+)|([01]{3}+1[01]{2}+)|([01]{4}+1[01]))") - or - // TODO: non-literal expressions? underscores? decimal/hex literals? // adding/setting read or write permissions for all/group/owner - this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=]*[rwxXst]*[rw].*") + this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=]*[xXst]*[rw].*") } } @@ -82,5 +115,5 @@ class PermissivePermissionsConfig extends DataFlow::Configuration { 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() \ No newline at end of file +select sink.getNode(), source, sink, "Overly permissive mask sets file to $@.", source.getNode(), + source.getNode().toString() From 1c89bbe18830ed9c9ffc39db2160be7d317f584e Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 15:44:54 +0100 Subject: [PATCH 06/17] fix select format of rb/overly-permissive-file --- ql/src/queries/security/cwe-732/WeakFilePermissions.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index d367bede0b2..cf8ba03b786 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -115,5 +115,5 @@ class PermissivePermissionsConfig extends DataFlow::Configuration { 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(), +select sink, source, sink, "Overly permissive mask sets file to $@.", source.getNode(), source.getNode().toString() From 46a14b282606aec39b5903368b1f78144271a9c2 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 15:54:22 +0100 Subject: [PATCH 07/17] move parseInt logic into getValue method predicate on IntegerLiteral --- ql/src/codeql_ruby/ast/Literal.qll | 27 +++++++++++++++++++ .../security/cwe-732/WeakFilePermissions.ql | 24 +---------------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Literal.qll b/ql/src/codeql_ruby/ast/Literal.qll index ce0825cce92..615b0b16210 100644 --- a/ql/src/codeql_ruby/ast/Literal.qll +++ b/ql/src/codeql_ruby/ast/Literal.qll @@ -47,6 +47,33 @@ class IntegerLiteral extends NumericLiteral, TIntegerLiteral { final override string getValueText() { result = g.getValue() } + final int getValue() { + exists(string s, string values, string str | + s = this.getValueText() 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"] and + values = "01234567" and + str = s.suffix(1) + 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" } diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index cf8ba03b786..6296ca2dd84 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -41,34 +41,12 @@ string access(int p) { p % 8 in [4, 5] and result = "readable" } -bindingset[s] -int parseInt(string s) { - exists(string values, string str | - 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"] and values = "01234567" and str = s.suffix(1) - 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) - ) - ) -} - /** 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 = parseInt(this.(IntegerLiteral).getValueText()) and + perm = this.(IntegerLiteral).getValue() and (acc = access(world_permission(perm)) or acc = access(group_permission(perm))) ) or From efa323c3045d97e0f09070a959a0841d6bc959d6 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 16:08:42 +0100 Subject: [PATCH 08/17] rb/overly-permissive-file use QL bitwise operators --- ql/src/queries/security/cwe-732/WeakFilePermissions.ql | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index 6296ca2dd84..c6d1760dc13 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -29,16 +29,17 @@ class FileModuleAccess extends Expr { } bindingset[p] -int world_permission(int p) { result = p % 8 } +int world_permission(int p) { result = p.bitAnd(7) } bindingset[p] -int group_permission(int p) { result = (p / 8) % 8 } +// 70 oct = 56 dec +int group_permission(int p) { result = p.bitAnd(56) } bindingset[p] string access(int p) { - p % 4 >= 2 and result = "writable" + p.bitAnd(2) != 0 and result = "writable" or - p % 8 in [4, 5] and result = "readable" + p.bitAnd(2) = 0 and p.bitAnd(4) != 0 and result = "readable" } /** An expression specifing a file permission that allows group/others read or write access */ From 35d5bae10ea5bf1cf2e7daf8b087c574833e1cb8 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 16:16:09 +0100 Subject: [PATCH 09/17] run formatter --- ql/src/queries/security/cwe-732/WeakFilePermissions.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index c6d1760dc13..c86d3459f0c 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -31,8 +31,8 @@ class FileModuleAccess extends Expr { bindingset[p] int world_permission(int p) { result = p.bitAnd(7) } -bindingset[p] // 70 oct = 56 dec +bindingset[p] int group_permission(int p) { result = p.bitAnd(56) } bindingset[p] From 05adfec03d4423fa4be98a5c9bb789409967f60b Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 17:02:54 +0100 Subject: [PATCH 10/17] account for more patterns in IntegerLiteral.getValue --- ql/src/codeql_ruby/ast/Literal.qll | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Literal.qll b/ql/src/codeql_ruby/ast/Literal.qll index 615b0b16210..a169c4f3780 100644 --- a/ql/src/codeql_ruby/ast/Literal.qll +++ b/ql/src/codeql_ruby/ast/Literal.qll @@ -51,15 +51,23 @@ class IntegerLiteral extends NumericLiteral, TIntegerLiteral { exists(string s, string values, string str | s = this.getValueText() and ( - s.matches("0b%") and values = "01" and str = s.suffix(2) + (s.matches("0b%") or s.matches("0B%")) and + values = "01" and + str = s.suffix(2) or - s.matches("0x%") and values = "0123456789abcdef" and str = s.suffix(2) + (s.matches("0x%") or s.matches("0X%")) and + values = "0123456789abcdef" and + str = s.suffix(2) or s.charAt(0) = "0" and - not s.charAt(1) = ["b", "x"] and + not s.charAt(1) = ["b", "B", "x", "X"] and values = "01234567" and str = s.suffix(1) or + (s.matches("0o%") or s.matches("0O%")) and + values = "01234567" and + str = s.suffix(2) + or s.charAt(0) != "0" and values = "0123456789" and str = s ) | From 43754528668aafaa2f6dddaa06fdf8fbed6d3cd8 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 17:08:33 +0100 Subject: [PATCH 11/17] more IntegerLiteral.getValue improvements --- ql/src/codeql_ruby/ast/Literal.qll | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Literal.qll b/ql/src/codeql_ruby/ast/Literal.qll index a169c4f3780..fbf06f0e8cb 100644 --- a/ql/src/codeql_ruby/ast/Literal.qll +++ b/ql/src/codeql_ruby/ast/Literal.qll @@ -49,22 +49,22 @@ class IntegerLiteral extends NumericLiteral, TIntegerLiteral { final int getValue() { exists(string s, string values, string str | - s = this.getValueText() and + s = this.getValueText().toLowerCase() and ( - (s.matches("0b%") or s.matches("0B%")) and + s.matches("0b%") and values = "01" and str = s.suffix(2) or - (s.matches("0x%") or s.matches("0X%")) and + s.matches("0x%") and values = "0123456789abcdef" and str = s.suffix(2) or s.charAt(0) = "0" and - not s.charAt(1) = ["b", "B", "x", "X"] and + not s.charAt(1) = ["b", "x", "o"] and values = "01234567" and str = s.suffix(1) or - (s.matches("0o%") or s.matches("0O%")) and + s.matches("0o%") and values = "01234567" and str = s.suffix(2) or From 2c8a4f833fe252e42fba773be2077f53fc2226f6 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 29 Apr 2021 19:11:39 +0100 Subject: [PATCH 12/17] make rb/overly-permissive-file a proper path-problem --- .../security/cwe-732/WeakFilePermissions.ql | 3 ++- .../cwe-732/WeakFilePermissions.expected | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index c86d3459f0c..13460d22603 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -10,6 +10,7 @@ */ import ruby +import codeql_ruby.dataflow.internal.DataFlowImpl::PathGraph private import codeql_ruby.dataflow.SSA private import codeql_ruby.dataflow.internal.DataFlowImpl as DataFlow @@ -94,5 +95,5 @@ class PermissivePermissionsConfig extends DataFlow::Configuration { from DataFlow::PathNode source, DataFlow::PathNode sink, PermissivePermissionsConfig conf where conf.hasFlowPath(source, sink) -select sink, source, sink, "Overly permissive mask sets file to $@.", source.getNode(), +select sink.getNode(), source, sink, "Overly permissive mask sets file to $@.", 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 1b5d49b2d28..965851448ff 100644 --- a/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected +++ b/ql/test/query-tests/security/cwe-732/WeakFilePermissions.expected @@ -1,3 +1,22 @@ +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 | From 48add9ffbc9b4ffb4424f249d83ec21babf84172 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Mon, 10 May 2021 11:00:59 +0100 Subject: [PATCH 13/17] remove internal import in rb/overly-permissive-file --- ql/src/queries/security/cwe-732/WeakFilePermissions.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index 13460d22603..79fd3f8b663 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -10,9 +10,9 @@ */ import ruby -import codeql_ruby.dataflow.internal.DataFlowImpl::PathGraph +import codeql_ruby.DataFlow +import DataFlow::PathGraph private import codeql_ruby.dataflow.SSA -private import codeql_ruby.dataflow.internal.DataFlowImpl as DataFlow // TODO: account for flows through tuple assignments /** An expression referencing the File or FileUtils module */ From 2154b7df3000866913846e9c5989fbfe698aade3 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Mon, 10 May 2021 11:02:48 +0100 Subject: [PATCH 14/17] add doc for IntegerLiteral.getValue --- ql/src/codeql_ruby/ast/Literal.qll | 1 + 1 file changed, 1 insertion(+) diff --git a/ql/src/codeql_ruby/ast/Literal.qll b/ql/src/codeql_ruby/ast/Literal.qll index fbf06f0e8cb..6a6cfc9ab5c 100644 --- a/ql/src/codeql_ruby/ast/Literal.qll +++ b/ql/src/codeql_ruby/ast/Literal.qll @@ -47,6 +47,7 @@ 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 From 89be8d87107beb1d1b4e8d24485f30f5e9720a17 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 13 May 2021 12:59:16 +0100 Subject: [PATCH 15/17] Apply suggestions from code review Co-authored-by: Arthur Baars --- ql/src/queries/security/cwe-732/WeakFilePermissions.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index 79fd3f8b663..d75aa6831e7 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -52,8 +52,8 @@ class PermissivePermissionsExpr extends Expr { (acc = access(world_permission(perm)) or acc = access(group_permission(perm))) ) or - // adding/setting read or write permissions for all/group/owner - this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=]*[xXst]*[rw].*") + // adding/setting read or write permissions for all/group/other + this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=][xXst]*[rw].*") } } From 0d1c4a12907effdfb0b59fd96c3780fe0e125380 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 13 May 2021 13:06:45 +0100 Subject: [PATCH 16/17] document that the WeakFilePermissions access predicate should return at most one value --- ql/src/queries/security/cwe-732/WeakFilePermissions.ql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index d75aa6831e7..7d6916e667c 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -40,6 +40,8 @@ bindingset[p] string access(int p) { p.bitAnd(2) != 0 and result = "writable" or + // report only the "most permissive" permission, i.e. report the file as + // readable only if it is not also writable p.bitAnd(2) = 0 and p.bitAnd(4) != 0 and result = "readable" } From b2f2f786acecd88d68a18a453379b951c7ac2c68 Mon Sep 17 00:00:00 2001 From: Alex Ford Date: Thu, 13 May 2021 13:22:14 +0100 Subject: [PATCH 17/17] allow the WeakFilePermissions access predicate to return multiple values --- ql/src/queries/security/cwe-732/WeakFilePermissions.ql | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql index 7d6916e667c..41c0edceb16 100644 --- a/ql/src/queries/security/cwe-732/WeakFilePermissions.ql +++ b/ql/src/queries/security/cwe-732/WeakFilePermissions.ql @@ -40,9 +40,7 @@ bindingset[p] string access(int p) { p.bitAnd(2) != 0 and result = "writable" or - // report only the "most permissive" permission, i.e. report the file as - // readable only if it is not also writable - p.bitAnd(2) = 0 and p.bitAnd(4) != 0 and result = "readable" + p.bitAnd(4) != 0 and result = "readable" } /** An expression specifing a file permission that allows group/others read or write access */