Selecting properties following an Object merge

[jason-invision]

Javascript

  delete (endpointUrl, body, config = {}) {
    return this.request(endpointUrl, Object.assign({method: 'DELETE', body: body}, config))
  }

  request (endpointUrl, opts = {}) {
    const options = {
      url: this.baseUrl + endpointUrl,
      json: true,
      resolveWithFullResponse: true,
      simple: true,
      time: true,
      timeout: REQUEST_TIMEOUT
    }

    // Merge request options
    if (opts) Object.assign(options, opts, {headers: Object.assign({}, opts.headers || {}, this.headers)})

    const req = request(options)

Query

import javascript

from ClientRequest req
select req.getAnArgument().getALocalSource().getAPropertyReference()

This is not listing body, which also prevents req.getADataNode() from returning any result.

Likewise,

req.getAnArgument().getALocalSource()

should return options from the Object.assign() call as a result, and it does not.

I have seen examples where data flow continues for

let obj = Object.assign(target, src)

from target and src to obj.

[asgerf]

Hi @jason-invision,

getALocalSource and getAPropertyReference only traverse local data flow, which excludes flow through function calls and merge calls like Object.assign.

Taint-tracking will indeed flow through Object.assign and the function calls. Are you tracking body using taint-tracking? In that case the problem might be that it's not body flowing into the request call, but an object containing body in one of its properties. Therefore it will appear never to reach the sink.

If that's the case I'd suggest adding a taint step that taints the entire object when its body property becomes tainted:

  override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
    // If `obj.body` becomes tainted, consider the the whole `obj` to be tainted
    exists(DataFlow::PropWrite write |
      write.getPropertyName() = "body" and
      pred = write.getRhs() and
      succ = write.getBase().getALocalSource()
    )
  }

[jason-invision]

That worked.

I wonder if there's a way to include this taint step for all arguments of a ClientRequest, since the individual PropRead's of the properties of the options object will occur after the sink's PathNode, if at all, when analyzing the data flow.

[asgerf]

Here's a technique for tracking arbitrarily deep into nested objects, which we also use in the prototype pollution query. This way, if any part of the options to a client request is tainted, you get an alert.

I've tried to annotate the example with some explanatory comments, but there's also a tutorial on flow labels:

// Introduce a new kind of flow label for tracking objects that
// contain a tainted value in one of their properties.
class TaintContainer extends DataFlow::FlowLabel {
  TaintContainer() { this = "TaintContainer" }
}

class MyTaint extends TaintTracking::Configuration {
  /* ... */

  override predicate isAdditionalFlowStep(
      DataFlow::Node pred,
      DataFlow::Node succ,
      DataFlow::FlowLabel predlbl,
      DataFlow::FlowLabel succlbl) {
    // If `obj.x` becomes tainted, consider the the whole `obj` to be a "taint container"
    exists(DataFlow::PropWrite write |
      pred = write.getRhs() and
      succ = write.getBase().getALocalSource() and
      predlbl = any(DataFlow::FlowLabel lbl) and
      succlbl instanceof TaintContainer
    )
  }

  override predicate isSink(DataFlow::Node node, DataFlow::FlowLabel lbl) {
    // Include both regular taint and taint containers in sinks
    node = any(ClientRequest req).getAnArgument() and
    (lbl.isTaint() or lbl instanceof TaintContainer)
  }
}