[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at Utils.unpack #35946

HyukjinKwon · 2022-03-23T04:00:02Z

What changes were proposed in this pull request?

This PR proposes to use FileUtil.unTarUsingJava that is a Java implementation for un-tar .tar files. unTarUsingJava is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264.

The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter).

Why are the changes needed?

There is a risk for arbitrary shell command injection via Utils.unpack when the filename is controlled by a malicious user. This is due to an issue in Hadoop's unTar, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904

Does this PR introduce any user-facing change?

Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command.

How was this patch tested?

Manually tested in local, and existing test cases should cover.

HyukjinKwon · 2022-03-23T04:24:58Z

This is also being reported via a different channel to Hadoop.

HyukjinKwon · 2022-03-23T04:54:28Z

core/src/main/scala/org/apache/spark/util/Utils.scala

      FileUtil.unTar(source, dest)
+    } else if (lowerSrc.endsWith(".tar")) {


tar alone has the problem.

So, after this PR, it will lose the file permissions from tar. Keeping file permissions is a non-trivial work (see SPARK-37677). I will follow how Hadoop handles this, and follow it later.

FWIW I added lines 596-598 to mitigate some of this. Can we update Hadoop to fix this too? I suppose inlining is a bit more code but more robust in this regard.

Yeah. cc @steveloughran FYI

fwiw apache/hadoop#4036 is adding perms wiring for unzip...I'm being very cautious in reviewing there

HyukjinKwon · 2022-03-23T08:27:20Z

cc @dongjoon-hyun mind taking a look please when you find some time?

core/src/main/scala/org/apache/spark/util/Utils.scala

HyukjinKwon · 2022-03-23T08:45:54Z

Thanks @martin-g

core/src/main/scala/org/apache/spark/util/Utils.scala

LuciferYang

LGTM + 1

srowen · 2022-03-23T14:07:49Z

core/src/main/scala/org/apache/spark/util/Utils.scala

      FileUtil.unTar(source, dest)
+    } else if (lowerSrc.endsWith(".tar")) {


FWIW I added lines 596-598 to mitigate some of this. Can we update Hadoop to fix this too? I suppose inlining is a bit more code but more robust in this regard.

core/src/main/scala/org/apache/spark/util/Utils.scala

dongjoon-hyun · 2022-03-24T00:34:27Z

Sorry for being late, @HyukjinKwon .

dongjoon-hyun

+1, LGTM.

HyukjinKwon · 2022-03-24T03:12:15Z

Merged to master, branch-3.3, branch-3.2 and branch-3.1.

…Utils.unpack ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes #35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…Utils.unpack ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…Utils.unpack ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 271b338)

…Utils.unpack ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…Utils.unpack (#643) ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> Co-authored-by: Hyukjin Kwon <gurwls223@apache.org>

…Utils.unpack ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…Utils.unpack (#645) ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> Co-authored-by: Hyukjin Kwon <gurwls223@apache.org>

…Utils.unpack (Kyligence#643) ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> Co-authored-by: Hyukjin Kwon <gurwls223@apache.org>

…Utils.unpack (#643) (#648) ### What changes were proposed in this pull request? This PR proposes to use `FileUtil.unTarUsingJava` that is a Java implementation for un-tar `.tar` files. `unTarUsingJava` is not public but it exists in all Hadoop versions from 2.1+, see HADOOP-9264. The security issue reproduction requires a non-Windows platform, and a non-gzipped TAR archive file name (contents don't matter). ### Why are the changes needed? There is a risk for arbitrary shell command injection via `Utils.unpack` when the filename is controlled by a malicious user. This is due to an issue in Hadoop's `unTar`, that is not properly escaping the filename before passing to a shell command:https://github.com/apache/hadoop/blob/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/FileUtil.java#L904 ### Does this PR introduce _any_ user-facing change? Yes, it prevents a security issue that, previously, allowed users to execute arbitrary shall command. ### How was this patch tested? Manually tested in local, and existing test cases should cover. Closes apache#35946 from HyukjinKwon/SPARK-38631. Authored-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 057c051) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> Co-authored-by: Hyukjin Kwon <gurwls223@apache.org>

HyukjinKwon force-pushed the SPARK-38631 branch 2 times, most recently from 8cb63a9 to 73a0252 Compare March 23, 2022 04:24

HyukjinKwon marked this pull request as draft March 23, 2022 04:29

HyukjinKwon force-pushed the SPARK-38631 branch 2 times, most recently from be0820b to b994691 Compare March 23, 2022 04:54

HyukjinKwon commented Mar 23, 2022

View reviewed changes

HyukjinKwon marked this pull request as ready for review March 23, 2022 04:56

github-actions bot added the CORE label Mar 23, 2022

martin-g approved these changes Mar 23, 2022

View reviewed changes

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated Show resolved Hide resolved

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated Show resolved Hide resolved

LuciferYang reviewed Mar 23, 2022

View reviewed changes

core/src/main/scala/org/apache/spark/util/Utils.scala Outdated Show resolved Hide resolved

LuciferYang approved these changes Mar 23, 2022

View reviewed changes

srowen reviewed Mar 23, 2022

View reviewed changes

github-actions bot added DOCS SQL STRUCTURED STREAMING labels Mar 23, 2022

Avoid using shall to untar (via Hadoop's FileUtil.unTar)

7b16ba4

HyukjinKwon force-pushed the SPARK-38631 branch from 26a069f to 7b16ba4 Compare March 24, 2022 00:07

dongjoon-hyun approved these changes Mar 24, 2022

View reviewed changes

HyukjinKwon closed this in 057c051 Mar 24, 2022

7mming7 mentioned this pull request Mar 28, 2022

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at … Kyligence/spark#438

Closed

Mrhs121 mentioned this pull request Jul 3, 2023

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at … Kyligence/spark#640

Closed

Mrhs121 mentioned this pull request Jul 13, 2023

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at … Kyligence/spark#643

Merged

Mrhs121 mentioned this pull request Jul 21, 2023

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at … Kyligence/spark#645

Merged

Mrhs121 mentioned this pull request Jul 27, 2023

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at … Kyligence/spark#648

Merged

HyukjinKwon deleted the SPARK-38631 branch January 15, 2024 00:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at Utils.unpack #35946

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at Utils.unpack #35946

HyukjinKwon commented Mar 23, 2022 •

edited

Loading

HyukjinKwon commented Mar 23, 2022

HyukjinKwon Mar 23, 2022

HyukjinKwon Mar 23, 2022

srowen Mar 23, 2022

HyukjinKwon Mar 23, 2022

steveloughran Mar 25, 2022

HyukjinKwon commented Mar 23, 2022

HyukjinKwon commented Mar 23, 2022

LuciferYang left a comment

srowen Mar 23, 2022

dongjoon-hyun commented Mar 24, 2022

dongjoon-hyun left a comment

HyukjinKwon commented Mar 24, 2022

		FileUtil.unTar(source, dest)
		} else if (lowerSrc.endsWith(".tar")) {

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at Utils.unpack #35946

[SPARK-38631][CORE] Uses Java-based implementation for un-tarring at Utils.unpack #35946

Conversation

HyukjinKwon commented Mar 23, 2022 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

HyukjinKwon commented Mar 23, 2022

HyukjinKwon Mar 23, 2022

Choose a reason for hiding this comment

HyukjinKwon Mar 23, 2022

Choose a reason for hiding this comment

srowen Mar 23, 2022

Choose a reason for hiding this comment

HyukjinKwon Mar 23, 2022

Choose a reason for hiding this comment

steveloughran Mar 25, 2022

Choose a reason for hiding this comment

HyukjinKwon commented Mar 23, 2022

HyukjinKwon commented Mar 23, 2022

LuciferYang left a comment

Choose a reason for hiding this comment

srowen Mar 23, 2022

Choose a reason for hiding this comment

dongjoon-hyun commented Mar 24, 2022

dongjoon-hyun left a comment

Choose a reason for hiding this comment

HyukjinKwon commented Mar 24, 2022

HyukjinKwon commented Mar 23, 2022 •

edited

Loading