You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by al...@apache.org on 2022/11/22 03:41:20 UTC
[fineract] 01/01: FINERACT-1794: Address improvement in file upload APIs
This is an automated email from the ASF dual-hosted git repository.
aleks pushed a commit to branch 1.8.1
in repository https://gitbox.apache.org/repos/asf/fineract.git
commit bacf0d92104e3ba4ab169b686692988bc5bb2535
Author: Aleks <al...@apache.org>
AuthorDate: Tue Nov 22 04:24:39 2022 +0100
FINERACT-1794: Address improvement in file upload APIs
---
.../contentrepository/ContentPathSanitizer.java | 28 ++++
.../ContentRepositoryFactory.java | 6 +-
.../FileSystemContentPathSanitizer.java | 141 +++++++++++++++++++++
.../FileSystemContentRepository.java | 30 +++--
.../src/main/resources/application.properties | 5 +
.../integrationtests/client/ImageTest.java | 117 +++++++++++++++++
.../test/resources/image-gif-correct-extension.gif | Bin 0 -> 1018890 bytes
.../test/resources/image-gif-wrong-extension.png | Bin 0 -> 1018890 bytes
.../test/resources/image-text-wrong-content.jsp | 21 +++
9 files changed, 338 insertions(+), 10 deletions(-)
diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentPathSanitizer.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentPathSanitizer.java
new file mode 100644
index 000000000..415532fcd
--- /dev/null
+++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentPathSanitizer.java
@@ -0,0 +1,28 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.documentmanagement.contentrepository;
+
+import java.io.BufferedInputStream;
+
+public interface ContentPathSanitizer {
+
+ String sanitize(String path);
+
+ String sanitize(String path, BufferedInputStream is);
+}
diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java
index bdfcb5c69..1f5faab30 100644
--- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java
+++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/ContentRepositoryFactory.java
@@ -33,18 +33,20 @@ public class ContentRepositoryFactory {
private final ApplicationContext applicationContext;
private final ExternalServicesPropertiesReadPlatformService externalServicesReadPlatformService;
+ private final FileSystemContentPathSanitizer contentPathSanitizer;
+
public ContentRepository getRepository() {
final ConfigurationDomainService configurationDomainServiceJpa = this.applicationContext.getBean("configurationDomainServiceJpa",
ConfigurationDomainService.class);
if (configurationDomainServiceJpa.isAmazonS3Enabled()) {
return createS3DocumentStore();
}
- return new FileSystemContentRepository();
+ return new FileSystemContentRepository(contentPathSanitizer);
}
public ContentRepository getRepository(final StorageType documentStoreType) {
if (documentStoreType == StorageType.FILE_SYSTEM) {
- return new FileSystemContentRepository();
+ return new FileSystemContentRepository(contentPathSanitizer);
}
return createS3DocumentStore();
}
diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/FileSystemContentPathSanitizer.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/FileSystemContentPathSanitizer.java
new file mode 100644
index 000000000..1bb29959f
--- /dev/null
+++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/FileSystemContentPathSanitizer.java
@@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.infrastructure.documentmanagement.contentrepository;
+
+import java.io.BufferedInputStream;
+import java.nio.file.Path;
+import java.util.List;
+import java.util.regex.Pattern;
+import javax.annotation.PostConstruct;
+import lombok.RequiredArgsConstructor;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.fineract.infrastructure.core.service.ThreadLocalContextUtil;
+import org.apache.fineract.infrastructure.documentmanagement.exception.ContentManagementException;
+import org.apache.tika.Tika;
+import org.apache.tika.metadata.Metadata;
+import org.apache.tika.parser.AutoDetectParser;
+import org.apache.tika.sax.BodyContentHandler;
+import org.springframework.beans.factory.annotation.Value;
+import org.springframework.stereotype.Component;
+
+@Slf4j
+@RequiredArgsConstructor
+@Component
+public class FileSystemContentPathSanitizer implements ContentPathSanitizer {
+
+ private static Pattern OVERWRITE_SIBLING_IMAGE = Pattern.compile(".*\\.\\./+[0-9]+/+.*");
+
+ @Value("${fineract.content.regex-whitelist-enabled}")
+ private boolean isRegexWhitelistEnabled;
+ @Value("${fineract.content.regex-whitelist}")
+ private List<String> regexWhitelist;
+ @Value("${fineract.content.mime-whitelist-enabled}")
+ private boolean isMimeWhitelistEnabled;
+ @Value("${fineract.content.mime-whitelist}")
+ private List<String> mimeWhitelist;
+ private List<Pattern> regexWhitelistPatterns;
+
+ @PostConstruct
+ public void init() {
+ regexWhitelistPatterns = regexWhitelist.stream().map(Pattern::compile).toList();
+ }
+
+ @Override
+ public String sanitize(String path) {
+ return sanitize(path, null);
+ }
+
+ @Override
+ public String sanitize(String path, BufferedInputStream is) {
+ try {
+ if (OVERWRITE_SIBLING_IMAGE.matcher(path).matches()) {
+ throw new RuntimeException(String.format("Trying to overwrite another resource's image: %s", path));
+ }
+
+ String sanitizedPath = Path.of(path).normalize().toString();
+
+ String fileName = FilenameUtils.getName(sanitizedPath).toLowerCase();
+
+ if (log.isDebugEnabled()) {
+ log.debug("Path: {} -> {} ({})", path, sanitizedPath, fileName);
+ }
+
+ if (isRegexWhitelistEnabled) {
+ boolean matches = regexWhitelistPatterns.stream().anyMatch(p -> p.matcher(fileName).matches());
+
+ if (!matches) {
+ throw new RuntimeException(String.format("File name not allowed: %s", fileName));
+ }
+ }
+
+ if (is != null && isMimeWhitelistEnabled) {
+ Tika tika = new Tika();
+ String extensionMimeType = tika.detect(fileName);
+
+ if (StringUtils.isEmpty(extensionMimeType)) {
+ throw new RuntimeException(String.format("Could not detect mime type for filename %s!", fileName));
+ }
+
+ if (!mimeWhitelist.contains(extensionMimeType)) {
+ throw new RuntimeException(
+ String.format("Detected mime type %s for filename %s not allowed!", extensionMimeType, fileName));
+ }
+
+ String contentMimeType = detectContentMimeType(is);
+
+ if (StringUtils.isEmpty(contentMimeType)) {
+ throw new RuntimeException(String.format("Could not detect content mime type for %s!", fileName));
+ }
+
+ if (!mimeWhitelist.contains(contentMimeType)) {
+ throw new RuntimeException(
+ String.format("Detected content mime type %s for %s not allowed!", contentMimeType, fileName));
+ }
+
+ if (!contentMimeType.equalsIgnoreCase(extensionMimeType)) {
+ throw new RuntimeException(String.format("Detected filename (%s) and content (%s) mime type do not match!",
+ extensionMimeType, contentMimeType));
+ }
+ }
+
+ Path target = Path.of(sanitizedPath);
+ Path rootFolder = Path.of(FileSystemContentRepository.FINERACT_BASE_DIR,
+ ThreadLocalContextUtil.getTenant().getName().replaceAll(" ", "").trim());
+
+ if (!target.startsWith(rootFolder)) {
+ throw new RuntimeException(String.format("Path traversal attempt: %s (%s)", target, rootFolder));
+ }
+
+ return sanitizedPath;
+ } catch (Exception e) {
+ throw new ContentManagementException(path, e.getMessage(), e);
+ }
+ }
+
+ private String detectContentMimeType(BufferedInputStream is) throws Exception {
+ AutoDetectParser parser = new AutoDetectParser();
+ BodyContentHandler handler = new BodyContentHandler();
+ Metadata metadata = new Metadata();
+ parser.parse(is, handler, metadata);
+
+ return metadata.get("Content-Type");
+ }
+}
diff --git a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/FileSystemContentRepository.java b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/FileSystemContentRepository.java
index 19a83e1fe..72614892e 100644
--- a/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/FileSystemContentRepository.java
+++ b/fineract-provider/src/main/java/org/apache/fineract/infrastructure/documentmanagement/contentrepository/FileSystemContentRepository.java
@@ -19,6 +19,7 @@
package org.apache.fineract.infrastructure.documentmanagement.contentrepository;
import com.google.common.io.Files;
+import java.io.BufferedInputStream;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.IOException;
@@ -42,6 +43,12 @@ public class FileSystemContentRepository implements ContentRepository {
public static final String FINERACT_BASE_DIR = System.getProperty("user.home") + File.separator + ".fineract";
+ private final FileSystemContentPathSanitizer pathSanitizer;
+
+ public FileSystemContentRepository(final FileSystemContentPathSanitizer pathSanitizer) {
+ this.pathSanitizer = pathSanitizer;
+ }
+
@Override
public String saveFile(final InputStream uploadedInputStream, final DocumentCommand documentCommand) {
final String fileName = documentCommand.getFileName();
@@ -88,23 +95,29 @@ public class FileSystemContentRepository implements ContentRepository {
}
private void deleteFileInternal(final String documentPath) {
- final File fileToBeDeleted = new File(documentPath);
+ String path = pathSanitizer.sanitize(documentPath);
+
+ final File fileToBeDeleted = new File(path);
final boolean fileDeleted = fileToBeDeleted.delete();
if (!fileDeleted) {
// no need to throw an Error, what's a caller going to do about it, so simply log a warning
- LOG.warn("Unable to delete file {}", documentPath);
+ LOG.warn("Unable to delete file {}", path);
}
}
@Override
public FileData fetchFile(final DocumentData documentData) {
- final File file = new File(documentData.fileLocation());
- return new FileData(Files.asByteSource(file), documentData.fileName(), documentData.contentType());
+ String path = pathSanitizer.sanitize(documentData.fileLocation());
+
+ final File file = new File(path);
+ return new FileData(Files.asByteSource(file), path, documentData.contentType());
}
@Override
public FileData fetchImage(final ImageData imageData) {
- final File file = new File(imageData.location());
+ String path = pathSanitizer.sanitize(imageData.location());
+
+ final File file = new File(path);
return new FileData(Files.asByteSource(file), imageData.getEntityDisplayName(), imageData.contentType().getValue());
}
@@ -139,9 +152,10 @@ public class FileSystemContentRepository implements ContentRepository {
}
private void writeFileToFileSystem(final String fileName, final InputStream uploadedInputStream, final String fileLocation) {
- try {
- makeDirectories(fileLocation);
- FileUtils.copyInputStreamToFile(uploadedInputStream, new File(fileLocation)); // NOSONAR
+ try (BufferedInputStream bis = new BufferedInputStream(uploadedInputStream)) {
+ String sanitizedPath = pathSanitizer.sanitize(fileLocation, bis);
+ makeDirectories(sanitizedPath);
+ FileUtils.copyInputStreamToFile(bis, new File(sanitizedPath)); // NOSONAR
} catch (final IOException ioException) {
LOG.warn("writeFileToFileSystem() IOException (logged because cause is not propagated in ContentManagementException)",
ioException);
diff --git a/fineract-provider/src/main/resources/application.properties b/fineract-provider/src/main/resources/application.properties
index a611949f5..15ee11e60 100644
--- a/fineract-provider/src/main/resources/application.properties
+++ b/fineract-provider/src/main/resources/application.properties
@@ -43,6 +43,11 @@ fineract.mode.batch-manager-enabled=${FINERACT_MODE_BATCH_MANAGER_ENABLED:true}
fineract.correlation.enabled=${FINERACT_LOGGING_HTTP_CORRELATION_ID_ENABLED:false}
fineract.correlation.header-name=${FINERACT_LOGGING_HTTP_CORRELATION_ID_HEADER_NAME:X-Correlation-ID}
+fineract.content.regex-whitelist-enabled=${FINERACT_CONTENT_REGEX_WHITELIST_ENABLED:true}
+fineract.content.regex-whitelist=${FINERACT_CONTENT_REGEX_WHITELIST:.*\\.pdf$,.*\\.doc,.*\\.docx,.*\\.xls,.*\\.xlsx,.*\\.jpg,.*\\.jpeg,.*\\.png}
+fineract.content.mime-whitelist-enabled=${FINERACT_CONTENT_MIME_WHITELIST_ENABLED:true}
+fineract.content.mime-whitelist=${FINERACT_CONTENT_MIME_WHITELIST:application/pdf,application/msword,application/vnd.openxmlformats-officedocument.wordprocessingml.document,application/vnd.ms-excel,application/vnd.openxmlformats-officedocument.spreadsheetml.sheet,image/jpeg,image/png}
+
# Logging pattern for the console
logging.pattern.console=${CONSOLE_LOG_PATTERN:%clr(%d{yyyy-MM-dd HH:mm:ss.SSS}){faint} %clr(${LOG_LEVEL_PATTERN:-%5p}) %clr(${PID:- }){magenta} %clr(%replace([%X{correlationId}]){'\\[\\]', ''}) %clr(---){faint} %clr([%15.15t]){faint} %clr(%-40.40logger{39}){cyan} %clr(:){faint} %m%n${LOG_EXCEPTION_CONVERSION_WORD:%wEx}}
diff --git a/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ImageTest.java b/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ImageTest.java
index 8bf466bbb..77496501d 100644
--- a/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ImageTest.java
+++ b/integration-tests/src/test/java/org/apache/fineract/integrationtests/client/ImageTest.java
@@ -18,10 +18,16 @@
*/
package org.apache.fineract.integrationtests.client;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
import java.io.File;
import java.io.IOException;
+import lombok.extern.slf4j.Slf4j;
import okhttp3.MediaType;
+import okhttp3.MultipartBody;
+import okhttp3.RequestBody;
import okhttp3.ResponseBody;
+import org.apache.commons.io.IOUtils;
import org.apache.fineract.client.services.ImagesApi;
import org.apache.fineract.client.util.Parts;
import org.junit.jupiter.api.Order;
@@ -36,6 +42,7 @@ import retrofit2.http.Headers;
*
* @author Michael Vorburger.ch
*/
+@Slf4j
public class ImageTest extends IntegrationTest {
final File testImage = new File(getClass().getResource("/michael.vorburger-crepes.jpg").getFile());
@@ -137,6 +144,116 @@ public class ImageTest extends IntegrationTest {
ok(fineract().images.delete("clients", clientId));
}
+ @Test
+ @Order(100)
+ void pathTraversalJsp() {
+ final MultipartBody.Part part = createPart("image-text-wrong-content.jsp",
+ "../../../../../../../../../../tmp/image-text-wrong-content.jsp", "image/gif");
+
+ assertThat(part).isNotNull();
+
+ Exception exception = assertThrows(Exception.class, () -> {
+ ok(fineract().images.create("clients", clientId, part));
+ });
+
+ assertThat(exception).isNotNull();
+
+ log.warn("Should not be able to upload a file that doesn't match the indicated content type: {}", exception.getMessage());
+ }
+
+ @Test
+ @Order(101)
+ void gifWithPngExtension() {
+ final MultipartBody.Part part = createPart("image-gif-wrong-extension.png", "image-gif-wrong-extension.png", "image/png");
+
+ assertThat(part).isNotNull();
+
+ Exception exception = assertThrows(Exception.class, () -> {
+ ok(fineract().images.create("clients", clientId, part));
+ });
+
+ assertThat(exception).isNotNull();
+
+ log.warn("Should not be able to upload a gif by just renaming the file extension: {}", exception.getMessage());
+ }
+
+ @Test
+ @Order(102)
+ void gifImage() {
+ final MultipartBody.Part part = createPart("image-gif-correct-extension.gif", "image-gif-correct-extension.gif", "image/png");
+
+ assertThat(part).isNotNull();
+
+ Exception exception = assertThrows(Exception.class, () -> {
+ ok(fineract().images.create("clients", clientId, part));
+ });
+
+ assertThat(exception).isNotNull();
+
+ log.warn("Should not be able to upload a gif it is not whitelisted: {}", exception.getMessage());
+ }
+
+ @Test
+ @Order(103)
+ void pathTraversalJpg() {
+ final MultipartBody.Part part = createPart("michael.vorburger-crepes.jpg",
+ "../../../../../../../../../../tmp/michael.vorburger-crepes.jpg", "image/jpeg");
+
+ assertThat(part).isNotNull();
+
+ Exception exception = assertThrows(Exception.class, () -> {
+ ok(fineract().images.create("clients", clientId, part));
+ });
+
+ assertThat(exception).isNotNull();
+
+ log.warn("Should not be able to upload a file with a forbidden name pattern: {}", exception.getMessage());
+ }
+
+ @Test
+ @Order(104)
+ void pathTraversalWithAbsolutePathJpg() {
+ final MultipartBody.Part part = createPart("michael.vorburger-crepes.jpg", "../17/michael.vorburger-crepes.jpg", "image/jpeg");
+
+ assertThat(part).isNotNull();
+
+ Exception exception = assertThrows(Exception.class, () -> {
+ ok(fineract().images.create("clients", clientId, part));
+ });
+
+ assertThat(exception).isNotNull();
+
+ log.warn("Should not be able to upload a file with a forbidden name pattern: {}", exception.getMessage());
+ }
+
+ @Test
+ @Order(105)
+ void pathTraversalWithAbsolutePathJpg2() {
+ final MultipartBody.Part part = createPart("michael.vorburger-crepes.jpg", "..//17//michael.vorburger-crepes.jpg", "image/jpeg");
+
+ assertThat(part).isNotNull();
+
+ Exception exception = assertThrows(Exception.class, () -> {
+ ok(fineract().images.create("clients", clientId, part));
+ });
+
+ assertThat(exception).isNotNull();
+
+ log.warn("Should not be able to upload a file with a forbidden name pattern: {}", exception.getMessage());
+ }
+
+ private MultipartBody.Part createPart(String fileResource, String fileName, String mediaType) {
+ try {
+ byte[] data = IOUtils.toByteArray(ImageTest.class.getClassLoader().getResourceAsStream(fileResource));
+ RequestBody rb = RequestBody.create(data, MediaType.get(mediaType));
+ return MultipartBody.Part.createFormData("file", fileName, rb);
+ } catch (Exception e) {
+ log.error(e.toString(), e);
+ }
+
+ return null;
+ }
+
interface ImagesApiWithHeadersForTest extends ImagesApi {
@Headers("Accept: text/plain")
diff --git a/integration-tests/src/test/resources/image-gif-correct-extension.gif b/integration-tests/src/test/resources/image-gif-correct-extension.gif
new file mode 100644
index 000000000..89b951662
Binary files /dev/null and b/integration-tests/src/test/resources/image-gif-correct-extension.gif differ
diff --git a/integration-tests/src/test/resources/image-gif-wrong-extension.png b/integration-tests/src/test/resources/image-gif-wrong-extension.png
new file mode 100644
index 000000000..89b951662
Binary files /dev/null and b/integration-tests/src/test/resources/image-gif-wrong-extension.png differ
diff --git a/integration-tests/src/test/resources/image-text-wrong-content.jsp b/integration-tests/src/test/resources/image-text-wrong-content.jsp
new file mode 100644
index 000000000..6e660673c
--- /dev/null
+++ b/integration-tests/src/test/resources/image-text-wrong-content.jsp
@@ -0,0 +1,21 @@
+<%--
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+--%>
+<JSP>add JSP CODE here </jsp>
\ No newline at end of file