Tracing data flow through Javascript class properties

[jason-invision]

I have the following example I am having trouble tracing the data flow through.

class BaseHandler {
   constructor (req, res) {
       ...
   }

   get service () {
       if (!this._service) {
          this._service = new services.Service()
       }
   }
}

class ServiceHandler extends BaseHandler {

  constructor (req, res) {
        super(req, res)

        this.service.handle(req)
}

I can trace req from the SourceNode all the way to the call to this.sevice.handle(req), but the data flow does not continue to the implementation of the handle() function.

[jason-invision]

I created this predicate to try and solve the problem. Not sure if it is precise enough.

override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
      exists(DataFlow::CallNode c, DataFlow::FunctionNode f |
        c.getCalleeName() = f.getName() and
        pred = c.getArgument(0) and
        succ = f.getParameter(0)
      )
    }

[esbena]

Not sure if it is precise enough.

I think it is fine for a quick-n-dirty query, but it is not something you want to have enabled by default, as it can result in a lot of spurious flow.

A few questions:

1. Did the additional taint step you showed solve the problem, or is it just an attempt?

2. Is the this.service.handle resolved to the expected function?

You can check with something like:

from DataFlow::ThisNode dis, DataFlow::CallNode call
where dis.getAPropertyRead("service").getAMethodCall("handle") = call
select call, call.getACallee()

3. Is this.service.handle defined as a property of an object that is obtained lazily through a getter?

I think that is a bit too indirect for us to reason about, but I do see the value in supporting such a pattern OOTB.

4. Is get service () { ... } supposed to return something?

[jason-invision]

Did the additional taint step you showed solve the problem, or is it just an attempt?

It found the paths, but it included some additional results that were not correct.

Is the this.service.handle resolved to the expected function?

I ran your example, but did not get any results.

Is get service () { ... } supposed to return something?

this.service is a property that holds the value of a class whose super class contains the function. The getter will either return the class instance so it can call the method, or it will create a new instance and return that.

[asgerf]

@jason-invision I think I recognize this problem, and you might be running into a limitation in how we track flow through getters. We're working on a general fix to this, but in the meantime you can try include this snippet in your query to patch up the call graph:

private predicate accessorCall(DataFlow::PropRead read, DataFlow::FunctionNode callee) {
  exists(DataFlow::ClassNode cls, string name |
    read = cls.getAnInstanceReference().getAPropertyRead(name) and
    callee = cls.getADirectSuperClass*().getInstanceMember(name, DataFlow::MemberKind::getter())
  )
}

private class AccessorCallPatch extends DataFlow::MethodCallNode {
  override Function getACallee(int imprecision) {
    result = super.getACallee(imprecision)
    or
    imprecision = 0 and
    exists(DataFlow::PropRead read, DataFlow::FunctionNode callee, string methodName, DataFlow::ClassNode cls |
      accessorCall(read, callee) and
      this = read.getAMethodCall(methodName) and
      cls.getAnInstanceReference().flowsTo(callee.getReturnNode()) and
      result = cls.getInstanceMethod(methodName).getFunction()
    )
  }
}

Could you try this and see if you can trace req further?

[asgerf]

Anywhere. Note that you don't have to use the predicate and class anywhere, the fact that we're overriding getACallee changes the underlying call graph that taint tracking relies on.

[jason-invision]

This worked. Thanks. I just added it as an additional taintstep.