Validate base_path in FsRepository to prevent path.repo containment bypass by kh3ra · Pull Request #22328 · opensearch-project/OpenSearch

kh3ra · 2026-06-26T09:22:45Z

Description

The fs repository base_path setting was read verbatim from REST input with no validation. An absolute base_path causes Path.resolve to discard the path.repo-validated location, redirecting all blob-store operations outside the repository (CWE-22) and enabling arbitrary filesystem deletion via the snapshot _cleanup API.

Add two layers of defense:

BASE_PATH_SETTING validator rejects absolute and upward-escaping (..) values after normalization (benign interior '..' that cancels out is allowed).
validateBasePathWithinRepo() resolves base_path against the location and verifies the result stays within a configured path.repo directory.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-06-26T09:24:08Z

PR Reviewer Guide 🔍

(Review updated until commit `d773722`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Backward Compatibility The new validator rejects any `base_path` containing `..`, including benign interior segments like `foo/../bar` or `a/b/../c` that normalize to safe relative paths. Existing repositories/snapshots configured with such values (previously accepted) will now fail to load, potentially breaking upgrades. The PR description states benign interior `..` that cancels out is allowed, but the implementation rejects all `..` occurrences via `value.contains("..")`. if (Strings.hasLength(value) && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:.") \|\| value.contains(".."))) { throw new IllegalArgumentException( "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" ); } Overly Broad Regex* The check `value.matches("^[A-Za-z]:.")` rejects any relative path whose first segment happens to start with a single letter followed by a colon (e.g. `a:b`). While unusual on POSIX, this is a legal filename and previously accepted. Consider gating drive-letter rejection to Windows or using a stricter pattern. Also, `value.contains("..")` will incorrectly match filenames like `foo..bar` (a literal `..` inside a name), which is a valid filename but is now rejected. if (Strings.hasLength(value) && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:.") \|\| value.contains(".."))) { throw new IllegalArgumentException( "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" ); } Symlink Bypass `isUnderRepo` uses `Path.normalize()` and `startsWith` without resolving symlinks (`toRealPath`). If the repository `location` or a component within it is a symlink pointing outside `path.repo`, the containment check may pass syntactically while the actual filesystem target lies outside the allowed directories. Consider using `toRealPath()` for the authoritative containment check. private boolean isUnderRepo(String basePath) { final String location = REPOSITORIES_LOCATION_SETTING.get(metadata.settings()); final Path locationFile = environment.resolveRepoFile(location); if (locationFile == null) { // location is already validated by validateLocation(); a null here is treated as a containment failure. return false; } final Path resolved = locationFile.resolve(basePath).normalize(); for (Path repoPath : environment.repoFiles()) { if (resolved.startsWith(repoPath)) { return true; } } return false; }

github-actions · 2026-06-26T09:24:32Z

PR Code Suggestions ✨

Latest suggestions up to d773722

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Match '..' as segment, not substring The `value.contains("..")` check rejects legitimate base paths that happen to contain two consecutive dots as part of a name (e.g., `my..dir` or `snapshot..backup`). Match `..` only as a full path segment by splitting on `/` and `</code> and checking segment equality, to avoid false positives.` server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [111-116] if (Strings.hasLength(value) - && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:.") \|\| value.contains(".."))) { + && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:."))) { throw new IllegalArgumentException( - "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" + "[base_path] must be a relative path; got [" + value + "]" ); } +if (Strings.hasLength(value)) { + for (String segment : value.split("[/\\\\]")) { + if ("..".equals(segment)) { + throw new IllegalArgumentException( + "[base_path] must not contain '..' segments; got [" + value + "]" + ); + } + } +} Suggestion importance[1-10]: 6 __ Why: Valid observation: value.contains("..") rejects legitimate names like my..dir. Checking .. as a full segment is more precise and reduces false positives, though the practical impact is limited.	Low
General	Normalize both paths before containment check `Path.startsWith` performs textual segment comparison and can be bypassed via symlinks or non-normalized `repoPath` entries. Normalize/`toAbsolutePath` both sides (and ideally `toRealPath` when possible) to ensure containment comparisons are on canonicalized paths. server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [212-218] -final Path resolved = locationFile.resolve(basePath).normalize(); +final Path resolved = locationFile.resolve(basePath).toAbsolutePath().normalize(); for (Path repoPath : environment.repoFiles()) { - if (resolved.startsWith(repoPath)) { + if (resolved.startsWith(repoPath.toAbsolutePath().normalize())) { return true; } } return false; Suggestion importance[1-10]: 6 __ Why: Normalizing/absolutizing both sides of the containment check strengthens the CWE-22 defense against non-normalized `repoPath` entries, though `environment.repoFiles()` typically returns absolute paths already. </result> </details></details></td><td align=center>Low </td></tr></tr></tbody></table> ___ #### Previous suggestions <details><summary>Suggestions up to commit 66855ce</summary> <br><table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>General</td> <td> <details><summary>Avoid false positives on '..' substring match</summary> ___ The check <code>value.contains("..")</code> will incorrectly reject legitimate base paths that <br>contain <code>..</code> as part of a filename (e.g., <code>my..folder</code> or <code>a..b</code>). Tokenize on path <br>separators and reject only segments that equal exactly <code>".."</code>, so that filenames <br>merely containing two consecutive dots are allowed. [server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [105-117]](https://github.com/opensearch-project/OpenSearch/pull/22328/files#diff-f0e471b04d4ecac21a1eb7fc92ddb44e5cded64a03978466c57792c95ee8a803R105-R117) ```diff public static final Setting<String> BASE_PATH_SETTING = Setting.simpleString("base_path", value -> { - // CWE-22 hardening (string-only fail-fast; avoids constructing a Path from a raw user-supplied string): - // reject absolute base_path values (POSIX "/", Windows "\\"/UNC, drive-letter "C:..") and any parent-directory - // ("..") segment. An absolute base_path makes Path.resolve discard the path.repo-validated location, escaping - // containment; a ".." segment lets the resolved path climb above it. The authoritative, location-aware - // containment check is performed in validateBasePathWithinRepo(). if (Strings.hasLength(value) - && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:.") \|\| value.contains(".."))) { + && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:."))) { throw new IllegalArgumentException( - "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" + "[base_path] must be a relative path; got [" + value + "]" ); + } + if (Strings.hasLength(value)) { + for (String segment : value.split("[/\\\\]")) { + if ("..".equals(segment)) { + throw new IllegalArgumentException( + "[base_path] must not contain '..' segments; got [" + value + "]" + ); + } + } } }); Suggestion importance[1-10]: 7 __ Why: Valid point: `value.contains("..")` rejects legitimate filenames like `my..folder`. Tokenizing path segments and only rejecting exact `..` segments improves correctness without weakening security.	Medium
Security	Normalize both sides of containment check `Path.startsWith` performs lexical comparison and can be bypassed via symlinks or unnormalized `repoFiles()` entries. Normalize/`toAbsolutePath` both sides before comparison to ensure consistent containment evaluation regardless of how `path.repo` was configured. server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [210-215] -final Path resolved = locationFile.resolve(basePath).normalize(); +final Path resolved = locationFile.resolve(basePath).normalize().toAbsolutePath(); for (Path repoPath : environment.repoFiles()) { - if (resolved.startsWith(repoPath)) { + if (resolved.startsWith(repoPath.normalize().toAbsolutePath())) { return; } } Suggestion importance[1-10]: 6 __ Why: Normalizing and converting to absolute paths on both sides of the `startsWith` containment check is a reasonable defense-in-depth improvement to avoid bypass via unnormalized `repoFiles()` entries, though symlink bypass would require `toRealPath` to truly mitigate.	Low

Suggestions up to commit f541f92

Category Suggestion Impact

General

Avoid over-rejecting names containing ".."

The check value.contains("..") over-rejects legitimate names that merely contain two
consecutive dots (e.g. my..folder, version..1). Restrict the rejection to actual
parent-directory path segments by splitting on both / and </code> and comparing each segment to .., so non-traversal names with .. substrings remain valid.


server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [105-117]
 public static final Setting<String> BASE_PATH_SETTING = Setting.simpleString("base_path", value -> {
-    // CWE-22 hardening (string-only fail-fast; avoids constructing a Path from a raw user-supplied string):
-    // reject absolute base_path values (POSIX "/", Windows "\\"/UNC, drive-letter "C:..") and any parent-directory
-    // ("..") segment. An absolute base_path makes Path.resolve discard the path.repo-validated location, escaping
-    // containment; a ".." segment lets the resolved path climb above it. The authoritative, location-aware
-    // containment check is performed in validateBasePathWithinRepo().
     if (Strings.hasLength(value)
-        && (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*") || value.contains(".."))) {
+        && (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*"))) {
         throw new IllegalArgumentException(
-            "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]"
+            "[base_path] must be a relative path; got [" + value + "]"
         );
+    }
+    if (Strings.hasLength(value)) {
+        for (String segment : value.split("[/\\\\]")) {
+            if ("..".equals(segment)) {
+                throw new IllegalArgumentException(
+                    "[base_path] must not contain '..' segments; got [" + value + "]"
+                );
+            }
+        }
     }
 });
Suggestion importance[1-10]: 6
__
Why: Valid point that value.contains("..") would reject legitimate names like my..folder. Restricting the check to actual path segments is more correct, though such names are uncommon in practice.

Low

Normalize repo paths before containment check

Comparing paths via startsWith without normalizing the configured repoFiles()
entries can yield false negatives when those entries contain symlinks, relative
segments, or trailing separators. Normalize (and ideally toAbsolutePath) each
repoPath before comparing to ensure the containment check is reliable across
platforms.

server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [210-215]

-final Path resolved = locationFile.resolve(basePath).normalize();
+final Path resolved = locationFile.toAbsolutePath().normalize();
 for (Path repoPath : environment.repoFiles()) {
-    if (resolved.startsWith(repoPath)) {
+    if (resolved.startsWith(repoPath.toAbsolutePath().normalize())) {
         return;
     }
 }

Suggestion importance[1-10]: 5

__

Why: Normalizing repoPath entries before comparison improves robustness of the containment check, though environment.repoFiles() typically already returns absolute normalized paths. The suggestion also changes resolved to drop basePath, which would be a bug.

Low

Suggestions up to commit df4bed9

Category	Suggestion	Impact
General	Avoid false positives on '..' substring check The `contains("..")` check is overly broad and will reject legitimate base paths that contain `..` as part of a filename (e.g., `my..folder`, `foo..bar`). Check for `..` as a path segment instead by splitting on path separators or checking for `/../`, `..</code>, leading ..//..</code>, or exact ...` server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [111-116] if (Strings.hasLength(value) - && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:.") \|\| value.contains(".."))) { + && (value.startsWith("/") \|\| value.startsWith("\\") \|\| value.matches("^[A-Za-z]:.") + \|\| value.equals("..") \|\| value.startsWith("../") \|\| value.startsWith("..\\") + \|\| value.endsWith("/..") \|\| value.endsWith("\\..") + \|\| value.contains("/../") \|\| value.contains("\\..\\") + \|\| value.contains("/..\\") \|\| value.contains("\\../"))) { throw new IllegalArgumentException( "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" ); } Suggestion importance[1-10]: 6 __ Why: The contains("..") check would reject legitimate filenames like my..folder. The suggestion correctly identifies a false-positive issue and proposes checking .. as a path segment, improving correctness without weakening security.	Low
Security	Normalize both sides before containment check For robust containment checks against symlink/relative-path bypass, normalize and use absolute paths for both the resolved location and the configured repo paths before comparison. Using `toAbsolutePath().normalize()` on both sides ensures `startsWith` compares canonicalized paths consistently. server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [210-215] -final Path resolved = locationFile.resolve(basePath).normalize(); +final Path resolved = locationFile.toAbsolutePath().resolve(basePath).normalize(); for (Path repoPath : environment.repoFiles()) { - if (resolved.startsWith(repoPath)) { + if (resolved.startsWith(repoPath.toAbsolutePath().normalize())) { return; } } Suggestion importance[1-10]: 5 __ Why: Normalizing both sides to absolute paths makes the `startsWith` containment check more robust against relative path inconsistencies, though `environment.repoFiles()` typically already returns absolute paths.	Low

Suggestions up to commit 83938d7

Category	Suggestion	Impact
Possible issue	Normalize repo paths before containment check Containment comparison via `startsWith` on non-normalized `repoPath` entries can be inconsistent if `environment.repoFiles()` returns paths with symlinks or trailing separators that differ from the normalized `resolved`. Normalize (and ideally `toAbsolutePath`) each `repoPath` before comparison to ensure correct containment checks across platforms. server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [215-220] -final Path resolved = locationFile.resolve(basePath).normalize(); +final Path resolved = locationFile.toAbsolutePath().resolve(basePath).normalize(); for (Path repoPath : environment.repoFiles()) { - if (resolved.startsWith(repoPath)) { + if (resolved.startsWith(repoPath.toAbsolutePath().normalize())) { return; } } Suggestion importance[1-10]: 6 __ Why: Normalizing `repoPath` entries before the `startsWith` containment check is a reasonable defense-in-depth improvement for path comparison robustness, though `environment.repoFiles()` likely already returns normalized absolute paths.	Low
General	Use Path-based startsWith for traversal check `normalized.startsWith("..")` only matches when the first segment is exactly `..`, but `Paths.get("..").normalize()` produces a path whose string is `..` — the check works for that case, yet a normalized path that begins with `..` may not always be matched by `startsWith(String)` on all filesystems due to separator interpretation. Use `startsWith(Paths.get(".."))` for a more reliable path-segment comparison. server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [114-116] if (Strings.hasLength(value)) { final Path normalized = Paths.get(value).normalize(); - if (normalized.isAbsolute() \|\| normalized.startsWith("..")) { + if (normalized.isAbsolute() \|\| normalized.startsWith(Paths.get(".."))) { Suggestion importance[1-10]: 5 __ Why: Using `startsWith(Paths.get(".."))` is more correct for path-segment comparison than `startsWith(String)`, which can be affected by separator interpretation; this is a valid robustness improvement.	Low
General	Handle invalid path syntax in validator `Paths.get(value)` will throw `InvalidPathException` for syntactically invalid paths (e.g. on Windows with reserved characters), which would surface as an unhelpful error during setting parsing. Wrap the parse and rethrow as `IllegalArgumentException` so the setting validator produces a consistent, descriptive error message for any malformed `base_path` value. server/src/main/java/org/opensearch/repositories/fs/FsRepository.java [114-121] if (Strings.hasLength(value)) { - final Path normalized = Paths.get(value).normalize(); + final Path normalized; + try { + normalized = Paths.get(value).normalize(); + } catch (java.nio.file.InvalidPathException e) { + throw new IllegalArgumentException("[base_path] is not a valid path: [" + value + "]", e); + } if (normalized.isAbsolute() \|\| normalized.startsWith("..")) { throw new IllegalArgumentException( "[base_path] must be a relative path that does not escape its root via '..'; got [" + value + "]" ); } } Suggestion importance[1-10]: 4 __ Why: Wrapping `InvalidPathException` into `IllegalArgumentException` provides a more consistent error message, but this is a minor improvement since the original exception would still convey the issue.	Low

github-actions · 2026-06-26T09:29:33Z

❌ Gradle check result for 83938d7: FAILURE