Empty `location` property for network taint operations

[eleumasc]

The location property is empty for the fetch.text(), fetch.json(), and WebSocket.MessageEvent.data taint operations.

Expected

{
  "operation": "fetch.text()",
  "builtin": true,
  "source": true,
  "location": {
    "filename": "https://example.org/script.js",
    "function": "f",
    "line": 1,
    "pos": 15,
    "scriptline": 1,
    "scripthash": "35a4393d72afbf6d792d3b0e46f465b6"
  },
  "arguments": []
}

Actual

{
  "operation": "fetch.text()",
  "builtin": true,
  "source": true,
  "location": {
    "filename": "",
    "function": "",
    "line": 0,
    "pos": 0,
    "scriptline": 0,
    "scripthash": "00000000000000000000000000000000"
  },
  "arguments": []
}

Likely related, in some instances of XMLHttpRequest.response taint operations, the location property is empty and the builtin property is false (it should be true).

Example

{
  "operation": "XMLHttpRequest.response",
  "builtin": false,
  "source": true,
  "location": {
    "filename": "",
    "function": "",
    "line": 0,
    "pos": 0,
    "scriptline": 0,
    "scripthash": "00000000000000000000000000000000"
  },
  "arguments": []
}

[leeN]

The builtin property is something I'd like to get rid of soonish in general. This specific case looks like a bug I introduced when reenabling end-to-end tainting, so I'll try to have a look this/next week. This looks really wrong.

Do you have a small reproducer to test this behavior? In case GitHub does not like uploading the files, feel free to send me an email (david.klein [at] tu-braunschweig.de) or open a PR with a mochitest, heh!

[eleumasc]

I have some updates on this issue.

The bug involving XMLHttpRequest.response seems to be related with taint propagation through dynamically evaluated code. For example, the following code reproduces the bug:

// index.js
const xhr = new XMLHttpRequest();
xhr.open("GET", "other.js");
xhr.onload = () => {
  if (xhr.readyState === xhr.DONE) {
    if (xhr.status === 200) {
      window.eval(xhr.responseText);
    }
  }
};
xhr.send();

// other.js
console.log("hello".taint);

The taint logged by other.js includes the following taint operation:

{
  "operation": "XMLHttpRequest.response",
  "builtin": false,
  "source": true,
  "location": {
    "filename": "",
    "function": "",
    "line": 0,
    "pos": 0,
    "scriptline": 0,
    "scripthash": "00000000000000000000000000000000"
  },
  "arguments": []
}

Hence, I argue that the solution must be searched somewhere in the taint propagation patches applied to the JS parser.

Regarding the related bug involving fetch.text() and fetch.json(), my current hypothesis is that the taint location is computed using the context in a position where the stack trace is not accessible, because the BodyConsumer is working in a separate thread. If that would be the case, a possible solution may be to compute the taint location object at the time of call to response.text()/response.json() and not when the string to be tainted is created, and passing that information to the BodyConsumer object.

Finally, I haven't investigated on the related bug involving WebSocket.MessageEvent.data yet.

[leeN]

Hmmm, that all sounds pretty sensible! That missing locations happen off threat was what I wanted to look into as well, but that seems like a decent explanation. And your suggestion to fix it sounds reasonable as well :)

Do you have time for a closer look/looking for a patch? We will stay on this version for a while (probably until early December, as this is the last version prior to some big internal changes, see #289 and we are looking to have a stable version for our BlackHat presentation in December..), so there shouldn't be any blockers with huge version updates you'd have to merge.

[eleumasc]

I can try, solving this issue could be necessary for my research too after all.

[eleumasc]

I have discovered that the cause of the bug involving XMLHttpRequest.response is that the taint is propagated using ParseTaint, which is probably related to the E2E tainting stuff, and the serialized taint string does not include all fields of the original object (among which location). Could we enhance the taint serialization method so that the taint string includes all fields of the taint object?

Also, I see that the JSON string is parsed using a custom parser rather than the builtin parser. I think that using the last one would be safer.

[leeN]

Ah, that makes a ton of sense. The empty location constructor was introduced due to the end-to-end stuff. The issue is that if you receive an X-Taint header, no location is transmitted, so an empty location is sensible here. But that should not affect non-e2e cases.

And yes, unifying the JSON parsing is on my to-do list for the next two months. If you want to look into this, there is some code in #275 you could use; it serializes the full taint object. That PR has some side effects that are not sufficiently well understood, e.g., we receive taints from the browser chrome, for example, and unclear performance impact as postMessages are reasonably common. But you can cherry-pick parts of the commit to perform the JSON (de)serialization :)