#22 Fix bug with password protected shares

files/src/main/java/de/nbscloud/files/FileSystemService.java

@@ -88,6 +88,7 @@ public class FileSystemService {
     private MimeTypeDetector mimeTypeDetector = new MimeTypeDetector();
 
     public Path createDirectory(String name) {
+
         try {
             return Files.createDirectories(this.locationTracker.resolve(name));
         } catch (IOException e) {
@@ -96,6 +97,8 @@ public class FileSystemService {
     }
 
     void createDirectory(Path path) {
+        this.locationTracker.ensureValidPath(path);
+
         try {
             Files.createDirectories(path);
         } catch (IOException e) {
@@ -104,14 +107,12 @@ public class FileSystemService {
     }
 
     public void createFile(String name, byte[] content) {
-        try {
+        createFile(this.locationTracker.resolve(name), content);
-            Files.write(this.locationTracker.resolve(name), content, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW);
-        } catch (IOException e) {
-            throw new FileSystemServiceException("Could not create file", e);
-        }
     }
 
     public void createFile(Path path, byte[] content) {
+        this.locationTracker.ensureValidPath(path);
+
         try {
             Files.write(path, content, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW);
         } catch (IOException e) {
@@ -120,6 +121,8 @@ public class FileSystemService {
     }
 
     public void overwriteFile(Path path, byte[] content) {
+        this.locationTracker.ensureValidPath(path);
+
         try {
             Files.write(path, content, StandardOpenOption.WRITE, StandardOpenOption.TRUNCATE_EXISTING);
         } catch (IOException e) {
@@ -128,6 +131,9 @@ public class FileSystemService {
     }
 
     public Path move(Path originalPath, Path newPath) {
+        this.locationTracker.ensureValidPath(originalPath);
+        this.locationTracker.ensureValidPath(newPath);
+
         try {
             return Files.move(originalPath, newPath, StandardCopyOption.ATOMIC_MOVE);
         } catch (IOException e) {
@@ -136,15 +142,12 @@ public class FileSystemService {
     }
 
     public boolean delete(String name) {
-        try {
+        return delete(this.locationTracker.resolve(name));
-            // TODO does only delete dirs if they are empty - but maybe we want that?
-            return Files.deleteIfExists(this.locationTracker.resolve(name));
-        } catch (IOException e) {
-            throw new FileSystemServiceException("Could not delete file", e);
-        }
     }
 
     public boolean delete(Path path) {
+        this.locationTracker.ensureValidPath(path);
+
         try {
             // TODO does only delete dirs if they are empty - but maybe we want that?
             return Files.deleteIfExists(path);
@@ -154,6 +157,8 @@ public class FileSystemService {
     }
 
     public byte[] get(Path path) {
+        this.locationTracker.ensureValidPath(path);
+
         try {
             return Files.readAllBytes(path);
         } catch (IOException e) {
@@ -170,6 +175,8 @@ public class FileSystemService {
     }
 
     public void stream(Path targetPath, ZipOutputStream outputStream) {
+        this.locationTracker.ensureValidPath(targetPath);
+
         try {
             Files.walkFileTree(targetPath, new SimpleFileVisitor<Path>() {
                 @Override
@@ -198,14 +205,12 @@ public class FileSystemService {
     }
 
     public InputStream stream(String name) {
-        try {
+        return stream(this.locationTracker.resolve(name));
-            return Files.newInputStream(this.locationTracker.resolve(name));
-        } catch (IOException e) {
-            throw new FileSystemServiceException("Could not stream file", e);
-        }
     }
 
-    private InputStream stream(Path name) {
+    public InputStream stream(Path name) {
+        this.locationTracker.ensureValidPath(name);
+
         try {
             return Files.newInputStream(name);
         } catch (IOException e) {
@@ -214,18 +219,16 @@ public class FileSystemService {
     }
 
     public long getSize(String name) {
-        try {
+        return getSize(this.locationTracker.resolve(name));
-            return Files.size(this.locationTracker.resolve(name));
-        } catch (IOException e) {
-            throw new FileSystemServiceException("Could not get file", e);
-        }
     }
 
     public long getSize(Path name) {
+        this.locationTracker.ensureValidPath(name);
+
         try {
             return Files.size(name);
         } catch (IOException e) {
-            throw new FileSystemServiceException("Could not get file", e);
+            throw new FileSystemServiceException("Could not get file size", e);
         }
     }
 
@@ -258,10 +261,16 @@ public class FileSystemService {
     }
 
     public String getMimeType(String name) {
-        try {
+        return getMimeType(this.locationTracker.resolve(name));
-            final String detectedMimeType = this.mimeTypeDetector.detectMimeType(this.locationTracker.resolve(name));
+    }
 
-            logger.debug("Detected mime type {} for file {}", detectedMimeType, name);
+    public String getMimeType(Path path) {
+        this.locationTracker.ensureValidPath(path);
+
+        try {
+            final String detectedMimeType = this.mimeTypeDetector.detectMimeType(path);
+
+            logger.debug("Detected mime type {} for file {}", detectedMimeType, path);
 
             return detectedMimeType;
         } catch (GetBytesException e) {
@@ -295,6 +304,8 @@ public class FileSystemService {
     }
 
     List<ContentContainer> list(Path startPath, SortOrder sortOrder, Function<Path, Path> relativizer) {
+        this.locationTracker.ensureValidPath(startPath);
+
         try {
             List<ContentContainer> contentList = Files.list(startPath)
                                                       .filter(path -> {
@@ -330,6 +341,8 @@ public class FileSystemService {
     }
 
     ContentTree getTree(Path startPath, SortOrder sortOrder, Function<Path, Path> relativizer) {
+        this.locationTracker.ensureValidPath(startPath);
+
         try {
             if (!Files.isDirectory(startPath)) {
                 return null;
@@ -399,6 +412,9 @@ public class FileSystemService {
     }
 
     List<String> collectDirs(Path sourcePath, Path baseDir, Function<Path, Path> relativizer, boolean includeSource) {
+        this.locationTracker.ensureValidPath(sourcePath);
+        this.locationTracker.ensureValidPath(baseDir);
+
         try {
             final List<String> resultList = new ArrayList<>();
             final boolean sourceIsDir = Files.isDirectory(sourcePath);

files/src/main/java/de/nbscloud/files/LocationTracker.java

@@ -106,6 +106,25 @@ public class LocationTracker implements InitializingBean {
         }
     }
 
+    public void ensureValidPath(Path path) {
+        // The provided systemd unit file restricts access to the nbscloud/ directory
+        // but better safe than sorry - user could roll their own service after all
+
+        if(path == null) {
+            throw new IllegalStateException("Null");
+        }
+
+        if(path.toString().contains("..")) {
+            throw new IllegalStateException("Relative path");
+        }
+
+        // Regardless of where we navigate to, we must never leave the base directory
+        // The normalize() call is important, as it resolves (possible) relative paths
+        if(!path.normalize().startsWith(this.baseDirPath)) {
+            throw new IllegalStateException("Illegal path: " + path);
+        }
+    }
+
     public Path resolve(String name) {
         validate_internal(name);
 

files/src/main/java/de/nbscloud/files/controller/FilesController.java

@@ -344,9 +344,9 @@ public class FilesController implements InitializingBean {
 
             return ResponseEntity.ok()
                                  .header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=" + filename)
-                                 .header(HttpHeaders.CONTENT_TYPE, this.fileSystemService.getMimeType(filename))
+                                 .header(HttpHeaders.CONTENT_TYPE, this.fileSystemService.getMimeType(sharedFilePath))
-                                 .header(HttpHeaders.CONTENT_LENGTH, String.valueOf(this.fileSystemService.getSize(filename)))
+                                 .header(HttpHeaders.CONTENT_LENGTH, String.valueOf(this.fileSystemService.getSize(sharedFilePath)))
-                                 .body(new InputStreamResource(new ObservableInputStream(this.fileSystemService.stream(filename), new ObservableInputStream.Observer() {
+                                 .body(new InputStreamResource(new ObservableInputStream(this.fileSystemService.stream(sharedFilePath), new ObservableInputStream.Observer() {
                                      @Override
                                      public void closed() throws IOException {
                                          if (share.isOneTime()) {

web-container/src/main/resources/static/changelog.txt

@@ -1,5 +1,8 @@
+v19:
+- #22 Fix a bug with password protected shares
+
 v18:
-- Password protected shares
+- #22 Password protected shares
 - Basic Note app implementation
 - Files app now offers API for other apps to store files