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 <[email protected]> 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
