RB: Experimental query to manually check request verb

[thiggy1342]

The aim of this query is to find all calls to .get?, .post?, .patch?, .put?, .delete?, and .head?, as well as element references to env["REQUEST_METHOD"] that are used in control flow expressions. The general idea is that if controller action methods are manually checking http methods as part of control flow, they might be unintentionally bypassing security measures that are intended to run before the controller method runs. For example, this could include authorization checks, CSRF token checks, etc.

There are a few areas where I'd like to refine this query:

• If possible, I'd like to limit findings to just those found in subclasses of ActionController. The idea here is that while some of the underlying scaffolding of a Rails app may need to use these checks in legitimate ways, it's probably not a good idea to do this in your standard controller classes. One thought I had was maybe checking the filepath of the file in question, but that seems a bit naive even though standard Rails convention would have the controllers in the app/controllers directory.
• Another feature I'd like to add is to check instances where env["REQUEST_METHOD"] references isn't directly used in a conditional statement or passed into include?, but is instead assigned to a variable who's value then flows into this check. I've tried to add this as part of CheckRequestMethodFromEnv but it doesn't seem to be working as expected.
• While my query does seem to be returning results from an existing application database, I don't seem to be getting any hits for my test classes. I need to figure out what I'm missing here.

[hmac]

@thiggy1342 thanks for opening this PR! To make sure I understand this idea, could you elaborate a bit on the link between checking HTTP verbs and bypassing authorization/CSRF checks?

In Rails, for example, auth checks are typically tied to a controller/action. For example, you might have something like

class MyController
  before_action :some_auth_check
  # ...

In this case all actions (request handlers) in MyController with be guarded with some_auth_check, regardless of what HTTP verb was used in the request.

Again in Rails, CSRF protection runs on all non-(GET/HEAD) requests and is enabled by default (and we have a query that checks if it has been explicitly disabled).

I think there's value in a query that looks for PersistentWriteAccesses (i.e. ActiveRecord create/update/delete operations) in GET request handlers, for example. But I'm not sure what sort of real-world vulnerabilities we would catch with this query. Are there CVEs or other things you could share with examples of this, or maybe sketch out an example vulnerability?

[thiggy1342]

@hmac Thanks for taking a look and that is a great question! This query aims to find examples where a Rails apps and controller routing is implemented in a non-standard way and folks might be getting themselves into trouble. Specifically, more than one uri path + HTTP verb combo would need to be routed to the same controller method. Something like this in routes.rb:

get "/example/resource", to "example#resource_method"
post "/example/resource", to "example#resource_method"

Then, inside of Example.resource_method, logic would branch based on the results from request.get? or request.post or env['REQUEST_METHOD'] == 'GET', etc.

I did some further digging, and as you mention, GET and HEAD requests are the only ones that don't get CSRF protection by default, and the detection happens at the request level. I'm not sure why I thought it was done as a component of routing (i.e. if the method name is "create", CSRF is checked but not for methods named "index."), so I don't think we'll find many cases where CSRF is missing.

There is still the possibility of a single method handling two HTTP verbs (again, in an app that where the default Rails routing is misused). For example if a single method handles PATCH and DELETE requests in a controller, and an "editor" user account is allowed to update a resource but not delete it, an unknowing developer may change before_action :ensure_admin to before_action :ensure_editor check to add functionality for PATCH requests routed to the method.

I don't have a great example of this in the wild, but the CSRF check has bit us before in the past. These may be too opinionated for the experimental ruleset, and perhaps a custom ruleset for internal use is the better path for this query.

[thiggy1342]

@hmac I've narrowed down the query scope to look specifically for control flow checks involving calls to request.get?, as it looks like HEAD requests are generally treated like GET requests, and having branching logic that checks just .get? could result in HEAD requests hitting codepaths that should be protected by CSRF. I have a few specific questions on parts of my query as well.

[thiggy1342]

