From 68991eb29460d91625e8918939a42144920d5fbf Mon Sep 17 00:00:00 2001 From: Sam Robson Date: Thu, 12 Nov 2026 16:36:07 +0000 Subject: [PATCH] fix(python): match argument positions and parameter names in py/tarslip sinks --- .../2026-11-12-tarslip-sink-arguments.md | 4 ++++ .../security/dataflow/TarSlipCustomizations.qll | 10 +++++----- .../Security/CWE-022-TarSlip/TarSlip.expected | 14 ++++++++++++++ .../Security/CWE-022-TarSlip/tarslip.py | 12 ++++++++++++ 4 files changed, 35 insertions(+), 5 deletions(-) create mode 100644 python/ql/lib/change-notes/2026-11-12-tarslip-sink-arguments.md diff --git a/python/ql/lib/change-notes/2026-11-12-tarslip-sink-arguments.md b/python/ql/lib/change-notes/2026-11-12-tarslip-sink-arguments.md new file mode 100644 index 000000000000..1bffc50e7701 --- /dev/null +++ b/python/ql/lib/change-notes/2026-11-12-tarslip-sink-arguments.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* The `py/tarslip` query now uses the correct argument positions and parameter names for `TarFile.extract` and `TarFile.extractall` sinks, reporting `extract(member=...)` calls with untrusted members and no longer falsely reporting `extractall(path, members)` calls when sanitized `members` are passed positionally. diff --git a/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll index 2dbe2c542aee..675f48a9442a 100644 --- a/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll @@ -96,7 +96,7 @@ module TarSlip { .getMember("extractall") .getACall() and hasUnsafeFilter(call) and - not exists(call.getParameter(2, "members")) and + not exists(call.getParameter(1, "members")) and this = call.(DataFlow::MethodCallNode).getObject() ) } @@ -107,10 +107,10 @@ module TarSlip { */ class ExtractSink extends Sink { ExtractSink() { - exists(DataFlow::CallCfgNode call | + exists(API::CallNode call | call = API::moduleImport("tarfile").getMember("open").getReturn().getMember("extract").getACall() and - this = call.getArg(0) and + this = call.getParameter(0, "member").asSink() and hasUnsafeFilter(call) ) } @@ -119,14 +119,14 @@ module TarSlip { /** The `members` argument `extractall` is considered a sink. */ class ExtractMembersSink extends Sink { ExtractMembersSink() { - exists(DataFlow::CallCfgNode call | + exists(API::CallNode call | call = API::moduleImport("tarfile") .getMember("open") .getReturn() .getMember("extractall") .getACall() and - this in [call.getArg(0), call.getArgByName("members")] and + this = call.getParameter(1, "members").asSink() and hasUnsafeFilter(call) ) } diff --git a/python/ql/test/query-tests/Security/CWE-022-TarSlip/TarSlip.expected b/python/ql/test/query-tests/Security/CWE-022-TarSlip/TarSlip.expected index abdccddd631b..734b85d69236 100644 --- a/python/ql/test/query-tests/Security/CWE-022-TarSlip/TarSlip.expected +++ b/python/ql/test/query-tests/Security/CWE-022-TarSlip/TarSlip.expected @@ -8,6 +8,8 @@ | tarslip.py:96:17:96:21 | ControlFlowNode for entry | tarslip.py:94:7:94:39 | ControlFlowNode for Attribute() | tarslip.py:96:17:96:21 | ControlFlowNode for entry | This file extraction depends on a $@. | tarslip.py:94:7:94:39 | ControlFlowNode for Attribute() | potentially untrusted source | | tarslip.py:110:1:110:3 | ControlFlowNode for tar | tarslip.py:109:7:109:39 | ControlFlowNode for Attribute() | tarslip.py:110:1:110:3 | ControlFlowNode for tar | This file extraction depends on a $@. | tarslip.py:109:7:109:39 | ControlFlowNode for Attribute() | potentially untrusted source | | tarslip.py:113:24:113:26 | ControlFlowNode for tar | tarslip.py:112:7:112:39 | ControlFlowNode for Attribute() | tarslip.py:113:24:113:26 | ControlFlowNode for tar | This file extraction depends on a $@. | tarslip.py:112:7:112:39 | ControlFlowNode for Attribute() | potentially untrusted source | +| tarslip.py:121:24:121:28 | ControlFlowNode for entry | tarslip.py:119:7:119:39 | ControlFlowNode for Attribute() | tarslip.py:121:24:121:28 | ControlFlowNode for entry | This file extraction depends on a $@. | tarslip.py:119:7:119:39 | ControlFlowNode for Attribute() | potentially untrusted source | +| tarslip.py:125:32:125:34 | ControlFlowNode for tar | tarslip.py:124:7:124:39 | ControlFlowNode for Attribute() | tarslip.py:125:32:125:34 | ControlFlowNode for tar | This file extraction depends on a $@. | tarslip.py:124:7:124:39 | ControlFlowNode for Attribute() | potentially untrusted source | edges | tarslip.py:14:1:14:3 | ControlFlowNode for tar | tarslip.py:15:1:15:3 | ControlFlowNode for tar | provenance | | | tarslip.py:14:7:14:39 | ControlFlowNode for Attribute() | tarslip.py:14:1:14:3 | ControlFlowNode for tar | provenance | | @@ -31,6 +33,11 @@ edges | tarslip.py:109:7:109:39 | ControlFlowNode for Attribute() | tarslip.py:109:1:109:3 | ControlFlowNode for tar | provenance | | | tarslip.py:112:1:112:3 | ControlFlowNode for tar | tarslip.py:113:24:113:26 | ControlFlowNode for tar | provenance | | | tarslip.py:112:7:112:39 | ControlFlowNode for Attribute() | tarslip.py:112:1:112:3 | ControlFlowNode for tar | provenance | | +| tarslip.py:119:1:119:3 | ControlFlowNode for tar | tarslip.py:120:5:120:9 | ControlFlowNode for entry | provenance | | +| tarslip.py:119:7:119:39 | ControlFlowNode for Attribute() | tarslip.py:119:1:119:3 | ControlFlowNode for tar | provenance | | +| tarslip.py:120:5:120:9 | ControlFlowNode for entry | tarslip.py:121:24:121:28 | ControlFlowNode for entry | provenance | | +| tarslip.py:124:1:124:3 | ControlFlowNode for tar | tarslip.py:125:32:125:34 | ControlFlowNode for tar | provenance | | +| tarslip.py:124:7:124:39 | ControlFlowNode for Attribute() | tarslip.py:124:1:124:3 | ControlFlowNode for tar | provenance | | nodes | tarslip.py:14:1:14:3 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | | tarslip.py:14:7:14:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | @@ -63,4 +70,11 @@ nodes | tarslip.py:112:1:112:3 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | | tarslip.py:112:7:112:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | tarslip.py:113:24:113:26 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | +| tarslip.py:119:1:119:3 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | +| tarslip.py:119:7:119:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| tarslip.py:120:5:120:9 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| tarslip.py:121:24:121:28 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| tarslip.py:124:1:124:3 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | +| tarslip.py:124:7:124:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| tarslip.py:125:32:125:34 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | subpaths diff --git a/python/ql/test/query-tests/Security/CWE-022-TarSlip/tarslip.py b/python/ql/test/query-tests/Security/CWE-022-TarSlip/tarslip.py index b2614b8a6d11..de87f02351fe 100644 --- a/python/ql/test/query-tests/Security/CWE-022-TarSlip/tarslip.py +++ b/python/ql/test/query-tests/Security/CWE-022-TarSlip/tarslip.py @@ -114,3 +114,15 @@ def safemembers(members): tar = tarfile.open(unsafe_filename_tar) tar.extractall(members=safemembers(tar), filter=extraction_filter) # safe -- we assume `safemembers` makes up for the unsafe filter + +# Keyword `member` argument to `extract` +tar = tarfile.open(unsafe_filename_tar) # $ Source +for entry in tar: + tar.extract(member=entry, path="/tmp/unpack/") # $ Alert + +# Positional `members` argument to `extractall` +tar = tarfile.open(unsafe_filename_tar) # $ Source +tar.extractall("/tmp/unpack/", tar) # $ Alert + +tar = tarfile.open(unsafe_filename_tar) +tar.extractall("/tmp/unpack/", safemembers(tar)) # safe