GeezerVison

All commit authors signed the Contributor License Agreement.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

FYI These are the whitespace changes that should be reverted:

I have made the requested changes; please review again

Thanks for making the requested changes!

@AA-Turner: please review the changes made to this pull request.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

I thought about this a lot, because I originally didn't want un-necessary churn, even if it's helpful. However, it's actually possible for the user to skip the download part and directly specify a directory where the lib sources are. So, assuming that this directory is untrusted, it's entirely possible to have a bad archive out there. The attack surface is really small as this requires executing the script in a non-standard way, but I think it's a legitimate improvement.

I need to test it locally though, (or @ParzivalHack, could you check that this effectively prevents the PT attack and share your PoC please?).

I thought about this a lot, because I originally didn't want un-necessary churn, even if it's helpful. However, it's actually possible for the user to skip the download part and directly specify a directory where the lib sources are. So, assuming that this directory is untrusted, it's entirely possible to have a bad archive out there. The attack surface is really small as this requires executing the script in a non-standard way, but I think it's a legitimate improvement.

I need to test it locally though, (or @ParzivalHack, could you check that this effectively prevents the PT attack and share your PoC please?).

Hey @picnixz,
Thanks for your feedback and for looking into this (again). I've re-tested the fix locally and can confirm that using the filter='data' argument fully prevents the path traversal attack.

I've put together a small (but detailed) repo with the PoC script, including a README.md that shows a side-by-side comparison of the vulnerable and patched outcomes.

You can check it out here: https://github.com/ParzivalHack/PT-PoC

Lmk if there's anything else I can do👍

Thank you for the PoC. Actually, I just looked at the docs for tarfile, but apparently, the data filter is the default filter for Python 3.14 and later. Which version are you using for Python? with Python 3.14+, extractall() shouldn't need to specify its extraction filter.

Thank you for the PoC. Actually, I just looked at the docs for tarfile, but apparently, the data filter is the default filter for Python 3.14 and later. Which version are you using for Python? with Python 3.14+, extractall() shouldn't need to specify its extraction filter.

No worries. And i was running everything with Python 3.13.3👍

So that's the reason why it's vulnerable. However, as I mentioned on the issue, if someone is using Python 3.13 and below for PYTHON_FOR_REGEN, then they could suffer from this so I'm ok with this change.

So that's the reason why it's vulnerable. However, as I mentioned on the issue, if someone is using Python 3.13 and below for PYTHON_FOR_REGEN, then they could suffer from this so I'm ok with this change.

Hey @picnixz,
Thanks for your review and feedback. Appreciate your help in getting this resolved.

Thanks again for the opportunity to contribute :)

Thanks @ParzivalHack for the PR, and @picnixz for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

GH-138262 is a backport of this pull request to the 3.14 branch.

GH-138263 is a backport of this pull request to the 3.13 branch.