Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,19 @@ public class FsRepository extends BlobStoreRepository {
Property.Deprecated
);

public static final Setting<String> BASE_PATH_SETTING = Setting.simpleString("base_path");
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)
Comment thread
cwperks marked this conversation as resolved.
&& (value.startsWith("/") || value.startsWith("\\") || value.matches("^[A-Za-z]:.*") || value.contains(".."))) {
Comment thread
cwperks marked this conversation as resolved.
throw new IllegalArgumentException(
"[base_path] must be a relative path that does not contain '..' segments; got [" + value + "]"
);
}
});

protected final Environment environment;

Expand Down Expand Up @@ -169,12 +181,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/<repo>/_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()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,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 {
Expand Down Expand Up @@ -265,6 +266,58 @@ public void testRestrictedSettingsDefault() {
assertTrue(restrictedSettings.contains(FsRepository.LOCATION_SETTING));
}

public void testBasePathEscapingPathRepoIsRejected() {
final Path repo = createTempDir();
final List<String> 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(() -> {
Expand Down
Loading