Skip to content

Commit

Permalink
Merge pull request from GHSA-2q4p-f6gf-mqr5
Browse files Browse the repository at this point in the history
* Fix filename validation in Support Bundle handling

The previous implementation of "SupportBundle#ensureFileWithinBundleDir"
was affected by a partial path traversal vulnerability and allowed
authenticated users with the Admin role to download or delete files in
sibling directories of the support bundle data directory.

See: GHSA-2q4p-f6gf-mqr5

* Add changelog snippet

* Fix bundle download with relative data_dir config

* Fix test for the assertj version we are using in 5.1

* Fix another test case
  • Loading branch information
bernd committed Jul 5, 2023
1 parent ff90f3e commit 02b8792
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 4 deletions.
2 changes: 2 additions & 0 deletions changelog/unreleased/ghsa-2q4p-f6gf-mqr5.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
type = "security"
message = "Fix partial path traversal vulnerability in Support Bundle feature. [GHSA-2q4p-f6gf-mqr5](https://github.com/Graylog2/graylog2-server/security/advisories/GHSA-2q4p-f6gf-mqr5)"
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ public List<BundleFile> listBundles() {
}

public void downloadBundle(String filename, OutputStream outputStream) throws IOException {
ensureFileWithinBundleDir(filename);
ensureFileWithinBundleDir(bundleDir, filename);

try {
final Path filePath = bundleDir.resolve(filename);
Expand All @@ -512,14 +512,15 @@ public void downloadBundle(String filename, OutputStream outputStream) throws IO
}
}

private void ensureFileWithinBundleDir(String filename) throws IOException {
if (!bundleDir.resolve(filename).toFile().getCanonicalPath().startsWith(bundleDir.toFile().getCanonicalPath())) {
@VisibleForTesting
void ensureFileWithinBundleDir(Path bundleDir, String filename) {
if (!bundleDir.resolve(filename).toAbsolutePath().normalize().startsWith(bundleDir.toAbsolutePath().normalize())) {
throw new NotFoundException();
}
}

public void deleteBundle(String filename) throws IOException {
ensureFileWithinBundleDir(filename);
ensureFileWithinBundleDir(bundleDir, filename);
final Path filePath = bundleDir.resolve(filename);
Files.delete(filePath);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,22 @@
import org.graylog2.shared.bindings.providers.ObjectMapperProvider;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;

import javax.ws.rs.NotFoundException;
import java.nio.file.Path;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.List;
import java.util.concurrent.ExecutorService;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

@ExtendWith(MockitoExtension.class)
public class SupportBundleServiceTest {
Expand Down Expand Up @@ -95,4 +100,32 @@ public void testLogSizeLimiterWithSpaceForOneZippedFile() {
assertThat(shrinkedList).hasSize(3);
assertThat(shrinkedList).extracting(LogFile::id).contains("memory", "0", "1");
}

@ParameterizedTest
@ValueSource(strings = {"/tmp/safe_dir", "safe_dir", "../safe_dir"})
void ensureWithinBundleDir(String bundleDirString) throws Exception {
final var bundleDir = Path.of(bundleDirString);

assertThatCode(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "file.zip"))
.doesNotThrowAnyException();
assertThatCode(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "hello/file.zip"))
.doesNotThrowAnyException();
assertThatCode(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "hello/world/file.zip"))
.doesNotThrowAnyException();
assertThatCode(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "..file.zip"))
.doesNotThrowAnyException();

assertThatThrownBy(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "/etc/file.zip"))
.isInstanceOf(NotFoundException.class);
assertThatThrownBy(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "/etc/hello/../world/../file.zip"))
.isInstanceOf(NotFoundException.class);
assertThatThrownBy(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "../file.zip"))
.isInstanceOf(NotFoundException.class);
assertThatThrownBy(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "../../file.zip"))
.isInstanceOf(NotFoundException.class);
assertThatThrownBy(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "../safe_dir_insecure/file.zip"))
.isInstanceOf(NotFoundException.class);
assertThatThrownBy(() -> supportBundleService.ensureFileWithinBundleDir(bundleDir, "/safe_dir_insecure/file.zip"))
.isInstanceOf(NotFoundException.class);
}
}

0 comments on commit 02b8792

Please sign in to comment.