Skip to content

Commit c5e3be2

Browse files
felickzfelickz
committed
Grape - detect params calls inside helper methods
- added unit tests for flow using inline format - removed grape from Arel tests (temporary)
1 parent ffd32ef commit c5e3be2

File tree

7 files changed

+216
-134
lines changed

7 files changed

+216
-134
lines changed

‎ruby/ql/lib/codeql/ruby/frameworks/Grape.qll‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,11 +121,18 @@ class GrapeParamsSource extends Http::Server::RequestInputAccess::Range {
121121
*/
122122
private class GrapeParamsCall extends ParamsCallImpl {
123123
GrapeParamsCall() {
124-
// Simplified approach: find params calls that are descendants of Grape API class methods
124+
// Params calls within endpoint blocks
125125
exists(GrapeApiClass api |
126126
this.getMethodName() = "params" and
127127
this.getParent+() = api.getADeclaration()
128128
)
129+
or
130+
// Params calls within helper methods (defined in helpers blocks)
131+
exists(GrapeApiClass api, DataFlow::CallNode helpersCall |
132+
helpersCall = api.getAModuleLevelCall("helpers") and
133+
this.getMethodName() = "params" and
134+
this.getParent+() = helpersCall.getBlock().asExpr().getExpr()
135+
)
129136
}
130137
}
131138

@@ -295,18 +302,31 @@ private class GrapeHelperMethod extends Method {
295302

296303
/**
297304
* Additional taint step to model dataflow from method arguments to parameters
298-
* for Grape helper methods defined in `helpers` blocks.
305+
* and from return values back to call sites for Grape helper methods defined in `helpers` blocks.
299306
* This bridges the gap where standard dataflow doesn't recognize the Grape DSL semantics.
300307
*/
301308
private class GrapeHelperMethodTaintStep extends AdditionalTaintStep {
302309
override predicate step(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
310+
// Map arguments to parameters for helper method calls
303311
exists(GrapeHelperMethod helperMethod, MethodCall call, int i |
304-
// Find calls to helper methods from within Grape endpoints
312+
// Find calls to helper methods from within Grape endpoints or other helper methods
305313
call.getMethodName() = helperMethod.getName() and
306-
exists(GrapeEndpoint endpoint | call.getParent+() = endpoint.getBody().asExpr().getExpr()) and
314+
exists(GrapeApiClass api | call.getParent+() = api.getADeclaration()) and
307315
// Map argument to parameter
308316
nodeFrom.asExpr().getExpr() = call.getArgument(i) and
309317
nodeTo.asParameter() = helperMethod.getParameter(i)
310318
)
319+
or
320+
// Model implicit return values: the last expression in a helper method flows to the call site
321+
exists(GrapeHelperMethod helperMethod, MethodCall helperCall, Expr lastExpr |
322+
// Find calls to helper methods from within Grape endpoints or other helper methods
323+
helperCall.getMethodName() = helperMethod.getName() and
324+
exists(GrapeApiClass api | helperCall.getParent+() = api.getADeclaration()) and
325+
// Get the last expression in the helper method (Ruby's implicit return)
326+
lastExpr = helperMethod.getLastStmt() and
327+
// Flow from the last expression in the helper method to the call site
328+
nodeFrom.asExpr().getExpr() = lastExpr and
329+
nodeTo.asExpr().getExpr() = helperCall
330+
)
311331
}
312332
}

‎ruby/ql/test/library-tests/frameworks/grape/Flow.expected‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
models
2+
edges
3+
| app.rb:103:13:103:18 | call to params | app.rb:103:13:103:70 | call to select | provenance | |
4+
| app.rb:103:13:103:70 | call to select | app.rb:149:21:149:31 | call to user_params | provenance | AdditionalTaintStep |
5+
| app.rb:103:13:103:70 | call to select | app.rb:165:21:165:31 | call to user_params | provenance | AdditionalTaintStep |
6+
| app.rb:107:13:107:32 | call to source | app.rb:143:18:143:43 | call to vulnerable_helper | provenance | AdditionalTaintStep |
7+
| app.rb:111:13:111:33 | call to source | app.rb:150:25:150:37 | call to simple_helper | provenance | AdditionalTaintStep |
8+
| app.rb:126:9:126:15 | user_id | app.rb:133:14:133:20 | user_id | provenance | |
9+
| app.rb:126:19:126:24 | call to params | app.rb:126:19:126:34 | ...[...] | provenance | |
10+
| app.rb:126:19:126:34 | ...[...] | app.rb:126:9:126:15 | user_id | provenance | |
11+
| app.rb:127:9:127:16 | route_id | app.rb:134:14:134:21 | route_id | provenance | |
12+
| app.rb:127:20:127:40 | call to route_param | app.rb:127:9:127:16 | route_id | provenance | |
13+
| app.rb:128:9:128:12 | auth | app.rb:135:14:135:17 | auth | provenance | |
14+
| app.rb:128:16:128:22 | call to headers | app.rb:128:16:128:38 | ...[...] | provenance | |
15+
| app.rb:128:16:128:38 | ...[...] | app.rb:128:9:128:12 | auth | provenance | |
16+
| app.rb:129:9:129:15 | session | app.rb:136:14:136:20 | session | provenance | |
17+
| app.rb:129:19:129:25 | call to cookies | app.rb:129:19:129:38 | ...[...] | provenance | |
18+
| app.rb:129:19:129:38 | ...[...] | app.rb:129:9:129:15 | session | provenance | |
19+
| app.rb:143:9:143:14 | result | app.rb:144:14:144:19 | result | provenance | |
20+
| app.rb:143:18:143:43 | call to vulnerable_helper | app.rb:143:9:143:14 | result | provenance | |
21+
| app.rb:149:9:149:17 | user_data | app.rb:151:14:151:22 | user_data | provenance | |
22+
| app.rb:149:21:149:31 | call to user_params | app.rb:149:9:149:17 | user_data | provenance | |
23+
| app.rb:150:9:150:21 | simple_result | app.rb:152:14:152:26 | simple_result | provenance | |
24+
| app.rb:150:25:150:37 | call to simple_helper | app.rb:150:9:150:21 | simple_result | provenance | |
25+
| app.rb:159:13:159:19 | user_id | app.rb:160:18:160:24 | user_id | provenance | |
26+
| app.rb:159:23:159:28 | call to params | app.rb:159:23:159:33 | ...[...] | provenance | |
27+
| app.rb:159:23:159:33 | ...[...] | app.rb:159:13:159:19 | user_id | provenance | |
28+
| app.rb:165:9:165:17 | user_data | app.rb:166:14:166:22 | user_data | provenance | |
29+
| app.rb:165:21:165:31 | call to user_params | app.rb:165:9:165:17 | user_data | provenance | |
30+
nodes
31+
| app.rb:103:13:103:18 | call to params | semmle.label | call to params |
32+
| app.rb:103:13:103:70 | call to select | semmle.label | call to select |
33+
| app.rb:107:13:107:32 | call to source | semmle.label | call to source |
34+
| app.rb:111:13:111:33 | call to source | semmle.label | call to source |
35+
| app.rb:126:9:126:15 | user_id | semmle.label | user_id |
36+
| app.rb:126:19:126:24 | call to params | semmle.label | call to params |
37+
| app.rb:126:19:126:34 | ...[...] | semmle.label | ...[...] |
38+
| app.rb:127:9:127:16 | route_id | semmle.label | route_id |
39+
| app.rb:127:20:127:40 | call to route_param | semmle.label | call to route_param |
40+
| app.rb:128:9:128:12 | auth | semmle.label | auth |
41+
| app.rb:128:16:128:22 | call to headers | semmle.label | call to headers |
42+
| app.rb:128:16:128:38 | ...[...] | semmle.label | ...[...] |
43+
| app.rb:129:9:129:15 | session | semmle.label | session |
44+
| app.rb:129:19:129:25 | call to cookies | semmle.label | call to cookies |
45+
| app.rb:129:19:129:38 | ...[...] | semmle.label | ...[...] |
46+
| app.rb:133:14:133:20 | user_id | semmle.label | user_id |
47+
| app.rb:134:14:134:21 | route_id | semmle.label | route_id |
48+
| app.rb:135:14:135:17 | auth | semmle.label | auth |
49+
| app.rb:136:14:136:20 | session | semmle.label | session |
50+
| app.rb:143:9:143:14 | result | semmle.label | result |
51+
| app.rb:143:18:143:43 | call to vulnerable_helper | semmle.label | call to vulnerable_helper |
52+
| app.rb:144:14:144:19 | result | semmle.label | result |
53+
| app.rb:149:9:149:17 | user_data | semmle.label | user_data |
54+
| app.rb:149:21:149:31 | call to user_params | semmle.label | call to user_params |
55+
| app.rb:150:9:150:21 | simple_result | semmle.label | simple_result |
56+
| app.rb:150:25:150:37 | call to simple_helper | semmle.label | call to simple_helper |
57+
| app.rb:151:14:151:22 | user_data | semmle.label | user_data |
58+
| app.rb:152:14:152:26 | simple_result | semmle.label | simple_result |
59+
| app.rb:159:13:159:19 | user_id | semmle.label | user_id |
60+
| app.rb:159:23:159:28 | call to params | semmle.label | call to params |
61+
| app.rb:159:23:159:33 | ...[...] | semmle.label | ...[...] |
62+
| app.rb:160:18:160:24 | user_id | semmle.label | user_id |
63+
| app.rb:165:9:165:17 | user_data | semmle.label | user_data |
64+
| app.rb:165:21:165:31 | call to user_params | semmle.label | call to user_params |
65+
| app.rb:166:14:166:22 | user_data | semmle.label | user_data |
66+
subpaths
67+
testFailures
68+
#select
69+
| app.rb:133:14:133:20 | user_id | app.rb:126:19:126:24 | call to params | app.rb:133:14:133:20 | user_id | $@ | app.rb:126:19:126:24 | call to params | call to params |
70+
| app.rb:134:14:134:21 | route_id | app.rb:127:20:127:40 | call to route_param | app.rb:134:14:134:21 | route_id | $@ | app.rb:127:20:127:40 | call to route_param | call to route_param |
71+
| app.rb:135:14:135:17 | auth | app.rb:128:16:128:22 | call to headers | app.rb:135:14:135:17 | auth | $@ | app.rb:128:16:128:22 | call to headers | call to headers |
72+
| app.rb:136:14:136:20 | session | app.rb:129:19:129:25 | call to cookies | app.rb:136:14:136:20 | session | $@ | app.rb:129:19:129:25 | call to cookies | call to cookies |
73+
| app.rb:144:14:144:19 | result | app.rb:107:13:107:32 | call to source | app.rb:144:14:144:19 | result | $@ | app.rb:107:13:107:32 | call to source | call to source |
74+
| app.rb:151:14:151:22 | user_data | app.rb:103:13:103:18 | call to params | app.rb:151:14:151:22 | user_data | $@ | app.rb:103:13:103:18 | call to params | call to params |
75+
| app.rb:152:14:152:26 | simple_result | app.rb:111:13:111:33 | call to source | app.rb:152:14:152:26 | simple_result | $@ | app.rb:111:13:111:33 | call to source | call to source |
76+
| app.rb:160:18:160:24 | user_id | app.rb:159:23:159:28 | call to params | app.rb:160:18:160:24 | user_id | $@ | app.rb:159:23:159:28 | call to params | call to params |
77+
| app.rb:166:14:166:22 | user_data | app.rb:103:13:103:18 | call to params | app.rb:166:14:166:22 | user_data | $@ | app.rb:103:13:103:18 | call to params | call to params |

‎ruby/ql/test/library-tests/frameworks/grape/Flow.ql‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
/**
2+
* @kind path-problem
3+
*/
4+
5+
import ruby
6+
import utils.test.InlineFlowTest
7+
import PathGraph
8+
import codeql.ruby.frameworks.Grape
9+
import codeql.ruby.Concepts
10+
11+
module GrapeConfig implements DataFlow::ConfigSig {
12+
predicate isSource(DataFlow::Node source) {
13+
source instanceof Http::Server::RequestInputAccess::Range
14+
or
15+
DefaultFlowConfig::isSource(source)
16+
}
17+
18+
predicate isSink(DataFlow::Node sink) { DefaultFlowConfig::isSink(sink) }
19+
}
20+
21+
import FlowTest<DefaultFlowConfig, GrapeConfig>
22+
23+
from PathNode source, PathNode sink
24+
where flowPath(source, sink)
25+
select sink, source, sink, "$@", source, source.toString()

‎ruby/ql/test/library-tests/frameworks/grape/Grape.expected‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
grapeApiClasses
22
| app.rb:1:1:90:3 | MyAPI |
33
| app.rb:92:1:96:3 | AdminAPI |
4+
| app.rb:98:1:168:3 | UserAPI |
45
grapeEndpoints
56
| app.rb:1:1:90:3 | MyAPI | app.rb:7:3:11:5 | call to get | GET | /hello/:name |
67
| app.rb:1:1:90:3 | MyAPI | app.rb:17:3:20:5 | call to post | POST | /messages |
@@ -13,6 +14,10 @@ grapeEndpoints
1314
| app.rb:1:1:90:3 | MyAPI | app.rb:78:3:82:5 | call to get | GET | /cookie_test |
1415
| app.rb:1:1:90:3 | MyAPI | app.rb:85:3:89:5 | call to get | GET | /header_test |
1516
| app.rb:92:1:96:3 | AdminAPI | app.rb:93:3:95:5 | call to get | GET | /admin |
17+
| app.rb:98:1:168:3 | UserAPI | app.rb:124:5:138:7 | call to get | GET | /comprehensive_test/:user_id |
18+
| app.rb:98:1:168:3 | UserAPI | app.rb:140:5:145:7 | call to get | GET | /helper_test/:user_id |
19+
| app.rb:98:1:168:3 | UserAPI | app.rb:147:5:153:7 | call to post | POST | /users |
20+
| app.rb:98:1:168:3 | UserAPI | app.rb:164:5:167:7 | call to post | POST | /users |
1621
grapeParams
1722
| app.rb:8:12:8:17 | call to params |
1823
| app.rb:14:3:16:5 | call to params |
@@ -22,19 +27,30 @@ grapeParams
2227
| app.rb:36:5:36:10 | call to params |
2328
| app.rb:60:12:60:17 | call to params |
2429
| app.rb:94:5:94:10 | call to params |
30+
| app.rb:103:13:103:18 | call to params |
31+
| app.rb:126:19:126:24 | call to params |
32+
| app.rb:142:19:142:24 | call to params |
33+
| app.rb:159:23:159:28 | call to params |
2534
grapeHeaders
2635
| app.rb:9:18:9:24 | call to headers |
2736
| app.rb:46:5:46:11 | call to headers |
2837
| app.rb:66:3:69:5 | call to headers |
2938
| app.rb:86:12:86:18 | call to headers |
3039
| app.rb:87:14:87:20 | call to headers |
40+
| app.rb:116:5:118:7 | call to headers |
41+
| app.rb:128:16:128:22 | call to headers |
3142
grapeRequest
3243
| app.rb:25:12:25:18 | call to request |
44+
| app.rb:130:21:130:27 | call to request |
3345
grapeRouteParam
3446
| app.rb:51:15:51:35 | call to route_param |
3547
| app.rb:52:15:52:36 | call to route_param |
3648
| app.rb:57:3:63:5 | call to route_param |
49+
| app.rb:127:20:127:40 | call to route_param |
50+
| app.rb:156:5:162:7 | call to route_param |
3751
grapeCookies
3852
| app.rb:72:3:75:5 | call to cookies |
3953
| app.rb:79:15:79:21 | call to cookies |
4054
| app.rb:80:16:80:22 | call to cookies |
55+
| app.rb:120:5:122:7 | call to cookies |
56+
| app.rb:129:19:129:25 | call to cookies |

‎ruby/ql/test/library-tests/frameworks/grape/app.rb‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,4 +93,76 @@ class AdminAPI < Grape::API
9393
get '/admin' do
9494
params[:token]
9595
end
96-
end
96+
end
97+
98+
class UserAPI < Grape::API
99+
VALID_PARAMS = %w(name email password password_confirmation)
100+
101+
helpers do
102+
def user_params
103+
params.select{|key,value| VALID_PARAMS.include?(key.to_s)} # Real helper implementation
104+
end
105+
106+
def vulnerable_helper(user_id)
107+
source "paramHelper" # Test parameter passing to helper
108+
end
109+
110+
def simple_helper
111+
source "simpleHelper" # Test simple helper return
112+
end
113+
end
114+
115+
# Headers and cookies blocks for DSL testing
116+
headers do
117+
requires :Authorization, type: String
118+
end
119+
120+
cookies do
121+
requires :session_id, type: String
122+
end
123+
124+
get '/comprehensive_test/:user_id' do
125+
# Test all Grape input sources
126+
user_id = params[:user_id] # params taint source
127+
route_id = route_param(:user_id) # route_param taint source
128+
auth = headers[:Authorization] # headers taint source
129+
session = cookies[:session_id] # cookies taint source
130+
body_data = request.body.read # request taint source
131+
132+
# Test sinks for all sources
133+
sink user_id # $ hasTaintFlow
134+
sink route_id # $ hasTaintFlow
135+
sink auth # $ hasTaintFlow
136+
sink session # $ hasTaintFlow
137+
# Note: request.body.read may not be detected by this flow test config
138+
end
139+
140+
get '/helper_test/:user_id' do
141+
# Test helper method parameter passing dataflow
142+
user_id = params[:user_id]
143+
result = vulnerable_helper(user_id)
144+
sink result # $ hasTaintFlow=paramHelper
145+
end
146+
147+
post '/users' do
148+
# Test helper method return dataflow
149+
user_data = user_params
150+
simple_result = simple_helper
151+
sink user_data # $ hasTaintFlow
152+
sink simple_result # $ hasTaintFlow=simpleHelper
153+
end
154+
155+
# Test route_param block pattern
156+
route_param :id do
157+
get do
158+
# params[:id] should be user input from the path
159+
user_id = params[:id]
160+
sink user_id # $ hasTaintFlow
161+
end
162+
end
163+
164+
post '/users' do
165+
user_data = user_params
166+
sink user_data # $ hasTaintFlow
167+
end
168+
end

‎ruby/ql/test/query-tests/security/cwe-089/ArelInjection.rb‎

Lines changed: 1 addition & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -6,66 +6,4 @@ def unsafe_action
66
sql = Arel.sql("SELECT * FROM users WHERE name = #{name}")
77
sql = Arel::Nodes::SqlLiteral.new("SELECT * FROM users WHERE name = #{name}")
88
end
9-
end
10-
11-
class PotatoAPI < Grape::API
12-
get '/unsafe_endpoint' do
13-
name = params[:user_name]
14-
# BAD: SQL statement constructed from user input
15-
sql = Arel.sql("SELECT * FROM users WHERE name = #{name}")
16-
sql = Arel::Nodes::SqlLiteral.new("SELECT * FROM users WHERE name = #{name}")
17-
end
18-
end
19-
20-
class SimpleAPI < Grape::API
21-
get '/test' do
22-
x = params[:name]
23-
Arel.sql("SELECT * FROM users WHERE name = #{x}")
24-
end
25-
end
26-
27-
# Test helper method pattern in Grape helpers block
28-
class TestAPI < Grape::API
29-
helpers do
30-
def vulnerable_helper(user_id)
31-
# BAD: SQL statement constructed from user input passed as parameter
32-
Arel.sql("SELECT * FROM users WHERE id = #{user_id}")
33-
end
34-
end
35-
36-
# Headers and cookies blocks for DSL testing
37-
headers do
38-
requires :Authorization, type: String
39-
end
40-
41-
cookies do
42-
requires :session_id, type: String
43-
end
44-
45-
get '/comprehensive_test/:user_id' do
46-
# BAD: Comprehensive test using all Grape input sources in one SQL query
47-
user_id = params[:user_id] # params taint source
48-
route_id = route_param(:user_id) # route_param taint source
49-
auth = headers[:Authorization] # headers taint source
50-
session = cookies[:session_id] # cookies taint source
51-
body_data = request.body.read # request taint source
52-
53-
# All sources flow to SQL injection
54-
Arel.sql("SELECT * FROM users WHERE id = #{user_id} AND route_id = #{route_id} AND auth = #{auth} AND session = #{session} AND data = #{body_data}")
55-
end
56-
57-
get '/helper_test' do
58-
# BAD: Test helper method dataflow
59-
user_id = params[:user_id]
60-
vulnerable_helper(user_id)
61-
end
62-
63-
# Test route_param block pattern
64-
route_param :id do
65-
get do
66-
# BAD: params[:id] should be user input from the path
67-
user_id = params[:id]
68-
Arel.sql("SELECT * FROM users WHERE id = #{user_id}")
69-
end
70-
end
71-
end
9+
end

0 commit comments

Comments
 (0)