RB: Experimental strong params query

[thiggy1342]

The aim of this query to to find direct or bulk param accesses in controller methods that are outside of methods adhering to the strong parameter convention found in rails: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html By accessing parameters directly, it may lead to a vulnerability where an unexpected parameter key/value pair is included in a payload that is then mistakenly accessed or passed to other areas of the application, opening the application up to potential exploitation.

I have tried to add some DataFlow tracking to this query to help reduce false positives in cases where the raw parameter access data flows to a Model method of some kind, which appears to be the biggest category of vulnerabilities addressed by the strong parameters pattern in the Rails documentation. I'm curious if y'all think this is useful as written. I feel like it loses some fidelity when finding potential injection vulnerabilities where the sink is a Model method. It would be trivial to remove this, as the WeakParams class comprises of most of the query that pinpoints param accesses outside of strong param methods. I will also need to update the tests if y'all think the DataFlow component should stick around.

[github-actions]

QHelp previews:

ruby/ql/src/experimental/weak-params/WeakParams.qhelp

Weak or direct parameter references are used

Directly checking request parameters without following a strong params pattern can lead to unintentional avenues for injection attacks.

Recommendation

Instead of manually checking parameters from the `param` object, it is recommended that you follow the strong parameters pattern established in Rails: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html

In the strong parameters pattern, you are able to specify required and allowed parameters for each action called by your controller methods. This acts as an additional layer of data validation before being passed along to other areas of your application, such as the model.

References

[hmac]

This seems like a good idea to me! You can simplify your query a lot, though. Firstly, there are many useful classes in codeql.ruby.frameworks.ActionController and codeql.ruby.frameworks.ActiveRecord that you can use.

To find calls to request.request_parameters etc we can first model request, which is a method call only available in an ActionController context.

/**
 * A call to `request` in an ActionController controller class.
 * This probably refers to the incoming HTTP request object.
 */
class ActionControllerRequest extends DataFlow::Node {
  ActionControllerRequest() {
    exists(DataFlow::CallNode c |
      c.asExpr().getExpr().getEnclosingModule() instanceof ActionControllerControllerClass and
      c.getMethodName() = "request"
    |
      c.flowsTo(this)
    )
  }
}

This doesn't include calls in ActionView classes unfortunately, but it's a start. ActionControllerControllerClass is defined in the ActionController library I mentioned above.

The reason we use flowsTo in this way is to catch cases like this:

class MyController < ApplicationController
  def create
    r = request
    User.create(r.request_parameters)
  end
end

Taking request.request_parameters as an example, we want to find any calls to request_parameters where the receiver is an ActionControllerRequest.

/**
 * A direct parameters reference that happens inside a controller class.
 */
class WeakParams extends DataFlow::CallNode {
  WeakParams() {
    this.getReceiver() instanceof ActionControllerRequest and
    this.getMethodName() =
      [
        "request_parameters"
      ]
  }
}

That gives us our source, now we need a sink. Luckily we already have modelling for ActiveRecord calls like User.create, User.update etc. These are all defined as instances of the PersistentWriteAccess concept (see codeql.ruby.Concepts). This concept represents any write to a persistent store such as a database. This is possibly too broad, and if we want to restrict the sources to just AR calls then we can do that (though the classes are currently private and would need to be made public). Assuming we're happy with using PersistentWriteAccess then our taint tracking config becomes:

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

  override predicate isSource(DataFlow::Node node) { node instanceof WeakParams }

  override predicate isSink(DataFlow::Node node) { node = any(PersistentWriteAccess a).getValue() }
}

In the above I haven't considered excluding uses inside "strong params" methods. This is for two reasons:

• The heuristic of "a strong params method is one that ends in _params" seems unreliable to me.
• As far as I know, the params object is safe from mass assignment attacks because you have to explicitly permit the keys you want to extract from it.

I'm also not sure where the expose_all and original_hash methods come from - I couldn't find them in the Rails docs.

[thiggy1342]

@hmac Thank you for the review, and I think this approach makes a lot more sense. My initial approach was way too verbose and brittle, as you mentioned. One question that I did have is: do you think the use of the allParamsAccess predicate makes sense here, or should I just bake that into the WeakParams class? I was trying to use the predicate as intended, but it may be overkill in this case.

Lastly, my tests for this need some work. So far, I've been doing most of my testing at the controller level, but I'd love to have some tests here as well. Is there an example of a test where some PersistentWriteAccess classes are "mocked" for testing purposes?

[hmac]

Yes, I think since allParamsAccess is only used in WeakParams, you could inline it into WeakParams. To recognise the calls in your test as PersistentWriteAccesses it should be enough to add a definition for TestObject so that we recognise it as an ActiveRecord class:

class TestObject < ActiveRecord::Base
end

However it looks like some shortcomings in our ActiveRecord library means that we won't recognise calls like TestObject.create(params). For now, if you change those calls to TestObject.create(foo: params[:foo]), so the tainted object is passed as a single attribute in a keyword parameter, then you should get some results. I've made a note for us to fix these gaps in the future.

[thiggy1342]

@hmac Thanks! I've added the tests and they seem to be working as expected, so I think this is pretty close unless there's anything else that I should add.

[hmac]

This just needs a change note and it's good to go!