Security vulnerabilities in rest files endpoint #3804

Author: dtaimanov

jmix-localfs/localfs/src/main/java/io/jmix/localfs/LocalFileStorage.java

@@ -18,12 +18,7 @@
 
 import com.google.common.collect.Maps;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
-import io.jmix.core.CoreProperties;
-import io.jmix.core.FileRef;
-import io.jmix.core.FileStorage;
-import io.jmix.core.FileStorageException;
-import io.jmix.core.TimeSource;
-import io.jmix.core.UuidProvider;
+import io.jmix.core.*;
 import io.jmix.core.annotation.Internal;
 import jakarta.annotation.PreDestroy;
 import org.apache.commons.io.FileUtils;
@@ -33,6 +28,7 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.beans.factory.annotation.Value;
 import org.springframework.stereotype.Component;
 
 import java.io.File;
@@ -71,6 +67,9 @@ public class LocalFileStorage implements FileStorage {
     @Autowired
     protected TimeSource timeSource;
 
+    @Value("${jmix.localfs.disable-path-check:false}")
+    protected Boolean disablePathCheck;
+
     protected boolean isImmutableFileStorage;
 
     protected ExecutorService writeExecutor = Executors.newFixedThreadPool(5,
@@ -162,8 +161,21 @@ public long saveStream(FileRef fileRef, InputStream inputStream) {
         checkFileExists(path);
 
         long size;
+        long maxAllowedSize = properties.getMaxFileSize().toBytes();
         try (OutputStream outputStream = Files.newOutputStream(path, CREATE_NEW)) {
-            size = IOUtils.copyLarge(inputStream, outputStream);
+            size = IOUtils.copyLarge(inputStream, outputStream, 0, maxAllowedSize);
+
+            if (size >= maxAllowedSize) {
+                if (inputStream.read() != IOUtils.EOF) {
+                    outputStream.close();
+                    if (path.toFile().exists()) path.toFile().delete();
+
+                    throw new FileStorageException(FileStorageException.Type.IO_EXCEPTION,
+                            String.format("File is too large: '%s'. Max file size = %s MB is exceeded but there are unread bytes left.",
+                                    path.toAbsolutePath(),
+                                    properties.getMaxFileSize().toMegabytes()));
+                }
+            }
             outputStream.flush();
 //            writeLog(path, false);
         } catch (IOException e) {
@@ -225,6 +237,11 @@ public InputStream openStream(FileRef reference) {
             }
 
             try {
+                if (!Boolean.TRUE.equals(disablePathCheck) && !path.toRealPath().startsWith(root.toRealPath())) {
+                    log.error("File '{}' is outside of root dir '{}': ", path, root);
+                    continue;
+                }
+
                 inputStream = Files.newInputStream(path);
             } catch (IOException e) {
                 log.error("Error opening input stream for " + path, e);

jmix-localfs/localfs/src/main/java/io/jmix/localfs/LocalFileStorageProperties.java

@@ -17,6 +17,8 @@
 package io.jmix.localfs;
 
 import org.springframework.boot.context.properties.ConfigurationProperties;
+import org.springframework.boot.context.properties.bind.DefaultValue;
+import org.springframework.util.unit.DataSize;
 
 @ConfigurationProperties(prefix = "jmix.localfs")
 public class LocalFileStorageProperties {
@@ -26,9 +28,16 @@ public class LocalFileStorageProperties {
      */
     String storageDir;
 
+    /**
+     * Maximum allowable file size.
+     */
+    DataSize maxFileSize;
+
     public LocalFileStorageProperties(
-            String storageDir) {
+            String storageDir,
+            @DefaultValue("100MB") DataSize maxFileSize) {
         this.storageDir = storageDir;
+        this.maxFileSize = maxFileSize;
     }
 
     /**
@@ -37,4 +46,11 @@ public LocalFileStorageProperties(
     public String getStorageDir() {
         return storageDir;
     }
+
+    /**
+     * @see #maxFileSize
+     */
+    public DataSize getMaxFileSize() {
+        return maxFileSize;
+    }
 }

jmix-rest/rest/src/main/java/io/jmix/rest/RestProperties.java

@@ -22,6 +22,7 @@
 
 import java.util.Collections;
 import java.util.Map;
+import java.util.Set;
 
 @ConfigurationProperties(prefix = "jmix.rest")
 public class RestProperties {
@@ -38,15 +39,22 @@ public class RestProperties {
     private final int defaultMaxFetchSize;
     private final Map<String, Integer> entityMaxFetchSize;
 
+    /**
+     * File extensions that can be opened for viewing in a browser by replying with 'Content-Disposition=inline' header.
+     */
+    protected Set<String> inlineEnabledFileExtensions;
+
     public RestProperties(
             @DefaultValue("false") boolean optimisticLockingEnabled,
             @DefaultValue("true") boolean responseFetchPlanEnabled,
             @DefaultValue("10000") int defaultMaxFetchSize,
+            @DefaultValue({"jpg", "png", "jpeg", "pdf"}) Set<String> inlineEnabledFileExtensions,
             @Nullable Map<String, Integer> entityMaxFetchSize) {
         this.optimisticLockingEnabled = optimisticLockingEnabled;
         this.responseFetchPlanEnabled = responseFetchPlanEnabled;
         this.defaultMaxFetchSize = defaultMaxFetchSize;
         this.entityMaxFetchSize = entityMaxFetchSize == null ? Collections.emptyMap() : entityMaxFetchSize;
+        this.inlineEnabledFileExtensions = inlineEnabledFileExtensions;
     }
 
     /**
@@ -63,6 +71,13 @@ public boolean isResponseFetchPlanEnabled() {
         return responseFetchPlanEnabled;
     }
 
+    /**
+     * @see #inlineEnabledFileExtensions
+     */
+    public Set<String> getInlineEnabledFileExtensions() {
+        return inlineEnabledFileExtensions;
+    }
+
     public int getEntityMaxFetchSize(String entityName) {
         return entityMaxFetchSize.getOrDefault(entityName, defaultMaxFetchSize);
     }

jmix-rest/rest/src/main/java/io/jmix/rest/impl/controller/FileDownloadController.java

@@ -17,11 +17,16 @@
 package io.jmix.rest.impl.controller;
 
 import io.jmix.core.AccessManager;
-import io.jmix.core.FileTransferService;
 import io.jmix.core.FileRef;
+import io.jmix.core.FileTransferService;
 import io.jmix.core.Metadata;
+import io.jmix.rest.RestProperties;
 import io.jmix.rest.accesscontext.RestFileDownloadContext;
 import io.jmix.rest.exception.RestAPIException;
+import jakarta.servlet.http.HttpServletResponse;
+import org.apache.commons.io.FilenameUtils;
+import org.apache.commons.lang3.BooleanUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -31,7 +36,7 @@
 import org.springframework.web.bind.annotation.RequestParam;
 import org.springframework.web.bind.annotation.RestController;
 
-import jakarta.servlet.http.HttpServletResponse;
+import java.util.Set;
 
 /**
  * REST API controller that is used for downloading files
@@ -48,6 +53,8 @@ public class FileDownloadController {
     protected AccessManager accessManager;
     @Autowired
     protected FileTransferService fileTransferService;
+    @Autowired
+    protected RestProperties restProperties;
 
     @GetMapping
     public void downloadFile(@RequestParam String fileRef,
@@ -58,6 +65,7 @@ public void downloadFile(@RequestParam String fileRef,
         try {
             FileRef fileReference;
             fileReference = FileRef.fromString(fileRef);
+            attachment = resolveAttachmentValue(attachment, fileReference);
             fileTransferService.downloadAndWriteResponse(fileReference, fileReference.getStorageName(), attachment, response);
         } catch (IllegalArgumentException e) {
             throw new RestAPIException("Invalid file reference",
@@ -75,4 +83,21 @@ protected void checkFileDownloadPermission() {
             throw new RestAPIException("File download failed", "File download is not permitted", HttpStatus.FORBIDDEN);
         }
     }
+
+    protected boolean resolveAttachmentValue(Boolean attachmentRequestParameterValue, FileRef fileRef) {
+        if (BooleanUtils.isTrue(attachmentRequestParameterValue)) {
+            return true;
+        }
+
+        String fileName = fileRef.getFileName();
+        String extension = FilenameUtils.getExtension(fileName);
+        if (StringUtils.isEmpty(extension)) {
+            // No extension - just download
+            return true;
+        } else {
+            // Check if file is allowed to be opened inline
+            Set<String> inlineEnabledFileExtensions = restProperties.getInlineEnabledFileExtensions();
+            return !inlineEnabledFileExtensions.contains(StringUtils.lowerCase(extension));
+        }
+    }
 }