JS: Fix handling of constant array index reads, and fix the fallout

[asgerf]

Previously, when reading an array element at a known index, we did not read values stored at an unknown index. We now properly read from unknown array indices.

However, this interacted poorly with how we handled split('#') and split('?') calls in TaintedUrlSuffix. In this case we want to block flow into the first element of the resulting array, while also transitioning from the tainted-url-suffix state to the taint state. Instead of the various hacks we've employed in the past, this is now modelled with the help of flow summaries.

Optional steps: Flow summaries for flow state and query-specific steps

Flow summaries cannot be query-specific and don't know anything about flow states (because those are query-specific), but I've adopted a solution where a flow summary can contribute so-called optional steps and barriers identified by a name, which queries can then opt into using their name.

So we use this summary for split('#') and split('?'):

      input = "Argument[this].OptionalBarrier[split-url-suffix]" and
      output = "ReturnValue.ArrayElement"
      or
      input = "Argument[this].OptionalStep[split-url-suffix-post]" and
      output = "ReturnValue.ArrayElement[1]"

And we then use those steps in our queries (here simplifying the indirection through the TaintedUrlSuffix.qll library)

predicate isBarrier(DataFlow::Node node, FlowState state) {
  optionalBarrier(node, "split-url-suffix") and state = "tainted-url-suffix"
}

predicate isAdditionalFlowStep(DataFlow::Node node1, FlowState state1, DataFlow::Node node2, FlowState state2) {
  optionalStep(node1, "split-url-suffix-post", node2) and
  state1 = "tainted-url-suffix" and
  state2 = "taint"
}

The barrier is used to disable the default behaviour of the split summary (which taints the whole array), and the step then enables flow into the array element at index 1.

At the moment this is hidden in DataFlowPrivate but we could consider exposing the feature more widely in the future.

Evaluation

Evaluation looks good, it fixes 8 FPs and has neutral perf. Since that's based on a slightly older version of the PR, a new run is also underway.

[erik-krogh]

Really nice work again 👍

[erik-krogh]

I don't think this comment is placed at the right place?
Or you meant Argument[this] instead of ArrayElement[0]?

[asgerf]

It refers to the ArrayElement[0] in the summary above here.

ArrayElement is automatically converted to a taint step, but ArrayElement[0] is not, which is why we need this extra summary.

[erik-krogh]

Oh, now I get it.
Maybe a reword like:
// The above doesn't add a taint-step from the array itself to the return value, so we add that manually.