diff --git a/server/src/main/java/org/opensearch/repositories/fs/FsRepository.java b/server/src/main/java/org/opensearch/repositories/fs/FsRepository.java index 4a9a91336ec1d..9dc335645274c 100644 --- a/server/src/main/java/org/opensearch/repositories/fs/FsRepository.java +++ b/server/src/main/java/org/opensearch/repositories/fs/FsRepository.java @@ -100,7 +100,19 @@ public class FsRepository extends BlobStoreRepository { Property.Deprecated ); - public static final Setting BASE_PATH_SETTING = Setting.simpleString("base_path"); + public static final Setting 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(".."))) { + throw new IllegalArgumentException( + "[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]" + ); + } + }); protected final Environment environment; @@ -132,12 +144,43 @@ protected void readMetadata() { } final String basePath = BASE_PATH_SETTING.get(metadata.settings()); if (Strings.hasLength(basePath)) { + if (isUnderRepo(basePath) == false) { + throw new RepositoryException( + metadata.name(), + "base_path [" + basePath + "] resolves to a location outside of the repository paths specified by path.repo" + ); + } this.basePath = new BlobPath().add(basePath); } else { this.basePath = BlobPath.cleanPath(); } } + /** + * Defense-in-depth containment check (CWE-22) that complements the {@link #BASE_PATH_SETTING} setting-level + * validator: ensures the user-supplied {@code base_path}, once resolved against the already-validated repository + * {@code location}, still falls within one of the operator-configured {@code path.repo} directories. The setting + * validator already rejects absolute and upward-escaping values; this location-aware check is the authoritative + * backstop. Without containment, an absolute {@code base_path} would cause {@link java.nio.file.Path#resolve} to + * discard the validated location entirely and redirect all blob-store operations to an arbitrary filesystem path + * (the {@code /_snapshot//_cleanup} arbitrary-deletion vector). + */ + 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; + } + protected void validateLocation() { String location = REPOSITORIES_LOCATION_SETTING.get(metadata.settings()); if (location.isEmpty()) { diff --git a/server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java b/server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java index d9f599714805b..5c40caebec92a 100644 --- a/server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java +++ b/server/src/test/java/org/opensearch/repositories/fs/FsRepositoryTests.java @@ -92,6 +92,7 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.emptySet; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; public class FsRepositoryTests extends OpenSearchTestCase { @@ -246,6 +247,58 @@ public void testRestrictedSettingsDefault() { assertTrue(restrictedSettings.contains(FsRepository.LOCATION_SETTING)); } + public void testBasePathEscapingPathRepoIsRejected() { + final Path repo = createTempDir(); + final List maliciousBasePaths = List.of( + repo.toAbsolutePath().toString(), // absolute path: Path.resolve discards the validated location + "/usr/share/opensearch/data/nodes/0", // absolute path (the reported POC payload) + "..", // parent-directory traversal + "../escape", + "nested/../../escape", + "foo/../bar", // interior '..' is rejected by the strict string validator + "a/b/../c" + ); + for (String basePath : maliciousBasePaths) { + final Settings settings = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath()) + .put(Environment.PATH_REPO_SETTING.getKey(), repo.toAbsolutePath()) + .put("location", repo) + .put(FsRepository.BASE_PATH_SETTING.getKey(), basePath) + .build(); + final RepositoryMetadata metadata = new RepositoryMetadata("test", "fs", settings); + final RuntimeException e = expectThrows( + RuntimeException.class, + () -> new FsRepository( + metadata, + new Environment(settings, null), + NamedXContentRegistry.EMPTY, + BlobStoreTestUtil.mockClusterService(), + new RecoverySettings(settings, new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) + ) + ); + assertThat("base_path [" + basePath + "] must be rejected", e.getMessage(), containsString("base_path")); + } + } + + public void testRelativeBasePathWithinPathRepoIsAccepted() { + final Path repo = createTempDir(); + final Settings settings = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toAbsolutePath()) + .put(Environment.PATH_REPO_SETTING.getKey(), repo.toAbsolutePath()) + .put("location", repo) + .put(FsRepository.BASE_PATH_SETTING.getKey(), "nested/base/path") + .build(); + final RepositoryMetadata metadata = new RepositoryMetadata("test", "fs", settings); + // A well-formed relative base_path that stays within path.repo must construct without throwing. + new FsRepository( + metadata, + new Environment(settings, null), + NamedXContentRegistry.EMPTY, + BlobStoreTestUtil.mockClusterService(), + new RecoverySettings(settings, new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)) + ); + } + private void runGeneric(ThreadPool threadPool, Runnable runnable) throws InterruptedException { CountDownLatch latch = new CountDownLatch(1); threadPool.generic().submit(() -> {