Skip to content

Commit db41ce5

Browse files
hmachmac
authored
Merge pull request #9605 from thiggy1342/experimental-manually-check-request-verb
RB: Experimental query to manually check request verb
2 parents 0a35f97 + b4d762f commit db41ce5

File tree

6 files changed

+273
-0
lines changed

6 files changed

+273
-0
lines changed

‎ruby/ql/src/change-notes/2022-07-21-check-http-verb.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new experimental query, `rb/manually-checking-http-verb`, to detect cases when the HTTP verb for an incoming request is checked and then used as part of control flow.

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Manually checking the HTTP request verb inside of a controller method can lead to
8+
CSRF bypass if GET or HEAD requests are handled improperly.
9+
</p>
10+
</overview>
11+
<recommendation>
12+
<p>
13+
It is better to use different controller methods for each resource/http verb combination
14+
and configure the Rails routes in your application to call them accordingly.
15+
</p>
16+
</recommendation>
17+
18+
<references>
19+
<li>
20+
See https://guides.rubyonrails.org/routing.html for more information.
21+
</li>
22+
</references>
23+
</qhelp>

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

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
/**
2+
* @name Manually checking http verb instead of using built in rails routes and protections
3+
* @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 path-problem
5+
* @problem.severity error
6+
* @security-severity 5.0
7+
* @precision low
8+
* @id rb/manually-checking-http-verb
9+
* @tags security
10+
*/
11+
12+
import ruby
13+
import codeql.ruby.DataFlow
14+
import codeql.ruby.controlflow.CfgNodes
15+
import codeql.ruby.frameworks.ActionController
16+
import codeql.ruby.TaintTracking
17+
import DataFlow::PathGraph
18+
19+
// any `request` calls in an action method
20+
class Request extends DataFlow::CallNode {
21+
Request() {
22+
this.getMethodName() = "request" and
23+
this.asExpr().getExpr().getEnclosingMethod() instanceof ActionControllerActionMethod
24+
}
25+
}
26+
27+
// `request.env`
28+
class RequestEnvMethod extends DataFlow::CallNode {
29+
RequestEnvMethod() {
30+
this.getMethodName() = "env" and
31+
any(Request r).flowsTo(this.getReceiver())
32+
}
33+
}
34+
35+
// `request.request_method`
36+
class RequestRequestMethod extends DataFlow::CallNode {
37+
RequestRequestMethod() {
38+
this.getMethodName() = "request_method" and
39+
any(Request r).flowsTo(this.getReceiver())
40+
}
41+
}
42+
43+
// `request.method`
44+
class RequestMethod extends DataFlow::CallNode {
45+
RequestMethod() {
46+
this.getMethodName() = "method" and
47+
any(Request r).flowsTo(this.getReceiver())
48+
}
49+
}
50+
51+
// `request.raw_request_method`
52+
class RequestRawRequestMethod extends DataFlow::CallNode {
53+
RequestRawRequestMethod() {
54+
this.getMethodName() = "raw_request_method" and
55+
any(Request r).flowsTo(this.getReceiver())
56+
}
57+
}
58+
59+
// `request.request_method_symbol`
60+
class RequestRequestMethodSymbol extends DataFlow::CallNode {
61+
RequestRequestMethodSymbol() {
62+
this.getMethodName() = "request_method_symbol" and
63+
any(Request r).flowsTo(this.getReceiver())
64+
}
65+
}
66+
67+
// `request.get?`
68+
class RequestGet extends DataFlow::CallNode {
69+
RequestGet() {
70+
this.getMethodName() = "get?" and
71+
any(Request r).flowsTo(this.getReceiver())
72+
}
73+
}
74+
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())
90+
}
91+
}
92+
93+
from HttpVerbConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
94+
where config.hasFlowPath(source, sink)
95+
select sink.getNode(), source, sink,
96+
"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.qlref

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql

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

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
class ExampleController < ActionController::Base
2+
# Should find
3+
def example_action
4+
if request.get?
5+
Resource.find(id: params[:example_id])
6+
end
7+
end
8+
9+
# Should find
10+
def other_action
11+
method = request.env['REQUEST_METHOD']
12+
if method == "GET"
13+
Resource.find(id: params[:id])
14+
end
15+
end
16+
17+
# Should find
18+
def foo
19+
method = request.request_method
20+
if method == "GET"
21+
Resource.find(id: params[:id])
22+
end
23+
end
24+
25+
# Should find
26+
def bar
27+
method = request.method
28+
if method == "GET"
29+
Resource.find(id: params[:id])
30+
end
31+
end
32+
33+
# Should find
34+
def baz
35+
method = request.raw_request_method
36+
if method == "GET"
37+
Resource.find(id: params[:id])
38+
end
39+
end
40+
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+
49+
# Should find
50+
def foobarbaz
51+
method = request.request_method_symbol
52+
if method == :GET
53+
Resource.find(id: params[:id])
54+
end
55+
end
56+
57+
# Should find
58+
def resource_action
59+
case request.env['REQUEST_METHOD']
60+
when "GET"
61+
Resource.find(id: params[:id])
62+
when "POST"
63+
Resource.new(id: params[:id], details: params[:details])
64+
end
65+
end
66+
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
76+
end
77+
78+
class SafeController < ActionController::Base
79+
# this class should have no hits because controllers rely on conventional Rails routes
80+
def index
81+
Resource.find(id: params[:id])
82+
end
83+
84+
def create
85+
Resource.new(id: params[:id], details: params[:details])
86+
end
87+
88+
def update
89+
Resource.update(id: params[:id], details: params[:details])
90+
end
91+
92+
def delete
93+
s = Resource.find(id: params[:id])
94+
s.delete
95+
end
96+
end
97+
98+
# There should be no hits from this class because it does not inherit from ActionController
99+
class NotAController
100+
def example_action
101+
if request.get?
102+
Resource.find(params[:example_id])
103+
end
104+
end
105+
106+
def resource_action
107+
case env['REQUEST_METHOD']
108+
when "GET"
109+
Resource.find(params[:id])
110+
when "POST"
111+
Resource.new(params[:id], params[:details])
112+
end
113+
end
114+
end
115+
116+
class Resource < ActiveRecord::Base
117+
end

0 commit comments

Comments
 (0)