Skip to content

Commit 2cc7033

Browse files
thiggy1342thiggy1342
authored
use taint config for data flow
1 parent 7df7b92 commit 2cc7033

3 files changed

Lines changed: 70 additions & 28 deletions

File tree

‎ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql‎

Lines changed: 21 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
/**
22
* @name Manually checking http verb instead of using built in rails routes and protections
33
* @description Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods.
4-
* @kind problem
4+
* @kind path-problem
55
* @problem.severity error
66
* @security-severity 5.0
77
* @precision low
@@ -13,6 +13,8 @@ import ruby
1313
import codeql.ruby.DataFlow
1414
import codeql.ruby.controlflow.CfgNodes
1515
import codeql.ruby.frameworks.ActionController
16+
import codeql.ruby.TaintTracking
17+
import DataFlow::PathGraph
1618

1719
// any `request` calls in an action method
1820
class Request extends DataFlow::CallNode {
@@ -70,32 +72,24 @@ class RequestGet extends DataFlow::CallNode {
7072
}
7173
}
7274

73-
// A conditional expression where the condition uses `request.method`, `request.request_method`, `request.raw_request_method`, `request.request_method_symbol`, or `request.get?` in some way.
74-
// e.g.
75-
// ```
76-
// r = request.request_method
77-
// if r == "GET"
78-
// ...
79-
// ```
80-
class RequestMethodConditional extends DataFlow::Node {
81-
RequestMethodConditional() {
82-
// We have to cast the dataflow node down to a specific CFG node (`ExprNodes::ConditionalExprCfgNode`) to be able to call `getCondition()`.
83-
// We then find the dataflow node corresponding to the condition CFG node,
84-
// and filter for just nodes where a request method accessor value flows to them.
85-
exists(DataFlow::Node conditionNode |
86-
conditionNode.asExpr() = this.asExpr().(ExprNodes::ConditionalExprCfgNode).getCondition()
87-
|
88-
(
89-
any(RequestMethod r).flowsTo(conditionNode) or
90-
any(RequestRequestMethod r).flowsTo(conditionNode) or
91-
any(RequestRawRequestMethod r).flowsTo(conditionNode) or
92-
any(RequestRequestMethodSymbol r).flowsTo(conditionNode) or
93-
any(RequestGet r).flowsTo(conditionNode)
94-
)
95-
)
75+
class HttpVerbConfig extends TaintTracking::Configuration {
76+
HttpVerbConfig() { this = "HttpVerbConfig" }
77+
78+
override predicate isSource(DataFlow::Node source) {
79+
source instanceof RequestMethod or
80+
source instanceof RequestRequestMethod or
81+
source instanceof RequestEnvMethod or
82+
source instanceof RequestRawRequestMethod or
83+
source instanceof RequestRequestMethodSymbol or
84+
source instanceof RequestGet
85+
}
86+
87+
override predicate isSink(DataFlow::Node sink) {
88+
exists(ExprNodes::ConditionalExprCfgNode c | c.getCondition() = sink.asExpr()) or
89+
exists(ExprNodes::CaseExprCfgNode c | c.getValue() = sink.asExpr())
9690
}
9791
}
9892

99-
from RequestMethodConditional req
100-
select req,
101-
"Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods."
93+
from HttpVerbConfig config, DataFlow::Node source, DataFlow::Node sink
94+
where config.hasFlow(source, sink)
95+
select sink.asExpr().getExpr(), source, sink, "Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods."

‎ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
edges
2+
| ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env : | ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] : |
3+
| ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] : | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... |
4+
| ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method : | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... |
5+
| ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method : | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... |
6+
| ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method : | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... |
7+
| ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol : | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... |
8+
| ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env : | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] |
9+
nodes
10+
| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | semmle.label | call to get? |
11+
| ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env : | semmle.label | call to env : |
12+
| ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] : | semmle.label | ...[...] : |
13+
| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | semmle.label | ... == ... |
14+
| ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method : | semmle.label | call to request_method : |
15+
| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | semmle.label | ... == ... |
16+
| ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method : | semmle.label | call to method : |
17+
| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | semmle.label | ... == ... |
18+
| ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method : | semmle.label | call to raw_request_method : |
19+
| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | semmle.label | ... == ... |
20+
| ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol : | semmle.label | call to request_method_symbol : |
21+
| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | semmle.label | ... == ... |
22+
| ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env : | semmle.label | call to env : |
23+
| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | semmle.label | ...[...] |
24+
subpaths
25+
#select
26+
| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
27+
| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
28+
| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
29+
| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
30+
| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
31+
| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
32+
| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |

‎ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,14 @@ def baz
3838
end
3939
end
4040

41+
# Should not find
42+
def baz2
43+
method = request.raw_request_method
44+
if some_other_function == "GET"
45+
Resource.find(id: params[:id])
46+
end
47+
end
48+
4149
# Should find
4250
def foobarbaz
4351
method = request.request_method_symbol
@@ -56,7 +64,15 @@ def resource_action
5664
end
5765
end
5866

59-
67+
# Should not find
68+
def resource_action
69+
case request.random_method
70+
when "GET"
71+
Resource.find(id: params[:id])
72+
when "POST"
73+
Resource.new(id: params[:id], details: params[:details])
74+
end
75+
end
6076
end
6177

6278
class SafeController < ActionController::Base

0 commit comments

Comments
 (0)