[SPARK-57962][PYTHON] Guard against path traversal in install_spark tar extraction#57040
[SPARK-57962][PYTHON] Guard against path traversal in install_spark tar extraction#57040peter-toth wants to merge 4 commits into
Conversation
…ar extraction ### What changes were proposed in this pull request? `install_spark()` in `python/pyspark/install.py` downloads a Spark release archive and extracts it by rewriting each tar member's name with `os.path.relpath(member.name, package_name + os.path.sep)` and calling `tar.extract(member, dest)`. `os.path.relpath` does not strip `..` segments, so a crafted archive member could resolve to a path outside `dest` (a "zip slip" / path traversal) when extracted. This change extracts the loop into a helper `_extract_tar` that validates each member's resolved destination stays within `dest` before extracting, raising `ValueError` otherwise. The behavior for legitimate archives is unchanged. ### Why are the changes needed? `install_spark()` runs at `pip install pyspark` time (when `PYSPARK_HADOOP_VERSION`/`PYSPARK_HIVE_VERSION` is set) and downloads from an Apache mirror or a user-supplied `PYSPARK_RELEASE_MIRROR`. Extracting archive members without a containment check means a member path containing `..` would be written outside the intended destination directory. Adding an explicit resolved-path check is a straightforward robustness improvement. The check is portable across all supported Python versions (the built-in `tarfile` extraction `filter='data'` is only available on 3.12+, while Spark supports 3.11+). ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit test `test_extract_tar` in `python/pyspark/tests/test_install_spark.py` that (1) extracts a benign archive with the top-level package directory stripped, and (2) asserts a member whose path escapes the destination directory is rejected and nothing is written. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8)
dongjoon-hyun
left a comment
There was a problem hiding this comment.
Could you double-check this of the PR description, @peter-toth ? AFAIK, data filter is backported to Python 3.11.4, 3.10.12, 3.9.12. Given that, this PR had better use data filter.
the built-in tarfile extraction filter='data' is only available on 3.12+, while Spark supports 3.11+
Address review feedback: the tarfile 'data' filter (PEP 706) is backported to Python 3.11.4 / 3.10.12 / 3.9.17, so it is available on all Spark-supported versions. Use it in _extract_tar instead of a manual resolved-path containment check; besides path traversal it also rejects absolute paths, unsafe links, and special files. Update the test to expect tarfile.OutsideDestinationError.
|
Thanks @dongjoon-hyun, you're right - I confirmed |
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM. Thank you, @peter-toth .
|
We can't do this unconditionally just because the latest 3.11.x supports it. For python users, it's very common to stay at a lower micro level (Python does not auto-upgrade). For those users, this will become an installation failure. They wouldn't know how to fix it. They would just blame spark. We need to either
|
In general, you are right. However, I'm not sure that is still a valid assumption for this improvement JIRA issue for Apache Spark 5.0.0 scheduled in 2027. Could you elaborate the mentioned comment environments a little more, please, @gaogaotiantian ? IIRC, Python 3.11.4, 3.10.12, 3.9.17 were released on 2023-06-06. It's already over 3 years and one month ago. |
|
If this is for 5.0.0 rather than 4.3.0, it might be much safer. 3.11 will be retired on Oct. 2027. My concern is that for some Python users, they download their minor Python version (3.11) once and never upgrade it. It's not that uncommon for Python users to stay at a certain version. For those users, when they upgrade pyspark version, they'll hit an error about This may not impact a huge user base, but it's still unnecessary. I think a simple version check |
|
May I ask if it's only your imagination or you are trying to refer some real production environments?
To be honest with you, I don't think it's a usable or real production system, @gaogaotiantian . If you are aware of any systems, you had better inform them to upgrade Python first instead of trying install new Spark there. That's the Python and Apache Spark community recommendation. For example, let's say that they are using Correctness/Crash/Hang Patches Python 3.11.4 (June 6, 2023)
Python 3.11.5 (August 24, 2023)
Python 3.11.6 (October 2, 2023)
Python 3.11.7 (December 4, 2023)
Python 3.11.8 (February 6, 2024)
Python 3.11.9 (April 2, 2024)
CVE Patches Python 3.11.4 (June 6, 2023)
Python 3.11.5 (August 24, 2023)
Python 3.11.8 (February 6, 2024)
Python 3.11.9 (April 2, 2024)
Python 3.11.10 (September 7, 2024)
Python 3.11.11 (December 3, 2024)
Python 3.11.12 (April 8, 2025)
Python 3.11.14 (October 2025)
Python 3.11.15 (March 3, 2026) — latest as of today
|
|
Python 3.8 has been end of life for more than 2 years yet we still ran tests with it at least until a few weeks ago. We still use Python 3.9 as the major python version to test spark 3.5 and it has been EOL for about a year too. I can list a bunch of unpatched CVEs for Python 3.9 too because it's not maintained anymore. It's not my imagination that Apache Spark is still using it in their github actions. I won't be surprised at all if there are some users that have a pinned Python version that's old because that happens to be the version when they pinned it. It's about what we claim to support and what we do support. I'm okay if we say in our documentation that we only support Python 3.11 that's greater than 3.11.4 - that's fine. But if we claim to support 3.11, we should not have something in our install script that won't work in 3.11.0. More importantly, what do we lose if we check the version? Would it be a worse experience for users if we generate an exception with a better explanation of what's happening and how to deal with it? Will the code be less maintainable with the extra if statement which could be easily removed when we do not support 3.11 anymore? What's the reason not to do the safety check when we know that there are cases where this won't work? Because we are absolutely sure that there is no user out there that's using old Python version? Or we should not care about them because they are not serious users that upgrade their Python version frequently? |
|
How about merging the previous version (4435bbd) of the PR to all active branches now and once we declared minimum version >= 3.12 we upgrade the code to its current form? |
|
I'm curious about the original issue. You mentioned that I tested it on my machine >>> os.path.realpath("/usr/../bin")
'/bin'So what's the issue? How to reproduce this problem? |
|
@gaogaotiantian the snippet tests >>> os.path.relpath("spark-x/../../etc/evil", "spark-x/")
'../../etc/evil'So import io, os, tarfile, tempfile
with tempfile.TemporaryDirectory() as tmp:
dest = os.path.join(tmp, "dest"); os.makedirs(dest)
p = os.path.join(tmp, "m.tar")
with tarfile.open(p, "w") as t:
info = tarfile.TarInfo("spark-x/../evil.txt"); info.size = 5
t.addfile(info, io.BytesIO(b"pwned"))
with tarfile.open(p) as t:
for m in t.getmembers():
m.name = os.path.relpath(m.name, "spark-x" + os.path.sep)
t.extract(m, dest)
print(os.path.exists(os.path.join(tmp, "evil.txt"))) # True -> escaped dest
|
|
@peter-toth oh it's my mistake. Did not realize that's |
dongjoon-hyun
left a comment
There was a problem hiding this comment.
For this one, it seems that I didn't add a value here. I dropped my previous comment and leave it to you, @peter-toth and @gaogaotiantian . Feel free to proceed in your ways.
…xtraction" This reverts commit 6aa2071.
…r the manual guard tarfile's filter='data' (PEP 706) rejects path-traversal members natively and would replace the manual containment check, but it is only generally available from Python 3.12.0 (backported to 3.11.4+). Add a note to revisit once Spark's minimum supported Python is >= 3.12.
|
I've reverted the Agreed |
|
I think we can keep it this way for both master and branch-4.x for now. spark 5.0 is supposed to be released next year and it's possible that we still want to support python 3.11 for spark 5.0. The missing candidate pool would be really small (just 3.11.0-3.11.3) then but it's still there. The ideal way is to get rid of it when we drop 3.11. The current implementation might not be the cleanest, but it's not horrible either. Just a few extra lines of code. We also have a really nice note there for the next step in the future so even if all of us forgot about it there will be someone to pick it up and it would be easy. My suggestion for now is to keep the master and branch-4.x sync. |
|
@gaogaotiantian , @dongjoon-hyun may I get an approval for the current state? |
…ar extraction ### What changes were proposed in this pull request? `install_spark()` in `python/pyspark/install.py` downloads a Spark release archive and extracts it by rewriting each tar member's name with `os.path.relpath(member.name, package_name + os.path.sep)` and calling `tar.extract(member, dest)`. `os.path.relpath` does not strip `..` segments (it can produce them), so a crafted archive member could resolve to a path outside `dest` (a "zip slip" / path traversal) when extracted. This change extracts the loop into a helper `_extract_tar` that, before extracting each member, resolves its destination (`os.path.realpath(os.path.join(dest, member.name))`) and rejects any member that would land outside `dest`. The behavior for legitimate archives is unchanged. tarfile's `filter='data'` (PEP 706) rejects such members natively and would replace this manual check, but it is only generally available from Python 3.12.0 (backported to 3.11.4+), so we keep the explicit check while Spark still supports Python 3.11, and leave a note to revisit once the minimum supported Python is >= 3.12. ### Why are the changes needed? `install_spark()` runs at `pip install pyspark` time (when `PYSPARK_HADOOP_VERSION`/`PYSPARK_HIVE_VERSION` is set) and downloads from an Apache mirror or a user-supplied `PYSPARK_RELEASE_MIRROR`. Extracting archive members without a containment check means a member path containing `..` would be written outside the intended destination directory. Adding an explicit resolved-path check is a straightforward robustness improvement that works on all supported Python versions. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit test `test_extract_tar` in `python/pyspark/tests/test_install_spark.py` that (1) extracts a benign archive with the top-level package directory stripped, and (2) asserts a member whose path escapes the destination directory is rejected and nothing is written. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8) Closes #57040 from peter-toth/SPARK-57962-install-spark-path-traversal-guard. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com> (cherry picked from commit d61f250) Signed-off-by: Peter Toth <peter.toth@gmail.com>
…ar extraction ### What changes were proposed in this pull request? `install_spark()` in `python/pyspark/install.py` downloads a Spark release archive and extracts it by rewriting each tar member's name with `os.path.relpath(member.name, package_name + os.path.sep)` and calling `tar.extract(member, dest)`. `os.path.relpath` does not strip `..` segments (it can produce them), so a crafted archive member could resolve to a path outside `dest` (a "zip slip" / path traversal) when extracted. This change extracts the loop into a helper `_extract_tar` that, before extracting each member, resolves its destination (`os.path.realpath(os.path.join(dest, member.name))`) and rejects any member that would land outside `dest`. The behavior for legitimate archives is unchanged. tarfile's `filter='data'` (PEP 706) rejects such members natively and would replace this manual check, but it is only generally available from Python 3.12.0 (backported to 3.11.4+), so we keep the explicit check while Spark still supports Python 3.11, and leave a note to revisit once the minimum supported Python is >= 3.12. ### Why are the changes needed? `install_spark()` runs at `pip install pyspark` time (when `PYSPARK_HADOOP_VERSION`/`PYSPARK_HIVE_VERSION` is set) and downloads from an Apache mirror or a user-supplied `PYSPARK_RELEASE_MIRROR`. Extracting archive members without a containment check means a member path containing `..` would be written outside the intended destination directory. Adding an explicit resolved-path check is a straightforward robustness improvement that works on all supported Python versions. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? New unit test `test_extract_tar` in `python/pyspark/tests/test_install_spark.py` that (1) extracts a benign archive with the top-level package directory stripped, and (2) asserts a member whose path escapes the destination directory is rejected and nothing is written. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 4.8) Closes #57040 from peter-toth/SPARK-57962-install-spark-path-traversal-guard. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com> (cherry picked from commit d61f250) Signed-off-by: Peter Toth <peter.toth@gmail.com>
…ar extraction `install_spark()` in `python/pyspark/install.py` downloads a Spark release archive and extracts it by rewriting each tar member's name with `os.path.relpath(member.name, package_name + os.path.sep)` and calling `tar.extract(member, dest)`. `os.path.relpath` does not strip `..` segments (it can produce them), so a crafted archive member could resolve to a path outside `dest` (a "zip slip" / path traversal) when extracted. This change extracts the loop into a helper `_extract_tar` that, before extracting each member, resolves its destination (`os.path.realpath(os.path.join(dest, member.name))`) and rejects any member that would land outside `dest`. The behavior for legitimate archives is unchanged. tarfile's `filter='data'` (PEP 706) rejects such members natively and would replace this manual check, but it is only generally available from Python 3.12.0 (backported to 3.11.4+), so we keep the explicit check while Spark still supports Python 3.11, and leave a note to revisit once the minimum supported Python is >= 3.12. `install_spark()` runs at `pip install pyspark` time (when `PYSPARK_HADOOP_VERSION`/`PYSPARK_HIVE_VERSION` is set) and downloads from an Apache mirror or a user-supplied `PYSPARK_RELEASE_MIRROR`. Extracting archive members without a containment check means a member path containing `..` would be written outside the intended destination directory. Adding an explicit resolved-path check is a straightforward robustness improvement that works on all supported Python versions. No. New unit test `test_extract_tar` in `python/pyspark/tests/test_install_spark.py` that (1) extracts a benign archive with the top-level package directory stripped, and (2) asserts a member whose path escapes the destination directory is rejected and nothing is written. Generated-by: Claude Code (Opus 4.8) Closes #57040 from peter-toth/SPARK-57962-install-spark-path-traversal-guard. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com> (cherry picked from commit d61f250) Signed-off-by: Peter Toth <peter.toth@gmail.com>
…ar extraction `install_spark()` in `python/pyspark/install.py` downloads a Spark release archive and extracts it by rewriting each tar member's name with `os.path.relpath(member.name, package_name + os.path.sep)` and calling `tar.extract(member, dest)`. `os.path.relpath` does not strip `..` segments (it can produce them), so a crafted archive member could resolve to a path outside `dest` (a "zip slip" / path traversal) when extracted. This change extracts the loop into a helper `_extract_tar` that, before extracting each member, resolves its destination (`os.path.realpath(os.path.join(dest, member.name))`) and rejects any member that would land outside `dest`. The behavior for legitimate archives is unchanged. tarfile's `filter='data'` (PEP 706) rejects such members natively and would replace this manual check, but it is only generally available from Python 3.12.0 (backported to 3.11.4+), so we keep the explicit check while Spark still supports Python 3.11, and leave a note to revisit once the minimum supported Python is >= 3.12. `install_spark()` runs at `pip install pyspark` time (when `PYSPARK_HADOOP_VERSION`/`PYSPARK_HIVE_VERSION` is set) and downloads from an Apache mirror or a user-supplied `PYSPARK_RELEASE_MIRROR`. Extracting archive members without a containment check means a member path containing `..` would be written outside the intended destination directory. Adding an explicit resolved-path check is a straightforward robustness improvement that works on all supported Python versions. No. New unit test `test_extract_tar` in `python/pyspark/tests/test_install_spark.py` that (1) extracts a benign archive with the top-level package directory stripped, and (2) asserts a member whose path escapes the destination directory is rejected and nothing is written. Generated-by: Claude Code (Opus 4.8) Closes #57040 from peter-toth/SPARK-57962-install-spark-path-traversal-guard. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com> (cherry picked from commit d61f250) Signed-off-by: Peter Toth <peter.toth@gmail.com>
…ar extraction `install_spark()` in `python/pyspark/install.py` downloads a Spark release archive and extracts it by rewriting each tar member's name with `os.path.relpath(member.name, package_name + os.path.sep)` and calling `tar.extract(member, dest)`. `os.path.relpath` does not strip `..` segments (it can produce them), so a crafted archive member could resolve to a path outside `dest` (a "zip slip" / path traversal) when extracted. This change extracts the loop into a helper `_extract_tar` that, before extracting each member, resolves its destination (`os.path.realpath(os.path.join(dest, member.name))`) and rejects any member that would land outside `dest`. The behavior for legitimate archives is unchanged. tarfile's `filter='data'` (PEP 706) rejects such members natively and would replace this manual check, but it is only generally available from Python 3.12.0 (backported to 3.11.4+), so we keep the explicit check while Spark still supports Python 3.11, and leave a note to revisit once the minimum supported Python is >= 3.12. `install_spark()` runs at `pip install pyspark` time (when `PYSPARK_HADOOP_VERSION`/`PYSPARK_HIVE_VERSION` is set) and downloads from an Apache mirror or a user-supplied `PYSPARK_RELEASE_MIRROR`. Extracting archive members without a containment check means a member path containing `..` would be written outside the intended destination directory. Adding an explicit resolved-path check is a straightforward robustness improvement that works on all supported Python versions. No. New unit test `test_extract_tar` in `python/pyspark/tests/test_install_spark.py` that (1) extracts a benign archive with the top-level package directory stripped, and (2) asserts a member whose path escapes the destination directory is rejected and nothing is written. Generated-by: Claude Code (Opus 4.8) Closes #57040 from peter-toth/SPARK-57962-install-spark-path-traversal-guard. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com> (cherry picked from commit d61f250) Signed-off-by: Peter Toth <peter.toth@gmail.com>
|
Thank you @dongjoon-hyun and @gaogaotiantian for the review. |
What changes were proposed in this pull request?
install_spark()inpython/pyspark/install.pydownloads a Spark release archive and extracts it by rewriting each tar member's name withos.path.relpath(member.name, package_name + os.path.sep)and callingtar.extract(member, dest).os.path.relpathdoes not strip..segments (it can produce them), so a crafted archive member could resolve to a path outsidedest(a "zip slip" / path traversal) when extracted.This change extracts the loop into a helper
_extract_tarthat, before extracting each member, resolves its destination (os.path.realpath(os.path.join(dest, member.name))) and rejects any member that would land outsidedest. The behavior for legitimate archives is unchanged.tarfile's
filter='data'(PEP 706) rejects such members natively and would replace this manual check, but it is only generally available from Python 3.12.0 (backported to 3.11.4+), so we keep the explicit check while Spark still supports Python 3.11, and leave a note to revisit once the minimum supported Python is >= 3.12.Why are the changes needed?
install_spark()runs atpip install pysparktime (whenPYSPARK_HADOOP_VERSION/PYSPARK_HIVE_VERSIONis set) and downloads from an Apache mirror or a user-suppliedPYSPARK_RELEASE_MIRROR. Extracting archive members without a containment check means a member path containing..would be written outside the intended destination directory. Adding an explicit resolved-path check is a straightforward robustness improvement that works on all supported Python versions.Does this PR introduce any user-facing change?
No.
How was this patch tested?
New unit test
test_extract_tarinpython/pyspark/tests/test_install_spark.pythat (1) extracts a benign archive with the top-level package directory stripped, and (2) asserts a member whose path escapes the destination directory is rejected and nothing is written.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)