[Checks]: Add an HTTP method confusion check

[linosgian]

Is your feature request related to a problem? Please describe.
There was an issue released recently regarding a bypass on Github's OAuth2 flow (essentially CSRF bypass). In a nutshell, the issue boils down to:

# In the router

match "/login/oauth/authorize", # For every request with this path...
  :to => "[the controller]", # ...send it to the controller...
  :via => [:get, :post] # ... as long as it's a GET or a POST request.

(the route only handles GET/POST)
and

# In the controller

if request.get?
  # serve authorization page HTML
else
  # grant permissions to app
end

The other "ingredient" of the vulnerability is the fact that Rails handles HEAD requests, exactly like GET, and just removes the response body before the response is sent back to the user. This means that all the processing done for GET requests will be executed for HEAD as well. The tricky part is that request.get? will return false for HEAD requests.

As shown in the code above, the developers were not explicit about which HTTP methods they handle (else ..), thinking that since it's not a GET request, then it must be POST (since the route only handles those two). Therefore, a HEAD request, fell through to the else clause, which resulted in CSRF bypass, since CSRF is not verified in GET/HEAD requests.

Describe the solution you'd like
I think that Brakeman can greatly assist security engineers/developers in finding such issues. I was thinking about tracking these kinds of calls (request.get? or request.<any http method>?), check whether they have an else clause in controllers. Additionally, if possible, double check that a route handles multiple HTTP methods in order to reduce false positives.

Describe alternatives you've considered
I can't come up with other solution, not knowing a great deal about Brakeman's internals, but I'm open to your suggestions.

Additional context
If you think that this is possible and something that we want to implement, I'd love to help out, with your guidance.

[presidentbeef]

Hi @linosgian,

Thank you for this suggestion.

So this is possible, but not straightforward, to implement in Brakeman. Essentially you have to first find all the request.get?-type calls (easy) but then go back to the methods where they are called and find the if conditions (if they exist) and then check the else clauses. This will be slower than most Brakeman checks.

Secondly, there is a bit of a gap between the code pattern and the potential vulnerability. It might be tricky to present the information, and I don't know what kind of false positives might come up. I'd lean towards making this an optional check (depending on results).

if possible, double check that a route handles multiple HTTP methods in order to reduce false positives

I think this information is available. (I've pretty much given up on handling Rails routes. They are too complicated and allow too much dynamic behavior.)

[presidentbeef]

I think this information is available.

I was wrong, this information (HTTP verbs for routes) is not available right now.

I have a rough implementation of this check and am looking through results for false positives to see how important it might be to check the routes.