diff --git a/python/ql/lib/change-notes/2023-11-27-tarfile-extraction-filters.md b/python/ql/lib/change-notes/2023-11-27-tarfile-extraction-filters.md new file mode 100644 index 000000000000..e57b7aa9bffe --- /dev/null +++ b/python/ql/lib/change-notes/2023-11-27-tarfile-extraction-filters.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- + +- Added support for tarfile extraction filters as defined in [PEP-706](https://peps.python.org/pep-0706). In particular, calls to `TarFile.extract`, and `TarFile.extractall` are no longer considered to be sinks for the `py/tarslip` query if a sufficiently safe filter is provided. diff --git a/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll b/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll index 1a3df320358a..880fc604b9c4 100644 --- a/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll +++ b/python/ql/lib/semmle/python/security/dataflow/TarSlipCustomizations.qll @@ -55,10 +55,38 @@ module TarSlip { ExcludeTarFilePy() { this.getLocation().getFile().getBaseName() = "tarfile.py" } } + /** + * Holds if `call` has an unsafe extraction filter, either by default (as the default is unsafe), + * or by being set to an explicitly unsafe value, such as `"fully_trusted"`, or `None`. + */ + private predicate hasUnsafeFilter(API::CallNode call) { + call = + API::moduleImport("tarfile") + .getMember("open") + .getReturn() + .getMember(["extract", "extractall"]) + .getACall() and + ( + exists(Expr filterValue | + filterValue = call.getParameter(4, "filter").getAValueReachingSink().asExpr() and + ( + filterValue.(StrConst).getText() = "fully_trusted" + or + filterValue instanceof None + ) + ) + or + not exists(call.getParameter(4, "filter")) + ) + } + /** * A sink capturing method calls to `extractall`. * - * For a call to `file.extractall` without arguments, `file` is considered a sink. + * For a call to `file.extractall`, `file` is considered a sink if + * + * - there are no other arguments, or + * - there are other arguments (except `members`), and the extraction filter is unsafe. */ class ExtractAllSink extends Sink { ExtractAllSink() { @@ -69,8 +97,13 @@ module TarSlip { .getReturn() .getMember("extractall") .getACall() and - not exists(call.getArg(_)) and - not exists(call.getArgByName(_)) and + ( + not exists(call.getArg(_)) and + not exists(call.getArgByName(_)) + or + hasUnsafeFilter(call) + ) and + not exists(call.getArgByName("members")) and this = call.(DataFlow::MethodCallNode).getObject() ) } @@ -84,7 +117,8 @@ module TarSlip { exists(DataFlow::CallCfgNode call | call = API::moduleImport("tarfile").getMember("open").getReturn().getMember("extract").getACall() and - this = call.getArg(0) + this = call.getArg(0) and + hasUnsafeFilter(call) ) } } @@ -99,7 +133,8 @@ module TarSlip { .getReturn() .getMember("extractall") .getACall() and - this in [call.getArg(0), call.getArgByName("members")] + this in [call.getArg(0), call.getArgByName("members")] 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 5041e7eee54e..f0a35a3d14e8 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 @@ -12,6 +12,15 @@ edges | tarslip.py:58:1:58:3 | GSSA Variable tar | tarslip.py:59:5:59:9 | GSSA Variable entry | | tarslip.py:58:7:58:39 | ControlFlowNode for Attribute() | tarslip.py:58:1:58:3 | GSSA Variable tar | | tarslip.py:59:5:59:9 | GSSA Variable entry | tarslip.py:61:21:61:25 | ControlFlowNode for entry | +| tarslip.py:90:1:90:3 | GSSA Variable tar | tarslip.py:91:1:91:3 | ControlFlowNode for tar | +| tarslip.py:90:7:90:39 | ControlFlowNode for Attribute() | tarslip.py:90:1:90:3 | GSSA Variable tar | +| tarslip.py:94:1:94:3 | GSSA Variable tar | tarslip.py:95:5:95:9 | GSSA Variable entry | +| tarslip.py:94:7:94:39 | ControlFlowNode for Attribute() | tarslip.py:94:1:94:3 | GSSA Variable tar | +| tarslip.py:95:5:95:9 | GSSA Variable entry | tarslip.py:96:17:96:21 | ControlFlowNode for entry | +| tarslip.py:109:1:109:3 | GSSA Variable tar | tarslip.py:110:1:110:3 | ControlFlowNode for tar | +| tarslip.py:109:7:109:39 | ControlFlowNode for Attribute() | tarslip.py:109:1:109:3 | GSSA Variable tar | +| tarslip.py:112:1:112:3 | GSSA Variable tar | tarslip.py:113:24:113:26 | ControlFlowNode for tar | +| tarslip.py:112:7:112:39 | ControlFlowNode for Attribute() | tarslip.py:112:1:112:3 | GSSA Variable tar | nodes | tarslip.py:14:1:14:3 | GSSA Variable tar | semmle.label | GSSA Variable tar | | tarslip.py:14:7:14:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | @@ -31,6 +40,19 @@ nodes | tarslip.py:58:7:58:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | | tarslip.py:59:5:59:9 | GSSA Variable entry | semmle.label | GSSA Variable entry | | tarslip.py:61:21:61:25 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| tarslip.py:90:1:90:3 | GSSA Variable tar | semmle.label | GSSA Variable tar | +| tarslip.py:90:7:90:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| tarslip.py:91:1:91:3 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | +| tarslip.py:94:1:94:3 | GSSA Variable tar | semmle.label | GSSA Variable tar | +| tarslip.py:94:7:94:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| tarslip.py:95:5:95:9 | GSSA Variable entry | semmle.label | GSSA Variable entry | +| tarslip.py:96:17:96:21 | ControlFlowNode for entry | semmle.label | ControlFlowNode for entry | +| tarslip.py:109:1:109:3 | GSSA Variable tar | semmle.label | GSSA Variable tar | +| tarslip.py:109:7:109:39 | ControlFlowNode for Attribute() | semmle.label | ControlFlowNode for Attribute() | +| tarslip.py:110:1:110:3 | ControlFlowNode for tar | semmle.label | ControlFlowNode for tar | +| tarslip.py:112:1:112:3 | GSSA Variable tar | semmle.label | GSSA Variable 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 | subpaths #select | tarslip.py:15:1:15:3 | ControlFlowNode for tar | tarslip.py:14:7:14:39 | ControlFlowNode for Attribute() | tarslip.py:15:1:15:3 | ControlFlowNode for tar | This file extraction depends on a $@. | tarslip.py:14:7:14:39 | ControlFlowNode for Attribute() | potentially untrusted source | @@ -38,3 +60,7 @@ subpaths | tarslip.py:39:17:39:21 | ControlFlowNode for entry | tarslip.py:35:7:35:39 | ControlFlowNode for Attribute() | tarslip.py:39:17:39:21 | ControlFlowNode for entry | This file extraction depends on a $@. | tarslip.py:35:7:35:39 | ControlFlowNode for Attribute() | potentially untrusted source | | tarslip.py:43:24:43:26 | ControlFlowNode for tar | tarslip.py:42:7:42:39 | ControlFlowNode for Attribute() | tarslip.py:43:24:43:26 | ControlFlowNode for tar | This file extraction depends on a $@. | tarslip.py:42:7:42:39 | ControlFlowNode for Attribute() | potentially untrusted source | | tarslip.py:61:21:61:25 | ControlFlowNode for entry | tarslip.py:58:7:58:39 | ControlFlowNode for Attribute() | tarslip.py:61:21:61:25 | ControlFlowNode for entry | This file extraction depends on a $@. | tarslip.py:58:7:58:39 | ControlFlowNode for Attribute() | potentially untrusted source | +| tarslip.py:91:1:91:3 | ControlFlowNode for tar | tarslip.py:90:7:90:39 | ControlFlowNode for Attribute() | tarslip.py:91:1:91:3 | ControlFlowNode for tar | This file extraction depends on a $@. | tarslip.py:90:7:90:39 | ControlFlowNode for Attribute() | potentially untrusted source | +| 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 | 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 3a009481961c..b9ca0e8e9440 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 @@ -82,3 +82,35 @@ def safemembers(members): for entry in tar: if not os.path.isabs(entry.name): tar.extract(entry, "/tmp/unpack/") + +# Extraction filters + +extraction_filter = "fully_trusted" + +tar = tarfile.open(unsafe_filename_tar) +tar.extractall(filter=extraction_filter) # unsafe +tar.close() + +tar = tarfile.open(unsafe_filename_tar) +for entry in tar: + tar.extract(entry, filter=extraction_filter) # unsafe + +extraction_filter = "data" + +tar = tarfile.open(unsafe_filename_tar) +tar.extractall(filter=extraction_filter) # safe +tar.close() + +tar = tarfile.open(unsafe_filename_tar) +for entry in tar: + tar.extract(entry, filter=extraction_filter) # safe + +extraction_filter = None +tar = tarfile.open(unsafe_filename_tar) +tar.extractall(filter=extraction_filter) # unsafe + +tar = tarfile.open(unsafe_filename_tar) +tar.extractall(members=tar, filter=extraction_filter) # unsafe + +tar = tarfile.open(unsafe_filename_tar) +tar.extractall(members=safemembers(tar), filter=extraction_filter) # safe -- we assume `safemembers` makes up for the unsafe filter