Merge pull request #1293 from jbc25/oppia-1225-unsafe-unzipping-pattern

#1225 - solved unsafe zip issue
DigitalCampus · Oct 28, 2022 · 923a8a8 · 923a8a8
2 parents 1db1801 + 40112ec
commit 923a8a8
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 60 deletions.
diff --git a/app/src/androidTest/java/androidTestFiles/utils/FileUtilsTests.java b/app/src/androidTest/java/androidTestFiles/utils/FileUtilsTests.java
@@ -54,15 +54,12 @@ public void UnzipFiles_correctPaths() throws Exception {
 
         File zipFile = createTestZipFile();
 
-        if (zipFile != null) {
-            FileUtils.unzipFiles(context, zipFile.getParentFile().getAbsolutePath(),
-                    zipFile.getName(),
-                    zipFile.getParentFile().getAbsolutePath());
-        }
+        FileUtils.unzipFiles(context, zipFile.getParentFile().getAbsolutePath(),
+                zipFile.getName(),
+                zipFile.getParentFile().getAbsolutePath());
 
         assertEquals(FILES_COUNT + 1, zipFile.getParentFile().listFiles().length);
 
-
     }
 
     @Test
@@ -105,33 +102,6 @@ public void UnzipFiles_wrongDstDir() throws Exception {
     }
 
 
-   /* @Test
-    public void UnzipFiles_createDir(){
-        File zipFile;
-        boolean result = false;
-        try {
-            zipFile = createTestZipFile();
-
-            String srcDirectory = zipFile.getParentFile().getParentFile().getAbsolutePath();
-            String srcFile = zipFile.getParentFile().getName() + File.separator + zipFile.getName();
-            String destDirectory = zipFile.getParentFile().getParentFile().getAbsolutePath();
-
-            if (zipFile != null) {
-                result = FileUtils.unzipFiles(srcDirectory,
-                        srcFile,
-                        destDirectory);
-            }
-
-            assertTrue(result);
-            assertEquals(FILES_COUNT + 1, zipFile.getParentFile().listFiles().length);
-
-        } catch (IOException e) {
-            e.printStackTrace();
-        }
-
-    }*/
-
-
     @Test
     public void CleanDir_correctPath() {
 
@@ -272,26 +242,11 @@ public void ReadFile_String() {
         }
     }
 
-    /* @Test
-    public void FileUtils_supportedMediafileType(){
-        assertFalse(FileUtils.isSupportedMediafileType(null));
-
-        assertTrue(FileUtils.isSupportedMediafileType("video/m4v"));
-
-        assertTrue(FileUtils.isSupportedMediafileType("video/mp4"));
-
-        assertTrue(FileUtils.isSupportedMediafileType("audio/mpeg"));
-
-        assertFalse(FileUtils.isSupportedMediafileType("application/json"));
-
-    }
-*/
 
     private File createTestZipFile() throws IOException {
 
         //Create the files that will be zipped
 
-        File zipFile = null;
         File[] files = new File[FILES_COUNT];
         try {
             for (int i = 0; i < FILES_COUNT; i++) {
@@ -302,7 +257,13 @@ private File createTestZipFile() throws IOException {
             e.printStackTrace();
         }
 
-        //Create the test zip file and add the previous files to it
+        return createZipFile(files);
+
+    }
+
+    private File createZipFile(File[] files) throws IOException {
+
+        File zipFile = null;
         BufferedInputStream is;
         ZipOutputStream out = null;
         try {

diff --git a/app/src/main/java/org/digitalcampus/oppia/utils/storage/FileUtils.java b/app/src/main/java/org/digitalcampus/oppia/utils/storage/FileUtils.java
@@ -113,21 +113,23 @@ public static void unzipFiles(Context context, String srcDirectory, String srcFi
 
             while ((entry = zis.getNextEntry()) != null) {
 
-                if (entry.getName().startsWith("..")) {
+                File f = new File(destDirectory, entry.getName());
+                String canonicalPath = f.getCanonicalPath();
+
+                File fileDestDir = new File(destDirectory);
+                String destDirCanonicalPath = fileDestDir.getCanonicalPath();
+
+                if (!canonicalPath.startsWith(destDirCanonicalPath)) {
                     throw new SecurityException("Suspect file: " + entry.getName()
                             + ". Possibility of trying to access parent directory");
                 }
 
-                String outputFilename = destDirectory + File.separator + entry.getName();
-
                 createDirIfNeeded(destDirectory, entry);
 
                 int count;
 
                 byte[] data = new byte[BUFFER_SIZE];
 
-                File f = new File(outputFilename);
-
                 // write the file to the disk
                 if (!f.isDirectory()) {
 
@@ -201,12 +203,12 @@ private static void zipSubFolder(ZipOutputStream out, File folder,
         File[] fileList = folder.listFiles();
 
         for (File file : fileList) {
-                if (file.isDirectory()) {
-                    zipSubFolder(out, file, basePathLength);
-                } else {
-                    String unmodifiedFilePath = file.getPath();
-                    try (FileInputStream fi = new FileInputStream(unmodifiedFilePath);
-                         BufferedInputStream origin = new BufferedInputStream(fi, BUFFER)) {
+            if (file.isDirectory()) {
+                zipSubFolder(out, file, basePathLength);
+            } else {
+                String unmodifiedFilePath = file.getPath();
+                try (FileInputStream fi = new FileInputStream(unmodifiedFilePath);
+                     BufferedInputStream origin = new BufferedInputStream(fi, BUFFER)) {
 
                     byte[] data = new byte[BUFFER];
                     String relativePath = unmodifiedFilePath