Ruby: Model flow through ActionController::Parameters

[hmac]

Add flow summaries for methods on ActionController::Parameters,
which mostly propagate taint from receiver to return value.

Evaluation shows that we catch a couple more TPs due to flow through parameters now reaching some sinks.

[hmac]

@asgerf just FYI, I wanted to define a TypeModel here, so I could write the type and flow summaries in MaD form:

  class ParamsInstanceType extends ModelInput::TypeModel {
    override DataFlow::LocalSourceNode getASource(string package, string type) {
      package = "actioncontroller" and
      type = "ActionController::Parameters" and
      result instanceof ParamsCall
    }
  }

But this created non-monotonic recursion errors, I think because ParamsCall uses API graphs (via ActionControllerContextCall). I don't know if it's possible to work around/fix this, but thought you would want to know. I expect many use cases for TypeModels will run into this problem.

[asgerf]

That is inevitable, I'm afraid. If ParamsCall depends on API graphs, shouldn't you already have an API node for it? In that case, using the API graph members will have to be good enough.

[hmac]

It's a bit more convoluted than that:

abstract class ParamsCall extends MethodCall {
  ParamsCall() { this.getMethodName() = "params" }
}

private class ActionControllerParamsCall extends ActionControllerContextCall, ParamsCall { }

private class ActionControllerContextCall extends MethodCall {
  private ActionControllerControllerClass controllerClass;

  ActionControllerContextCall() {
    this.getReceiver() instanceof SelfVariableAccess and
    this.getEnclosingModule() = controllerClass
  }
}

class ActionControllerControllerClass extends ClassDeclaration {
  ActionControllerControllerClass() {
    this.getSuperclassExpr() =
      [
        API::getTopLevelMember("ActionController").getMember("Base"),
        API::getTopLevelMember("ApplicationController")
      ].getASubclass().getAValueReachableFromSource().asExpr().getExpr()
  }
}

[asgerf]

Actually on second thought it seems really easy to add #10603. Would this work for you?

[hmac]

I don't think that helps here (though I could be wrong). We have API nodes for ActionControllerControllerClass, but not ParamsCall, which is abstract. In fact there's a second contribution to this class from calls to params in ERB files (see ActionView.qll), and in that case we have no API nodes at all.

[asgerf]

OK, I've taken a look at that model and the problem arises when we're using API graphs to populate concepts that are based on AST nodes. Having to step outside API graphs should be considered tech debt, and not be a common case, and I believe Ruby's API graphs need to be developed a bit further for this to work. In particular we'll need to add MethodDefinitionNode (in JS this would correspond to a MkDef wrapping a DataFlow::FunctionNode) and something like the MkClassInstance node from JS. I'll look into that, but it will not arrive in time for this PR.

[hmac]

That sounds like the right sort of thing we'd need for a lot of this modelling 👍

[alexrford]

Really nice, the changes make sense and the new results look legit!

[hvitved]

Shouldn't prev be restricted?

[hmac]

Ah yes, prev should be a ParamsInstance! 🤦

[hvitved]

I tend to write this = any(DataFlow::CallNode call instead.

[hvitved]

Presumably, only Argument[self] when exists(ParamsInstance i | i.asExpr().getExpr() = result.getReceiver()) (and similarly for Argument[0])?

[hmac]

I don't think so, though I may be misunderstanding you.

getACall already restricts us to cases where either the receiver is a ParamsInstance or arg0 is a ParamsInstance. merge is a stdlib method on Hash. In addition, activesupport adds reverse_merge and with_defaults to Hash. Thus we get the following behaviour:

params = ActionController::Parameters.new({a: 1})
=> #<ActionController::Parameters {"a"=>1} permitted: false>
params.permit! # required for some of these to work
=> #<ActionController::Parameters {"a"=>1} permitted: true>
params.merge({b:2})
=> #<ActionController::Parameters {"a"=>1, "b"=>2} permitted: true>
{b:2}.merge(params)
=> {:b=>2, "a"=>1}
params.reverse_merge({b:2})
=> #<ActionController::Parameters {"b"=>2, "a"=>1} permitted: true>
{b:2}.reverse_merge(params)
=> #<ActionController::Parameters {"a"=>1, "b"=>2} permitted: true>
params.with_defaults({b:2})
=> #<ActionController::Parameters {"b"=>2, "a"=>1} permitted: true>
{b:2}.with_defaults(params)
=> #<ActionController::Parameters {"a"=>1, "b"=>2} permitted: true>

params2 = ActionController::Parameters.new({b:2}).tap(&:permit!)
=> #<ActionController::Parameters {"b"=>2} permitted: false>
params.merge(params2)
=> #<ActionController::Parameters {"a"=>1, "b"=>2} permitted: true>
params2.merge(params)
=> #<ActionController::Parameters {"b"=>2, "a"=>1} permitted: true>
params.reverse_merge(params2)
=> #<ActionController::Parameters {"b"=>2, "a"=>1} permitted: true>
params2.reverse_merge(params)
=> #<ActionController::Parameters {"a"=>1, "b"=>2} permitted: true>
params.with_defaults(params2)
=> #<ActionController::Parameters {"b"=>2, "a"=>1} permitted: true>
params2.with_defaults(params)
=> #<ActionController::Parameters {"a"=>1, "b"=>2} permitted: true>

In all cases, taint is propagated from receiver and arg0 to the return value, regardless of which is the ParamsInstance.

[hmac]

Lots of these aren't covered in the tests, so I'll add cases for them.

[hmac]

Actually the only missing cases are merging two params instances, but in practice these would both be tainted anyway so it's not so important to test I think.

[hvitved]

OK, just wanted to double check that there was not only supposed to be flow from the argument that is an instance of ParamsInstance.

[hvitved]

Same comment as above.

[hvitved]

LGTM. I have started a new DCA run; let's see what it says before merging.

[hmac]

@hvitved DCA shows a 5% slowdown on opal/opal but I don't know why, since there are no ActionController::Parameters instances in that project. Otherwise everything looks OK.

[hvitved]

DCA shows a 5% slowdown on opal/opal but I don't know why

I think that may just be noise. So go ahead and merge.