Skip to content

Commit 2c0fc7d

Browse files
committed
parse integer permission args as ints instead of using regex matches
1 parent 0a6dc6f commit 2c0fc7d

1 file changed

Lines changed: 43 additions & 10 deletions

File tree

ql/src/queries/security/cwe-732/WeakFilePermissions.ql

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ private import codeql_ruby.dataflow.SSA
1414
private import codeql_ruby.dataflow.internal.DataFlowImpl as DataFlow
1515

1616
// TODO: account for flows through tuple assignments
17-
1817
/** An expression referencing the File or FileUtils module */
1918
class FileModuleAccess extends Expr {
2019
FileModuleAccess() {
@@ -29,18 +28,52 @@ class FileModuleAccess extends Expr {
2928
}
3029
}
3130

31+
bindingset[p]
32+
int world_permission(int p) { result = p % 8 }
33+
34+
bindingset[p]
35+
int group_permission(int p) { result = (p / 8) % 8 }
36+
37+
bindingset[p]
38+
string access(int p) {
39+
p % 4 >= 2 and result = "writable"
40+
or
41+
p % 8 in [4, 5] and result = "readable"
42+
}
43+
44+
bindingset[s]
45+
int parseInt(string s) {
46+
exists(string values, string str |
47+
s.matches("0b%") and values = "01" and str = s.suffix(2)
48+
or
49+
s.matches("0x%") and values = "0123456789abcdef" and str = s.suffix(2)
50+
or
51+
s.charAt(0) = "0" and not s.charAt(1) = ["b", "x"] and values = "01234567" and str = s.suffix(1)
52+
or
53+
s.charAt(0) != "0" and values = "0123456789" and str = s
54+
|
55+
result =
56+
sum(int index, string c, int v, int exp |
57+
c = str.replaceAll("_", "").charAt(index) and
58+
v = values.indexOf(c.toLowerCase()) and
59+
exp = str.replaceAll("_", "").length() - index - 1
60+
|
61+
v * values.length().pow(exp)
62+
)
63+
)
64+
}
65+
3266
/** An expression specifing a file permission that allows group/others read or write access */
3367
class PermissivePermissionsExpr extends Expr {
68+
// TODO: non-literal expressions?
3469
PermissivePermissionsExpr() {
35-
this.(IntegerLiteral).getValueText().regexpMatch("0[0-7](([2-7].)|.[2-7])")
36-
or
37-
this.(IntegerLiteral)
38-
.getValueText()
39-
.regexpMatch("0b[01]{3}+((1[01]{5}+)|([01]1[01]{4}+)|([01]{3}+1[01]{2}+)|([01]{4}+1[01]))")
70+
exists(int perm, string acc |
71+
perm = parseInt(this.(IntegerLiteral).getValueText()) and
72+
(acc = access(world_permission(perm)) or acc = access(group_permission(perm)))
73+
)
4074
or
41-
// TODO: non-literal expressions? underscores? decimal/hex literals?
4275
// adding/setting read or write permissions for all/group/owner
43-
this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=]*[rwxXst]*[rw].*")
76+
this.(StringLiteral).getValueText().regexpMatch(".*[ago][^-=+]*[+=]*[xXst]*[rw].*")
4477
}
4578
}
4679

@@ -82,5 +115,5 @@ class PermissivePermissionsConfig extends DataFlow::Configuration {
82115

83116
from DataFlow::PathNode source, DataFlow::PathNode sink, PermissivePermissionsConfig conf
84117
where conf.hasFlowPath(source, sink)
85-
select sink.getNode(), source, sink, "Overly permissive mask sets file to $@.",
86-
source.getNode(), source.getNode().toString()
118+
select sink.getNode(), source, sink, "Overly permissive mask sets file to $@.", source.getNode(),
119+
source.getNode().toString()

0 commit comments

Comments
 (0)