codeql icon indicating copy to clipboard operation
codeql copied to clipboard

Python: remove the imprecise container taint steps

Open yoff opened this issue 1 year ago • 0 comments
trafficstars

We used to have taint steps from any element of a collection to the entire collection (see here). These are very imprecise, leading to false positives (e.g. seen here and here). They are also at odds with how other languages treat collections, see our issue about this.

We wish to keep the semantics, that if a collection is tainted, then all elements are considered tainted. Therefor we now try to not taint collections, if we have precise information about which elements are tainted. For a list, if an element is tainted, we do not know which one, so any read is potentially reading tainted information. There is not much difference between the list having content and the list being tainted. But for a dictionary, if an entry is tainted and we know which one, only reads of the appropriate key is reading tainted information. All other reads should ideally be considered safe (they used to not be). If we do not know that other keys are safe, e.g. if the collection came from an untrusted source, we can taint the collection itself, and all reads will be considered dangerous. So for collections with precise content, there is a big difference between having content and the collection being tainted.

Thus we wish to remove these imprecise taint steps for tuples and dictionaries, where we track content precisely (we keep them for lists and sets, where content is imprecise anyway). This PR now seems to demonstrate that we can achieve this, although with some caveats:

  • we use implicit reads, which makes reasoning about use/use-flow and sinks a bit complicated
  • we need a solution to additional flow steps for conversions

The issue with conversions is as follows: We are moving away from tainting collections when only precise content is tainted. But some operations may read any of the collection elements, for instance decoders. A call like

tainted_obj = {"foo": TAINTED_STRING}
encoded = ujson.dumps(tainted_obj)

used to transfer taint from tainted_obj to encoded, via an additional taint step. But now there is no taint to transfer because tainted_obj itself is not tainted. Instead, it has to make a read step. Adding read steps is not trivial, though, the best mechanism we have is that of flow summaries, but it is awkward to use here, or two reasons:

  1. We do not actually collect decoder calls, but rather their input and output, whereas flow summaries are formulated in terms of calls and access paths.
  2. Non-monotonic recursion

We might also be able to get the converter reads as implicit reads, but there is currently no mechanism for doing that for all taint flow configurations at once.

yoff avatar Jul 22 '24 14:07 yoff