Python: Do the py/tarslip sinks use the wrong argument positions for TarFile.extract and TarFile.extractall? #23114

Description of the issue

I'm wondering whether the argument positions used by the py/tarslip sinks are intended, because they don't seem to match TarFile.extract(member, path="", set_attrs=True, *, numeric_owner=False, filter=None) and TarFile.extractall(path=".", members=None, *, numeric_owner=False, filter=None). As a result, tar.extract(member=entry, path=dest) is not reported, although tar.extract(entry, path=dest) is, and tar.extractall(dest, safe_members(tar)) is reported even when safe_members filters the members, although tar.extractall(dest, members=safe_members(tar)) is not.

Affected sinks

• ExtractSink only uses call.getArg(0), so a member passed as member=... is never a sink.
• ExtractAllSink reports the receiver when not exists(call.getParameter(2, "members")). As far as I can tell, API::CallNode.getParameter(i) is getArg(i), which does not count self, so a positional members is argument 1 and argument 2 never exists in a valid call. A positional members is therefore treated as if there were none.
• ExtractMembersSink uses call.getArg(0), which for extractall is path, not members.
• hasUnsafeFilter uses getParameter(4, "filter"). If self is counted, 2 and 4 are exactly the positions of members and filter in extractall(self, path, members, *, numeric_owner, filter). For filter this is harmless because it is keyword-only.

The members index came with #15051. The query tests only pass members= by keyword and the member to extract positionally, so they don't cover these forms.

Steps to reproduce

Not reported (false negative):

import sys
import tarfile

with tarfile.open(sys.argv[1]) as tar:
    for entry in tar:
        tar.extract(member=entry, path="/tmp/unpack/")

Reported, for comparison: the same file with tar.extract(entry, path="/tmp/unpack/"). Adding filter="fully_trusted" to the keyword form is also not reported, while the positional form with that filter is reported (as in the query test tar.extract(entry, filter=extraction_filter)).

Reported (false positive), at the receiver tar in the last line:

import os
import sys
import tarfile


def safe_members(tar):
    for info in tar:
        if info.issym() or info.islnk():
            continue
        if os.path.isabs(info.name) or ".." in info.name:
            continue
        yield info


with tarfile.open(sys.argv[1]) as tar:
    tar.extractall("/tmp/unpack/", safe_members(tar))

Not reported, for comparison: the same file with members=safe_members(tar). python-security-extended.qls gives the same results as py/tarslip alone.

I ran each file unchanged except for the destination directory. The extract files got an archive with ok.txt and ../dotdot.txt. The extractall files got an archive that also contains a symbolic link link → .. followed by link/pwned.txt, and a hard link hl → ../victim.txt followed by a regular member hl.

So the keyword form behaves exactly like the reported positional form, and the filtered members passed positionally behaves exactly like the unreported keyword form (nothing is written outside dest either way). The keyword form is not unusual: a GitHub code search for "extract(member=" language:python tarfile currently finds about 390 files.

Question

Are these argument positions intended? Would it be reasonable to use getParameter(0, "member") in ExtractSink, and position 1 for members in both ExtractAllSink and ExtractMembersSink (so that an unfiltered positional members stays reported once the receiver is no longer)?

Environment

• CodeQL CLI 2.27.4 (codeql/python-queries 1.9.0); CodeQL CLI 2.27.2 and 2.15.4 give the same results
• CodeQL repository commit for the links above: fbf9fca5ce7c3b64c2f5d719ea0444577850cb4a (TarSlipCustomizations.qll and ApiGraphs.qll are identical in 2.27.4 and this commit)
• Python 3.10.12, 3.12.3 (Ubuntu), 3.13.16 and 3.14.8 on Linux/amd64