(spark) 01/05: RemoteBlockPushResolver validate paths

[pt...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

peter-toth pushed a commit to branch branch-4.0
in repository https://gitbox.apache.org/repos/asf/spark.git

commit 8f981296bf82e954ed034a71d112515082fec17c
Author: Holden Karau <ho...@snowflake.com>
AuthorDate: Sun Aug 30 05:02:15 2026 +0000

    RemoteBlockPushResolver validate paths
    
    Co-authored-by: Holden Karau <ho...@pigscanfly.ca>
    (cherry picked from commit f3611f63c0f28db13d4db2ba9a30de44da075ece)
---
 .../network/shuffle/RemoteBlockPushResolver.java   | 39 ++++++++++++++++++++--
 .../shuffle/RemoteBlockPushResolverSuite.java      | 20 +++++++++++
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java b/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java
index 6e9bd548f532..3846d1d4abd1 100644
--- a/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java
+++ b/common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java
@@ -894,6 +894,7 @@ public class RemoteBlockPushResolver implements MergedShuffleFileManager {
             String.format("Failed to get the merge directory information from the " +
               "shuffleManagerMeta %s in executor registration message", shuffleManagerMeta));
         }
+        validateMergeDirectoryName(mergeDir, shuffleManagerMeta);
         if (attemptId == UNDEFINED_ATTEMPT_ID) {
           // When attemptId is -1, there is no attemptId stored in the ExecutorShuffleInfo.
           // Only the first ExecutorRegister message can register the merge dirs.
@@ -950,6 +951,25 @@ public class RemoteBlockPushResolver implements MergedShuffleFileManager {
     }
   }
 
+  /**
+   * Validates the peer-supplied merge directory name. Spark only ever generates a
+   * single directory name here ("merge_manager" or "merge_manager_&lt;attemptId&gt;"), so a
+   * value containing a path separator, a "." or ".." segment, can be rejected.
+   */
+  private static void validateMergeDirectoryName(String mergeDir, String shuffleManagerMeta) {
+    boolean valid = !mergeDir.isEmpty()
+      && !mergeDir.equals(".")
+      && !mergeDir.equals("..")
+      && mergeDir.indexOf('/') < 0
+      && mergeDir.indexOf('\\') < 0
+      && !mergeDir.contains(File.separator);
+    if (!valid) {
+      throw new IllegalArgumentException(
+        String.format("Invalid merge directory name %s from the shuffleManagerMeta %s in " +
+          "executor registration message", mergeDir, shuffleManagerMeta));
+    }
+  }
+
   /**
    * Shutdown mergedShuffleCleaner and close the DB during shutdown
    */
@@ -2032,13 +2052,28 @@ public class RemoteBlockPushResolver implements MergedShuffleFileManager {
         String mergeDirectory,
         int subDirsPerLocalDir) {
       activeLocalDirs = Arrays.stream(localDirs)
-        .map(localDir ->
+        .map(localDir -> {
           // Merge directory is created at the same level as block-manager directory. The list of
           // local directories that we get from ExecutorShuffleInfo are paths of each
           // block-manager directory. The mergeDirectory is the merge directory name that we get
           // from ExecutorShuffleInfo. To find out the merge directory location, we first find the
           // parent dir of the block-manager directory and then append merge directory name to it.
-          Paths.get(localDir).getParent().resolve(mergeDirectory).toFile().getPath())
+          Path parent = Paths.get(localDir).getParent();
+          if (parent == null) {
+            throw new IllegalArgumentException(
+              "Invalid local directory without a parent dir: " + localDir);
+          }
+          // The resolved merge directory must remain directly under the
+          // parent of the local dir.
+          Path normalizedParent = parent.normalize();
+          Path resolved = normalizedParent.resolve(mergeDirectory).normalize();
+          if (resolved.equals(normalizedParent) || !resolved.startsWith(normalizedParent)) {
+            throw new IllegalArgumentException(
+              String.format("Merge directory %s escapes the local dir parent %s for " +
+                "application %s", mergeDirectory, normalizedParent, appId));
+          }
+          return resolved.toFile().getPath();
+        })
         .toArray(String[]::new);
       this.subDirsPerLocalDir = subDirsPerLocalDir;
       if (logger.isInfoEnabled()) {
diff --git a/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java b/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java
index edd5e1961a50..de3329083fb6 100644
--- a/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java
+++ b/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java
@@ -486,6 +486,26 @@ public class RemoteBlockPushResolverSuite {
       "executor registration message", re.getMessage());
   }
 
+  @Test
+  public void testExecutorRegisterWithTraversalMergeDirRejected() throws IOException {
+    String testApp = "executorRegisterWithTraversalMergeDir";
+    Path[] activeLocalDirs = createLocalDirs(1);
+    IllegalArgumentException relative = assertThrows(IllegalArgumentException.class,
+      () -> registerExecutor(testApp, prepareLocalDirs(activeLocalDirs, MERGE_DIRECTORY),
+        "shuffleManager:{\"mergeDir\": \"../../other-tenant\", \"attemptId\": \"1\"}"));
+    assertTrue(relative.getMessage().contains("Invalid merge directory name"));
+
+    IllegalArgumentException absolute = assertThrows(IllegalArgumentException.class,
+      () -> registerExecutor(testApp, prepareLocalDirs(activeLocalDirs, MERGE_DIRECTORY),
+        "shuffleManager:{\"mergeDir\": \"/data/other-tenant\", \"attemptId\": \"1\"}"));
+    assertTrue(absolute.getMessage().contains("Invalid merge directory name"));
+
+    IllegalArgumentException dot = assertThrows(IllegalArgumentException.class,
+      () -> registerExecutor(testApp, prepareLocalDirs(activeLocalDirs, MERGE_DIRECTORY),
+        "shuffleManager:{\"mergeDir\": \"..\", \"attemptId\": \"1\"}"));
+    assertTrue(dot.getMessage().contains("Invalid merge directory name"));
+  }
+
   @Test
   public void testExecutorRegistrationFromTwoAppAttempts() throws IOException {
     String testApp = "testExecutorRegistrationFromTwoAppAttempts";


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org