The idea here is that I would also like to find instances where the value of `env["REQUEST_METHOD"] is compared to a string literal as part of a control flow check. I'm having a bit of trouble wrapping my head around how I'd constrain things to be part of a ConditionalExpr from here, or am I thinking about this the wrong way? Should I just add that part directly to my query?

[thiggy1342]

Also, does this seem useful or should I just stick to singling out calls to request.get??

[hmac]

The reason this isn't working is the use of toString(), which is generally just for debugging purposes and usually doesn't do what you want. In this case, calling toString() on the string literal "GET" will return "GET" rather than GET (i.e. the quotes will be included). What you want is .getConstantValue().getStringlikeValue().

Similarly with the second clause of GetRequestMethodFromEnv, you want to do this.getReceiver().(MethodCall).getMethodName() = "env" rather than this.getAChild+().toString() = "env".

On that note, generally you don't want to use getAChild() because there's usually a more specific way to get at the element you're interested in. If you find yourself doing something like getAChild+() because the thing you want might be nested somewhere deep inside the expression, consider using dataflow and instead looking for nodes where the thing you want flows to that node.

[thiggy1342]

Ah awesome! I've been leaning on .toString() far too much, so this is great!

[hmac]

Sorry for the delay! I've left some comments and general tips on how to make this query more accurate and efficient. Just shout if anything is unclear, or if you're ready for another review.

[hmac]

We can narrow down this extent of this class by looking only at method calls inside ActionController actions:

import codeql.ruby.frameworks.ActionController
// ...
class CheckGetRequest extends MethodCall {
  CheckGetRequest() {
    this.getEnclosingMethod() instanceof ActionControllerActionMethod
    and this.getMethodName() = "get?"
  }
}

(this improves the performance of the query, as we only look at get? calls that are relevant to us)

[hmac]

This class effectively already exists, as codeql.ruby.frameworks.ActionController::ActionControllerControllerClass.

[hmac]

Correct me if I'm wrong, but I think in an ActionController action there's no env variable in scope. I think you have request.env instead. There's also the following methods which look relevant to this query:

• request.method
• request.request_method
• request.raw_request_method
• request.request_method_symbol

If you write this query at the dataflow level instead of the AST level you can make it more robust to different ways people might write this Ruby code. For example, we would want to find cases like

method = request.request_method
allowed_method = "GET"
if method == allowed_method
  ...

One way to do this is to find dataflow nodes corresponding to each part we're interested in, and check that they flow to each other. For example:

import codeql.ruby.controlflow.CfgNodes

// any `request` calls in an action method
class Request extends DataFlow::CallNode {
  Request() {
    this.getMethodName() = "request" and
    this.asExpr().getExpr() instanceof ActionControllerActionMethod
  }
}

// `request.request_method`
class RequestMethod extends DataFlow::CallNode {
  Env() {
    this.getMethodName() = "request_method" and
    any(Request r).flowsTo(this.getReceiver())
  }
}

// A conditional expression where the condition uses `request.request_method` in some way.
// e.g.
// ```
// r = request.request_method
// if r == "GET"
// ...
// ```
class RequestMethodConditional extends DataFlow::Node {
  RequestMethodConditional() {
    // We have to cast the dataflow node down to a specific CFG node (`ExprNodes::ConditionalExprCfgNode`) to be able to call `getCondition()`.
    // We then find the dataflow node corresponding to the condition CFG node,
    // and filter for just nodes where `request_method` flows to them.
    exists(DataFlow::Node conditionNode |
      conditionNode.asExpr() = this.asExpr().(ExprNodes::ConditionalExprCfgNode).getCondition()
    |
      any(RequestMethod r).flowsTo(conditionNode)
    )
  }
}

Hopefully this makes sense and you can follow this approach for the rest of the query.

More generally, I have a hunch that we can abstract away the conditional checking using our BarrierGuards library, which models code that can "guard" some other bit of code. I will have a look at whether that's feasible here, but the approach above should work well enough to start with.

Please let me know if anything here is confusing or unclear! I'll try to help.

[thiggy1342]

Thanks for taking a look! I've pushed up some changes where I think I've followed this pattern, but the quick evals aren't quite working like I'd expect. While I'm not sure we'd see the full query find any results, the quick eval that I'm running on the Request class for example isn't returning any results. I'm wondering if there's anything I might have missed when trying to follow this pattern. I'm almost certain that I should be able to easily find any uses of request in just about any rails app.

[hmac]

Sorry, that's due to a typo in what I wrote above. The second clause of Request should be

this.asExpr().getExpr().getEnclosingMethod() instanceof ActionControllerActionMethod

because we're looking for any call to request that is inside an action controller method.

[hmac]

Since the query is in ql/src/experimental I think we should put this test in ql/test/query-tests/experimental.

[thiggy1342]

@hmac After making the recommended changes, it looks like I can capture instances of reqeust.get? but nothing else. One thing I did notice is that when calling request.get? there isn't a comparison operation, just testing if it returns true. I'm wondering if I'm missing something for instances where the data from our source needs to flow through a comparison operations i.e. request.method == 'GET' and our current query include those?

[hmac]

You're right, I think that's the problem. For an expression like method == "GET", we don't consider the data in method to flow to the whole expression, because that would imply that the expression returns method in some way, but it doesn't - it returns a boolean.

However we do consider taint to flow from method to the whole expression, because the value of method can influence the value of method == "GET". So the problem here is we're using dataflow when we should be using taint tracking. Something like this should find all your results:

import codeql.ruby.TaintTracking

// ...

class HttpVerbConfig extends TaintTracking::Configuration {
  HttpVerbConfig() { this = "HttpVerbConfig" }

  override predicate isSource(DataFlow::Node source) {
    source instanceof RequestRequestMethod or
    source instanceof RequestMethod or
    source instanceof RequestRawRequestMethod or
    source instanceof RequestRequestMethodSymbol or
    source instanceof RequestGet
  }

  override predicate isSink(DataFlow::Node sink) {
    exists(ExprNodes::ConditionalExprCfgNode c | c.getCondition() = sink.asExpr())
  }
}

from HttpVerbConfig config, DataFlow::Node source, DataFlow::Node sink
where config.hasFlow(source, sink)
select source, sink

This is very similar to what you already have: we just look for taint flowing from any of the request methods into the condition of a conditional expr.

As a side note, I would suggest adding a test case that has a conditional expression on some condition that doesn't use one of these methods, to make sure we're targeting only the relevant conditionals.

[github-actions]

QHelp previews:

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

Manually checking http verb instead of using built in rails routes and protections

Manually checking the HTTP request verb inside of a controller method can lead to CSRF bypass if GET or HEAD requests are handled improperly.

Recommendation

It is better to use different controller methods for each resource/http verb combination and configure the Rails routes in your application to call them accordingly.

References

• See https://guides.rubyonrails.org/routing.html for more information.

[thiggy1342]

@hmac That did the trick! I also went ahead and expanded the sink to catch cases where the result of on of our method checks flows into a case statement as well, since that could also be used for control flow. I've updated the tests and added a qhelp file, so if you think things look good, this might be about ready to ship.

[thiggy1342]

perhaps not, I need to sort out this path problem formatting.

[hmac]

Just need a change note on this and we're good 👍