From 246efbbaa0b2cbc4a58347e6e5859b730afc47b9 Mon Sep 17 00:00:00 2001 From: Dan Torrey Date: Thu, 8 Apr 2021 15:03:56 +0200 Subject: [PATCH 1/5] Add path check --- .../graylog2/lookup/adapters/CSVFileDataAdapter.java | 6 +++++- .../adapters/LookupDataAdapterValidationContext.java | 10 +++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java index a63a78b3cbbc..f5cce4f0c29c 100644 --- a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java +++ b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java @@ -297,10 +297,14 @@ public static Builder builder() { } @Override - public Optional> validate() { + public Optional> validate(LookupDataAdapterValidationContext context) { final ArrayListMultimap errors = ArrayListMultimap.create(); final Path path = Paths.get(path()); + if (!context.getPathChecker().fileIsInAllowedPath(path)) { + errors.put("path", ALLOWED_PATH_ERROR); + } + if (!Files.exists(path)) { errors.put("path", "The file does not exist."); } else if (!Files.isReadable(path)) { diff --git a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/LookupDataAdapterValidationContext.java b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/LookupDataAdapterValidationContext.java index c7bf054c8456..57c436300bb9 100644 --- a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/LookupDataAdapterValidationContext.java +++ b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/LookupDataAdapterValidationContext.java @@ -17,6 +17,7 @@ package org.graylog2.lookup.adapters; import com.google.inject.Inject; +import org.graylog2.lookup.AllowedAuxiliaryPathChecker; import org.graylog2.system.urlwhitelist.UrlWhitelistService; /** @@ -24,13 +25,20 @@ */ public class LookupDataAdapterValidationContext { private final UrlWhitelistService urlWhitelistService; + private final AllowedAuxiliaryPathChecker pathChecker; @Inject - public LookupDataAdapterValidationContext(UrlWhitelistService urlWhitelistService) { + public LookupDataAdapterValidationContext(UrlWhitelistService urlWhitelistService, + AllowedAuxiliaryPathChecker pathChecker) { this.urlWhitelistService = urlWhitelistService; + this.pathChecker = pathChecker; } public UrlWhitelistService getUrlWhitelistService() { return urlWhitelistService; } + + public AllowedAuxiliaryPathChecker getPathChecker() { + return pathChecker; + } } From a3bcbbddeb3ebeed692e30c07faa5ec2582f58e4 Mon Sep 17 00:00:00 2001 From: Dan Torrey Date: Thu, 8 Apr 2021 16:44:08 +0200 Subject: [PATCH 2/5] Return after aux file check --- .../java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java | 1 + 1 file changed, 1 insertion(+) diff --git a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java index f5cce4f0c29c..ce89574d59b3 100644 --- a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java +++ b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java @@ -303,6 +303,7 @@ public Optional> validate(LookupDataAdapterValidationCo final Path path = Paths.get(path()); if (!context.getPathChecker().fileIsInAllowedPath(path)) { errors.put("path", ALLOWED_PATH_ERROR); + return; } if (!Files.exists(path)) { From 41af62a67cf887c0f8d381911023d634461b7b0d Mon Sep 17 00:00:00 2001 From: Dan Torrey Date: Thu, 8 Apr 2021 17:03:31 +0200 Subject: [PATCH 3/5] Return after aux file check --- .../java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java index ce89574d59b3..fa46d5eb9b57 100644 --- a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java +++ b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java @@ -303,7 +303,7 @@ public Optional> validate(LookupDataAdapterValidationCo final Path path = Paths.get(path()); if (!context.getPathChecker().fileIsInAllowedPath(path)) { errors.put("path", ALLOWED_PATH_ERROR); - return; + return Optional.of(errors); } if (!Files.exists(path)) { From a43d5dd8831638aa55133e969c53072bc4aea4a8 Mon Sep 17 00:00:00 2001 From: Dan Torrey Date: Thu, 8 Apr 2021 17:28:52 +0200 Subject: [PATCH 4/5] Add tests for config validation --- .../adapters/CSVFileDataAdapterTest.java | 66 ++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java b/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java index b305a356ed15..41ff70badc78 100644 --- a/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java +++ b/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java @@ -17,6 +17,7 @@ package org.graylog2.lookup.adapters; import com.codahale.metrics.MetricRegistry; +import com.google.common.collect.Multimap; import com.google.common.io.Resources; import org.graylog2.lookup.AllowedAuxiliaryPathChecker; import org.graylog2.plugin.lookup.LookupCachePurge; @@ -30,13 +31,16 @@ import java.net.URL; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.Optional; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; import static org.graylog2.lookup.adapters.CSVFileDataAdapter.Config; import static org.graylog2.lookup.adapters.CSVFileDataAdapter.NAME; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; @@ -52,7 +56,10 @@ public class CSVFileDataAdapterTest { AllowedAuxiliaryPathChecker pathChecker; @Mock - LookupCachePurge cachePurge; + private LookupCachePurge cachePurge; + + @Mock + private LookupDataAdapterValidationContext validationContext; public CSVFileDataAdapterTest() throws Exception { final URL resource = Resources.getResource("org/graylog2/lookup/adapters/CSVFileDataAdapterTest.csv"); @@ -123,6 +130,63 @@ public void refresh_failure_disallowedFileLocation() throws Exception { assertTrue(csvFileDataAdapter.getError().isPresent()); } + @Test + public void testConfigValidationSuccess() { + final Config config = Config.builder() + .type(NAME) + .path(csvFile.toString()) + .separator(",") + .quotechar("\"") + .keyColumn("key") + .valueColumn("value") + .checkInterval(60) + .caseInsensitiveLookup(false) + .build(); + when(validationContext.getPathChecker()).thenReturn(pathChecker); + when(pathChecker.fileIsInAllowedPath(any(Path.class))).thenReturn(true); + final Optional> result = config.validate(validationContext); + assertFalse(result.isPresent()); + } + + @Test + public void testConfigValidationFileDoesNotExist() { + final Config config = Config.builder() + .type(NAME) + .path("fake-path") + .separator(",") + .quotechar("\"") + .keyColumn("key") + .valueColumn("value") + .checkInterval(60) + .caseInsensitiveLookup(false) + .build(); + when(validationContext.getPathChecker()).thenReturn(pathChecker); + when(pathChecker.fileIsInAllowedPath(any(Path.class))).thenReturn(true); + final Optional> result = config.validate(validationContext); + assertTrue(result.isPresent()); + assertEquals("The file does not exist.", String.join("", result.get().asMap().get("path"))); + } + + @Test + public void testConfigValidationFileNotInPermittedLocation() { + final Config config = Config.builder() + .type(NAME) + .path("fake-path") + .separator(",") + .quotechar("\"") + .keyColumn("key") + .valueColumn("value") + .checkInterval(60) + .caseInsensitiveLookup(false) + .build(); + when(validationContext.getPathChecker()).thenReturn(pathChecker); + when(pathChecker.fileIsInAllowedPath(any(Path.class))).thenReturn(false); + final Optional> result = config.validate(validationContext); + assertTrue(result.isPresent()); + assertEquals("The specified CSV file is not in an allowed path.", + String.join("", result.get().asMap().get("path"))); + } + private Config baseConfig() { return Config.builder() .type(NAME) From cdf32883042484a3acd63a29a4e1ca621eca3f33 Mon Sep 17 00:00:00 2001 From: Dan Torrey Date: Fri, 9 Apr 2021 14:55:29 +0200 Subject: [PATCH 5/5] Provide more appropriate error message --- .../lookup/adapters/CSVFileDataAdapter.java | 14 +++++++++++++- .../lookup/adapters/CSVFileDataAdapterTest.java | 4 ++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java index fa46d5eb9b57..43eedfa81165 100644 --- a/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java +++ b/graylog2-server/src/main/java/org/graylog2/lookup/adapters/CSVFileDataAdapter.java @@ -62,7 +62,16 @@ public class CSVFileDataAdapter extends LookupDataAdapter { private static final Logger LOG = LoggerFactory.getLogger(CSVFileDataAdapter.class); public static final String NAME = "csvfile"; - public static final String ALLOWED_PATH_ERROR = "The specified CSV file is not in an allowed path."; + + /** + * If the AllowedAuxiliaryPathChecker is enabled (one or more paths provided to the allowed_auxiliary_paths server + * configuration property), then this error path will also be triggered for cases where the file does not exist. + * This is unavoidable, since the AllowedAuxiliaryPathChecker tries to resolve symbolic links and relative paths, + * which cannot be done if the file does not exist. Therefore this error message also indicates the possibility + * that the file does not exist. + */ + public static final String ALLOWED_PATH_ERROR = + "The specified CSV file either does not exist or is not in an allowed path."; private final Config config; private final AllowedAuxiliaryPathChecker pathChecker; @@ -303,6 +312,9 @@ public Optional> validate(LookupDataAdapterValidationCo final Path path = Paths.get(path()); if (!context.getPathChecker().fileIsInAllowedPath(path)) { errors.put("path", ALLOWED_PATH_ERROR); + + // Intentionally return here, because in the Cloud context, we should not perform the following checks + // to report to the user whether or not a file exists. return Optional.of(errors); } diff --git a/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java b/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java index 41ff70badc78..19b87fecbb33 100644 --- a/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java +++ b/graylog2-server/src/test/java/org/graylog2/lookup/adapters/CSVFileDataAdapterTest.java @@ -107,7 +107,7 @@ public void doGet_failure_filePathInvalid() throws Exception { csvFileDataAdapter = new CSVFileDataAdapter("id", "name", config, new MetricRegistry(), pathChecker); assertThatThrownBy(() -> csvFileDataAdapter.doStart()) .isExactlyInstanceOf(IllegalStateException.class) - .hasMessageStartingWith("The specified CSV file is not in an allowed path."); + .hasMessageStartingWith(CSVFileDataAdapter.ALLOWED_PATH_ERROR); } @Test @@ -183,7 +183,7 @@ public void testConfigValidationFileNotInPermittedLocation() { when(pathChecker.fileIsInAllowedPath(any(Path.class))).thenReturn(false); final Optional> result = config.validate(validationContext); assertTrue(result.isPresent()); - assertEquals("The specified CSV file is not in an allowed path.", + assertEquals(CSVFileDataAdapter.ALLOWED_PATH_ERROR, String.join("", result.get().asMap().get("path"))); }