(impala) 02/05: IMPALA-15345: Resolve '..' segments before trusted_jar_paths check

[mi...@apache.org]

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

MikaelSmith pushed a commit to branch branch-4.5.3
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 8cfaf9b5a62522b409ed6a52ec0d322f6b4c2407
Author: Michael Smith <mi...@cloudera.com>
AuthorDate: Fri Sep 11 16:16:26 2026 -0700

    IMPALA-15345: Resolve '..' segments before trusted_jar_paths check
    
    BackendConfig.isJarPathAllowed() compared the raw driver.url / HDFS
    location string against the configured --trusted_jar_paths prefixes
    using String#startsWith(). That string is later turned into a Hadoop
    Path and resolved by the filesystem, which collapses '.' and '..'
    segments. A URL such as '<trusted_dir>/../../secret.jar' therefore
    passed the prefix check while actually resolving to a JAR outside the
    trusted directory, letting Impala load and execute attacker-controlled
    code in impalad.
    
    Resolve both the candidate path and each allowlist entry through
    Hadoop's Path class (which performs the same '.'/'..' collapsing)
    before comparing, and require the resolved path to be nested inside,
    or equal to, a resolved trusted directory rather than merely share a
    string prefix.
    
    Testing:
    - Added BackendConfigTest cases covering '..'-escape rejection,
      legitimate in-directory '..'/'.' usage, sibling-directory-name
      spoofing, and malformed allowlist entries.
    
    Change-Id: I4288aba344d7d0cd07c9fb8281fc8d3c0490e432
    Assisted-by: Claude Sonnet 5 (GitHub Copilot)
---
 .../org/apache/impala/service/BackendConfig.java   | 44 ++++++++++++++++++--
 .../apache/impala/service/BackendConfigTest.java   | 47 ++++++++++++++++++++++
 2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/fe/src/main/java/org/apache/impala/service/BackendConfig.java b/fe/src/main/java/org/apache/impala/service/BackendConfig.java
index e7d9bddd5..68a1a0234 100644
--- a/fe/src/main/java/org/apache/impala/service/BackendConfig.java
+++ b/fe/src/main/java/org/apache/impala/service/BackendConfig.java
@@ -23,6 +23,7 @@ import static org.apache.hadoop.fs.CommonConfigurationKeysPublic
 import java.util.Scanner;
 
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.security.authentication.util.KerberosName;
 import org.apache.impala.analysis.SqlScanner;
 import org.apache.impala.thrift.TBackendGflags;
@@ -566,19 +567,56 @@ public class BackendConfig {
   }
 
   /**
-   * Returns true if 'path' starts with one of the allowed prefixes configured in
-   * --trusted_jar_paths. An empty allowlist (the default) disables jar
+   * Returns true if 'path', once resolved the same way Hadoop's Path class resolves
+   * it (collapsing '.' and '..' segments), is inside one of the directories
+   * configured in --trusted_jar_paths. An empty allowlist (the default) disables jar
    * loading from table properties entirely.
+   *
+   * 'path' must be resolved (rather than compared as a raw string) because the caller
+   * eventually turns it into a Hadoop Path before reading the file, and Hadoop
+   * resolves '..' segments at that point. Comparing the raw, unresolved string would
+   * let a path like '<trusted_dir>/../../secret.jar' pass the allowlist check while
+   * actually resolving to a location outside the trusted directory.
    */
   public boolean isJarPathAllowed(String path) {
     String allowlist = backendCfg_.trusted_jar_paths;
     if (Strings.isNullOrEmpty(allowlist) || Strings.isNullOrEmpty(path)) return false;
+    String resolvedPath = resolveJarPath(path);
+    if (resolvedPath == null) return false;
     try (Scanner scanner = new Scanner(allowlist).useDelimiter(",")) {
       while (scanner.hasNext()) {
         String prefix = scanner.next().trim();
-        if (!prefix.isEmpty() && path.startsWith(prefix)) return true;
+        if (prefix.isEmpty()) continue;
+        String resolvedPrefix = resolveJarPath(prefix);
+        if (resolvedPrefix != null && isWithinTrustedDir(resolvedPath, resolvedPrefix)) {
+          return true;
+        }
       }
     }
     return false;
   }
