Skip to content

Commit 26a0167

Browse files
aibaarsaibaars
committed
Ruby: add taint step test for hash patterns
1 parent 49c4522 commit 26a0167

File tree

8 files changed

+111
-23
lines changed

8 files changed

+111
-23
lines changed

‎ruby/ql/lib/codeql/ruby/ast/Pattern.qll

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,10 @@ private class TPattern =
9797
* ```
9898
*/
9999
class CasePattern extends AstNode, TPattern {
100-
CasePattern() { casePattern(toGenerated(this)) }
100+
CasePattern() {
101+
casePattern(toGenerated(this)) or
102+
this = any(HashPattern p).getValue(_)
103+
}
101104
}
102105

103106
/**
@@ -267,7 +270,10 @@ class HashPattern extends CasePattern, THashPattern {
267270
StringlikeLiteral getKey(int n) { toGenerated(result) = this.keyValuePair(n).getKey() }
268271

269272
/** Gets the value of the `n`th pair. */
270-
CasePattern getValue(int n) { toGenerated(result) = this.keyValuePair(n).getValue() }
273+
AstNode getValue(int n) {
274+
toGenerated(result) = this.keyValuePair(n).getValue() or
275+
synthChild(this, n, result)
276+
}
271277

272278
/** Gets the value for a given key name. */
273279
CasePattern getValueByKey(string key) {

‎ruby/ql/lib/codeql/ruby/ast/Variable.qll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,8 @@ class VariableAccess extends Expr instanceof VariableAccessImpl {
115115
implicitWriteAccess(toGenerated(this))
116116
or
117117
this = any(SimpleParameterSynthImpl p).getDefininingAccess()
118+
or
119+
this = any(HashPattern p).getValue(_)
118120
}
119121

120122
final override string toString() { result = VariableAccessImpl.super.toString() }

‎ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ private import codeql.ruby.ast.internal.Expr
77
private import codeql.ruby.ast.internal.Variable
88
private import codeql.ruby.ast.internal.Pattern
99
private import codeql.ruby.ast.internal.Scope
10+
private import codeql.ruby.ast.internal.TreeSitter
1011
private import codeql.ruby.AST
1112

1213
/** A synthesized AST node kind. */
@@ -921,3 +922,35 @@ private module ForLoopDesugar {
921922
}
922923
}
923924
}
925+
926+
/**
927+
* ```rb
928+
* { a: }
929+
* ```
930+
* desugars to,
931+
* ```rb
932+
* { a: a }
933+
* ```
934+
*/
935+
private module ImplicitHashValueSynthesis {
936+
private Ruby::AstNode keyWithoutValue(HashPattern parent, int i) {
937+
exists(Ruby::KeywordPattern pair |
938+
result = pair.getKey() and
939+
result = toGenerated(parent.(HashPattern).getKey(i)) and
940+
not exists(pair.getValue())
941+
)
942+
}
943+
944+
private class ImplicitHashValueSynthesis extends Synthesis {
945+
final override predicate child(AstNode parent, int i, Child child) {
946+
exists(TVariableReal variable |
947+
access(keyWithoutValue(parent, i), variable) and
948+
child = SynthChild(LocalVariableAccessRealKind(variable))
949+
)
950+
}
951+
952+
final override predicate location(AstNode n, Location l) {
953+
exists(HashPattern p, int i | n = p.getValue(i) and l = keyWithoutValue(p, i).getLocation())
954+
}
955+
}
956+
}

‎ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ predicate implicitAssignmentNode(Ruby::AstNode n) {
3939
or
4040
n = any(Ruby::HashPattern parent).getChild(_).(Ruby::HashSplatParameter).getName()
4141
or
42+
n = any(Ruby::KeywordPattern parent | not exists(parent.getValue())).getKey()
43+
or
4244
n = any(Ruby::ExceptionVariable ev).getChild()
4345
or
4446
n = any(Ruby::For for).getPattern()
@@ -104,10 +106,19 @@ private predicate scopeDefinesParameterVariable(
104106
)
105107
}
106108

109+
private string variableName(Ruby::AstNode i) {
110+
result = i.(Ruby::Identifier).getValue()
111+
or
112+
exists(Ruby::KeywordPattern p | i = p.getKey() and not exists(p.getValue()) |
113+
result = i.(Ruby::String).getChild(0).(Ruby::StringContent).getValue() or
114+
result = i.(Ruby::HashKeySymbol).getValue()
115+
)
116+
}
117+
107118
/** Holds if `name` is assigned in `scope` at `i`. */
108-
private predicate scopeAssigns(Scope::Range scope, string name, Ruby::Identifier i) {
119+
private predicate scopeAssigns(Scope::Range scope, string name, Ruby::AstNode i) {
109120
(explicitAssignmentNode(i, _) or implicitAssignmentNode(i)) and
110-
name = i.getValue() and
121+
name = variableName(i) and
111122
scope = scopeOf(i)
112123
}
113124

@@ -132,11 +143,11 @@ private module Cached {
132143
other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
133144
)
134145
} or
135-
TLocalVariableReal(Scope::Range scope, string name, Ruby::Identifier i) {
146+
TLocalVariableReal(Scope::Range scope, string name, Ruby::AstNode i) {
136147
scopeDefinesParameterVariable(scope, name, i)
137148
or
138149
i =
139-
min(Ruby::Identifier other |
150+
min(Ruby::AstNode other |
140151
scopeAssigns(scope, name, other)
141152
|
142153
other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
@@ -296,10 +307,10 @@ private module Cached {
296307
}
297308

298309
cached
299-
predicate access(Ruby::Identifier access, VariableReal variable) {
310+
predicate access(Ruby::AstNode access, VariableReal variable) {
300311
exists(string name |
301312
variable.getNameImpl() = name and
302-
name = access.getValue()
313+
name = variableName(access)
303314
|
304315
variable.getDeclaringScopeImpl() = scopeOf(access) and
305316
not access.getLocation().strictlyBefore(variable.getLocationImpl()) and
@@ -371,7 +382,7 @@ private predicate inherits(Scope::Range scope, string name, Scope::Range outer)
371382
(
372383
scopeDefinesParameterVariable(outer, name, _)
373384
or
374-
exists(Ruby::Identifier i |
385+
exists(Ruby::AstNode i |
375386
scopeAssigns(outer, name, i) and
376387
i.getLocation().strictlyBefore(scope.getLocation())
377388
)
@@ -420,7 +431,7 @@ private class VariableRealAdapter extends VariableImpl, TVariableReal instanceof
420431
class LocalVariableReal extends VariableReal, TLocalVariableReal {
421432
private Scope::Range scope;
422433
private string name;
423-
private Ruby::Identifier i;
434+
private Ruby::AstNode i;
424435

425436
LocalVariableReal() { this = TLocalVariableReal(scope, name, i) }
426437

‎ruby/ql/test/library-tests/dataflow/local/DataflowStep.expected

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -70,13 +70,16 @@
7070
| local_dataflow.rb:50:18:50:18 | [post] x | local_dataflow.rb:51:20:51:20 | x |
7171
| local_dataflow.rb:50:18:50:18 | x | local_dataflow.rb:51:20:51:20 | x |
7272
| local_dataflow.rb:51:9:51:15 | "break" | local_dataflow.rb:51:3:51:15 | break |
73-
| local_dataflow.rb:60:1:86:3 | self (test_case) | local_dataflow.rb:78:12:78:20 | self |
74-
| local_dataflow.rb:60:1:86:3 | self in test_case | local_dataflow.rb:78:12:78:20 | self |
75-
| local_dataflow.rb:60:1:86:3 | self in test_case | local_dataflow.rb:79:18:79:24 | self |
76-
| local_dataflow.rb:60:1:86:3 | self in test_case | local_dataflow.rb:80:22:80:28 | self |
77-
| local_dataflow.rb:60:1:86:3 | self in test_case | local_dataflow.rb:82:6:82:12 | self |
78-
| local_dataflow.rb:60:1:86:3 | self in test_case | local_dataflow.rb:83:6:83:12 | self |
79-
| local_dataflow.rb:60:1:86:3 | self in test_case | local_dataflow.rb:84:6:84:12 | self |
73+
| local_dataflow.rb:60:1:89:3 | self (test_case) | local_dataflow.rb:78:12:78:20 | self |
74+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:78:12:78:20 | self |
75+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:79:18:79:24 | self |
76+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:80:22:80:28 | self |
77+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:82:6:82:12 | self |
78+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:83:6:83:12 | self |
79+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:84:6:84:12 | self |
80+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:85:20:85:26 | self |
81+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:86:26:86:32 | self |
82+
| local_dataflow.rb:60:1:89:3 | self in test_case | local_dataflow.rb:87:18:87:24 | self |
8083
| local_dataflow.rb:60:15:60:15 | x | local_dataflow.rb:60:15:60:15 | x |
8184
| local_dataflow.rb:60:15:60:15 | x | local_dataflow.rb:61:12:61:12 | x |
8285
| local_dataflow.rb:61:7:68:5 | case ... | local_dataflow.rb:61:3:68:5 | ... = ... |
@@ -107,27 +110,42 @@
107110
| local_dataflow.rb:73:7:73:7 | x | local_dataflow.rb:72:7:73:7 | then ... |
108111
| local_dataflow.rb:74:3:75:6 | else ... | local_dataflow.rb:69:7:76:5 | case ... |
109112
| local_dataflow.rb:75:6:75:6 | x | local_dataflow.rb:74:3:75:6 | else ... |
110-
| local_dataflow.rb:78:7:85:5 | case ... | local_dataflow.rb:78:3:85:5 | ... = ... |
113+
| local_dataflow.rb:78:7:88:3 | case ... | local_dataflow.rb:78:3:88:3 | ... = ... |
111114
| local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:79:18:79:24 | self |
112115
| local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:80:22:80:28 | self |
113116
| local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:82:6:82:12 | self |
117+
| local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:85:20:85:26 | self |
118+
| local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:86:26:86:32 | self |
119+
| local_dataflow.rb:78:12:78:20 | [post] self | local_dataflow.rb:87:18:87:24 | self |
114120
| local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:79:18:79:24 | self |
115121
| local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:80:22:80:28 | self |
116122
| local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:82:6:82:12 | self |
123+
| local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:85:20:85:26 | self |
124+
| local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:86:26:86:32 | self |
125+
| local_dataflow.rb:78:12:78:20 | self | local_dataflow.rb:87:18:87:24 | self |
117126
| local_dataflow.rb:79:11:79:11 | b | local_dataflow.rb:79:23:79:23 | b |
118-
| local_dataflow.rb:79:13:79:43 | then ... | local_dataflow.rb:78:7:85:5 | case ... |
127+
| local_dataflow.rb:79:13:79:43 | then ... | local_dataflow.rb:78:7:88:3 | case ... |
119128
| local_dataflow.rb:79:18:79:24 | call to sink | local_dataflow.rb:79:13:79:43 | then ... |
120129
| local_dataflow.rb:80:6:80:6 | a | local_dataflow.rb:80:11:80:11 | a |
121130
| local_dataflow.rb:80:11:80:11 | [post] a | local_dataflow.rb:80:27:80:27 | a |
122131
| local_dataflow.rb:80:11:80:11 | a | local_dataflow.rb:80:27:80:27 | a |
123-
| local_dataflow.rb:80:17:80:47 | then ... | local_dataflow.rb:78:7:85:5 | case ... |
132+
| local_dataflow.rb:80:17:80:47 | then ... | local_dataflow.rb:78:7:88:3 | case ... |
124133
| local_dataflow.rb:80:22:80:28 | call to sink | local_dataflow.rb:80:17:80:47 | then ... |
125134
| local_dataflow.rb:81:7:81:7 | c | local_dataflow.rb:82:11:82:11 | c |
126135
| local_dataflow.rb:81:11:81:11 | d | local_dataflow.rb:83:11:83:11 | d |
127136
| local_dataflow.rb:81:14:81:14 | e | local_dataflow.rb:84:11:84:11 | e |
128-
| local_dataflow.rb:81:18:84:32 | then ... | local_dataflow.rb:78:7:85:5 | case ... |
137+
| local_dataflow.rb:81:18:84:32 | then ... | local_dataflow.rb:78:7:88:3 | case ... |
129138
| local_dataflow.rb:81:23:84:13 | call to [] | local_dataflow.rb:81:18:84:32 | then ... |
130139
| local_dataflow.rb:82:6:82:12 | [post] self | local_dataflow.rb:83:6:83:12 | self |
131140
| local_dataflow.rb:82:6:82:12 | self | local_dataflow.rb:83:6:83:12 | self |
132141
| local_dataflow.rb:83:6:83:12 | [post] self | local_dataflow.rb:84:6:84:12 | self |
133142
| local_dataflow.rb:83:6:83:12 | self | local_dataflow.rb:84:6:84:12 | self |
143+
| local_dataflow.rb:85:11:85:11 | f | local_dataflow.rb:85:25:85:25 | f |
144+
| local_dataflow.rb:85:15:85:45 | then ... | local_dataflow.rb:78:7:88:3 | case ... |
145+
| local_dataflow.rb:85:20:85:26 | call to sink | local_dataflow.rb:85:15:85:45 | then ... |
146+
| local_dataflow.rb:86:16:86:16 | g | local_dataflow.rb:86:31:86:31 | g |
147+
| local_dataflow.rb:86:21:86:51 | then ... | local_dataflow.rb:78:7:88:3 | case ... |
148+
| local_dataflow.rb:86:26:86:32 | call to sink | local_dataflow.rb:86:21:86:51 | then ... |
149+
| local_dataflow.rb:87:8:87:8 | x | local_dataflow.rb:87:23:87:23 | x |
150+
| local_dataflow.rb:87:13:87:43 | then ... | local_dataflow.rb:78:7:88:3 | case ... |
151+
| local_dataflow.rb:87:18:87:24 | call to sink | local_dataflow.rb:87:13:87:43 | then ... |

‎ruby/ql/test/library-tests/dataflow/local/Nodes.expected

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ ret
1212
| local_dataflow.rb:50:3:50:13 | next |
1313
| local_dataflow.rb:51:3:51:15 | break |
1414
| local_dataflow.rb:52:3:52:10 | "normal" |
15-
| local_dataflow.rb:78:3:85:5 | ... = ... |
15+
| local_dataflow.rb:78:3:88:3 | ... = ... |
1616
arg
1717
| local_dataflow.rb:3:8:3:10 | self | local_dataflow.rb:3:8:3:10 | call to p | self |
1818
| local_dataflow.rb:3:10:3:10 | a | local_dataflow.rb:3:8:3:10 | call to p | position 0 |
@@ -67,3 +67,9 @@ arg
6767
| local_dataflow.rb:84:6:84:12 | call to sink | local_dataflow.rb:81:23:84:13 | call to [] | position 2 |
6868
| local_dataflow.rb:84:6:84:12 | self | local_dataflow.rb:84:6:84:12 | call to sink | self |
6969
| local_dataflow.rb:84:11:84:11 | e | local_dataflow.rb:84:6:84:12 | call to sink | position 0 |
70+
| local_dataflow.rb:85:20:85:26 | self | local_dataflow.rb:85:20:85:26 | call to sink | self |
71+
| local_dataflow.rb:85:25:85:25 | f | local_dataflow.rb:85:20:85:26 | call to sink | position 0 |
72+
| local_dataflow.rb:86:26:86:32 | self | local_dataflow.rb:86:26:86:32 | call to sink | self |
73+
| local_dataflow.rb:86:31:86:31 | g | local_dataflow.rb:86:26:86:32 | call to sink | position 0 |
74+
| local_dataflow.rb:87:18:87:24 | self | local_dataflow.rb:87:18:87:24 | call to sink | self |
75+
| local_dataflow.rb:87:23:87:23 | x | local_dataflow.rb:87:18:87:24 | call to sink | position 0 |

‎ruby/ql/test/library-tests/dataflow/local/TaintflowStep.expected

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,26 @@ edges
55
| local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:82:11:82:11 | c |
66
| local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:83:11:83:11 | d |
77
| local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:84:11:84:11 | e |
8+
| local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:85:25:85:25 | f |
9+
| local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:86:31:86:31 | g |
10+
| local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:87:23:87:23 | x |
811
nodes
912
| local_dataflow.rb:78:12:78:20 | call to source : | semmle.label | call to source : |
1013
| local_dataflow.rb:79:23:79:23 | b | semmle.label | b |
1114
| local_dataflow.rb:80:27:80:27 | a | semmle.label | a |
1215
| local_dataflow.rb:82:11:82:11 | c | semmle.label | c |
1316
| local_dataflow.rb:83:11:83:11 | d | semmle.label | d |
1417
| local_dataflow.rb:84:11:84:11 | e | semmle.label | e |
18+
| local_dataflow.rb:85:25:85:25 | f | semmle.label | f |
19+
| local_dataflow.rb:86:31:86:31 | g | semmle.label | g |
20+
| local_dataflow.rb:87:23:87:23 | x | semmle.label | x |
1521
subpaths
1622
#select
1723
| local_dataflow.rb:79:23:79:23 | b | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:79:23:79:23 | b | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |
1824
| local_dataflow.rb:80:27:80:27 | a | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:80:27:80:27 | a | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |
1925
| local_dataflow.rb:82:11:82:11 | c | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:82:11:82:11 | c | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |
2026
| local_dataflow.rb:83:11:83:11 | d | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:83:11:83:11 | d | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |
2127
| local_dataflow.rb:84:11:84:11 | e | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:84:11:84:11 | e | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |
28+
| local_dataflow.rb:85:25:85:25 | f | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:85:25:85:25 | f | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |
29+
| local_dataflow.rb:86:31:86:31 | g | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:86:31:86:31 | g | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |
30+
| local_dataflow.rb:87:23:87:23 | x | local_dataflow.rb:78:12:78:20 | call to source : | local_dataflow.rb:87:23:87:23 | x | $@ | local_dataflow.rb:78:12:78:20 | call to source : | call to source : |

‎ruby/ql/test/library-tests/dataflow/local/local_dataflow.rb

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,9 @@ def test_case x
8282
sink(c), # $ hasTaintFlow=1
8383
sink(d), # $ hasTaintFlow=1
8484
sink(e)] # $ hasTaintFlow=1
85-
end
85+
in { a: f } then sink(f) # $ hasTaintFlow=1
86+
in { foo: 1, g: } then sink(g) # $ hasTaintFlow=1
87+
in { x: } then sink(x) # $ hasTaintFlow=1
88+
end
8689
end
8790

0 commit comments

Comments
 (0)