General issue - JavaScript data flow analysis

[gdemarcsek]

Description of the issue
The following DOM XSS vector is recognized nicely (CWE-079):

function test() {
    let x = new URLSearchParams(location.search).get('x');
    document.getElementById("sink").innerHTML = x;
}

But this one is not recognized:

function test() {
    [document.getElementById("sink")][0].innerHTML = new URLSearchParams(location.search).get('x');
}

Is this a limitation of the JS data flow analysis library, generic limitation of the methodology or just an uncovered scenario (e.g. not propagating taint through array literals to their elements?)

My original test case was more complicated and a bit more realistic (like I could imagine similar code to be written IRL):

function f(e) {
    e.innerHTML = new URLSearchParams(location.search).get('x');
}
[document.getElementById("sink")].map(f);

But this didn't work so I started to simplify. Same for a forEach + arrow function.

[hmakholm]

@github/codeql-javascript might have some input here.

[erik-krogh]

I would say it's an uncovered scenario.

In DOM.qll we track which values are DOM nodes.
We don't use the full featured data flow analysis to track these DOM nodes, we instead use a more lightweight approach we call type tracking.
Getting type-tracking to support your original test case would only require a small change to the ArrayCreationStep class.
Specifically, changing ArrayCreationStep to the below would support your original test case.

private class ArrayCreationStep extends PreCallGraphStep {
  override predicate storeStep(DataFlow::Node element, DataFlow::SourceNode obj, string prop) {
    exists(DataFlow::ArrayCreationNode arr, int i |
      element = arr.getElement(i) and
      obj = arr and
      if arr = any(PromiseAllCreation c).getArrayNode()
      then prop = arrayElement(i)
      else prop = arrayElement()
    )
  }
}

However, this might introduce false positives if an array contain elements of different types.

But it's something I think is worth investigating.

[github-actions]

This issue is stale because it has been open 14 days with no activity. Comment or remove the stale label in order to avoid having this issue closed in 7 days.

[erik-krogh]

But it's something I think is worth investigating.

I've now merged the above suggestion as part of another PR.
So the scenario you describe is now supported.