+
+  /**
+   * Resolves 'rawPath' via Hadoop's Path class so that '.' and '..' segments are
+   * collapsed the same way they will be when the path is later used to read a file.
+   * Returns null if 'rawPath' cannot be parsed as a Path.
+   */
+  private static String resolveJarPath(String rawPath) {
+    try {
+      return new Path(rawPath).toString();
+    } catch (IllegalArgumentException e) {
+      return null;
+    }
+  }
+
+  /**
+   * Returns true if resolved path 'path' is exactly the resolved directory
+   * 'trustedDir', or is nested inside it. Both arguments must already be resolved via
+   * resolveJarPath().
+   */
+  private static boolean isWithinTrustedDir(String path, String trustedDir) {
+    if (path.equals(trustedDir)) return true;
+    String trustedDirWithSlash = trustedDir.endsWith("/") ? trustedDir : trustedDir + "/";
+    return path.startsWith(trustedDirWithSlash);
+  }
 }
diff --git a/fe/src/test/java/org/apache/impala/service/BackendConfigTest.java b/fe/src/test/java/org/apache/impala/service/BackendConfigTest.java
index f56355010..a4f161887 100644
--- a/fe/src/test/java/org/apache/impala/service/BackendConfigTest.java
+++ b/fe/src/test/java/org/apache/impala/service/BackendConfigTest.java
@@ -111,4 +111,51 @@ public class BackendConfigTest {
     BackendConfig cfg = configWith(",");
     assertFalse(cfg.isJarPathAllowed("/trusted/foo.jar"));
   }
+
+  // --- isJarPathAllowed: path traversal ('..') resolution ---
+
+  @Test
+  public void testIsJarPathAllowed_dotDotEscapesTrustedDir_returnsFalse() {
+    // Raw string starts with the trusted prefix, but resolving '..' segments (the
+    // same way the caller's Hadoop Path will) lands outside the trusted directory.
+    BackendConfig cfg = configWith("/trusted/");
+    assertFalse(cfg.isJarPathAllowed("/trusted/../../secret/foo.jar"));
+  }
+
+  @Test
+  public void testIsJarPathAllowed_dotDotEscapesTrustedDirWithScheme_returnsFalse() {
+    BackendConfig cfg = configWith("hdfs://nn/trusted/");
+    assertFalse(cfg.isJarPathAllowed("hdfs://nn/trusted/../../secret/foo.jar"));
+  }
+
+  @Test
+  public void testIsJarPathAllowed_dotDotStaysWithinTrustedDir_returnsTrue() {
+    // '..' segments that resolve to a location still inside the trusted directory
+    // are legitimate and should be allowed.
+    BackendConfig cfg = configWith("/trusted/");
+    assertTrue(cfg.isJarPathAllowed("/trusted/sub/../foo.jar"));
+  }
+
+  @Test
+  public void testIsJarPathAllowed_dotSegmentIsNoOp_returnsTrue() {
+    BackendConfig cfg = configWith("/trusted/");
+    assertTrue(cfg.isJarPathAllowed("/trusted/./foo.jar"));
+  }
+
+  @Test
+  public void testIsJarPathAllowed_siblingDirWithSamePrefixSpelling_returnsFalse() {
+    // A directory named similarly to the trusted one (but not nested inside it) must
+    // not be treated as trusted just because the raw string starts with the prefix.
+    BackendConfig cfg = configWith("/trusted");
+    assertFalse(cfg.isJarPathAllowed("/trusted-evil/foo.jar"));
+  }
+
+  @Test
+  public void testIsJarPathAllowed_unparsablePrefix_isSkipped() {
+    // A malformed allowlist entry must not be able to match everything; it should be
+    // skipped, and a valid subsequent entry should still be usable.
+    BackendConfig cfg = configWith("::not a valid path::,/trusted/");
+    assertTrue(cfg.isJarPathAllowed("/trusted/foo.jar"));
+    assertFalse(cfg.isJarPathAllowed("/untrusted/foo.jar"));
+  }
 }