Merge pull request #39 from code-lts/security-fix

Fix remote arbitrary file upload in any directory of the file system
dilab · Dec 17, 2023 · 3c6dbf5 · 3c6dbf5
2 parents 62c3fff + d3552ef
commit 3c6dbf5
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 14 deletions.
diff --git a/composer.json b/composer.json
@@ -10,7 +10,8 @@
   "require": {
     "php": ">=8.1.0",
     "cakephp/filesystem": "^3.0",
-    "monolog/monolog": "^2.0"
+    "monolog/monolog": "^2.0",
+    "ondrej-vrto/php-filename-sanitize": "^1.4"
   },
   "require-dev": {
     "phpunit/phpunit": "~10.0"

diff --git a/src/Resumable.php b/src/Resumable.php
@@ -8,6 +8,7 @@
 use Dilab\Network\Response;
 use Monolog\Logger;
 use Monolog\Handler\StreamHandler;
+use OndrejVrto\FilenameSanitize\FilenameSanitize;
 
 class Resumable
 {
@@ -159,18 +160,14 @@ public function getExtension()
     }
 
     /**
-     * Makes sure the orginal extension never gets overriden by user defined filename.
+     * Creates a safe name
      *
-     * @param string User defined filename
-     * @param string Original filename
-     * @return string Filename that always has an extension from the original file
+     * @param string $name Original name
+     * @return string A safer name
      */
-    private function createSafeFilename($filename, $originalFilename)
+    private function createSafeName(string $name): string
     {
-        $filename = $this->removeExtension($filename);
-        $extension = $this->findExtension($originalFilename);
-
-        return sprintf('%s.%s', $filename, $extension);
+        return FilenameSanitize::of($name)->get();
     }
 
     public function handleTestChunk()
@@ -227,9 +224,9 @@ private function createFileAndDeleteTmp($identifier, $filename)
 
         // if the user has set a custom filename
         if (null !== $this->filename) {
-            $finalFilename = $this->createSafeFilename($this->filename, $filename);
+            $finalFilename = $this->createSafeName($this->filename);
         } else {
-            $finalFilename = $filename;
+            $finalFilename = $this->createSafeName($filename);
         }
 
         // replace filename reference by the final file
@@ -288,7 +285,7 @@ public function tmpChunkDir($identifier)
         if (!empty($this->instanceId)){
             $tmpChunkDir .= $this->instanceId . DIRECTORY_SEPARATOR;
         }
-        $tmpChunkDir .= $identifier;
+        $tmpChunkDir .= $this->createSafeName($identifier);
         $this->ensureDirExists($tmpChunkDir);
         return $tmpChunkDir;
     }
@@ -318,7 +315,7 @@ private function ensureDirExists($path)
 
     public function tmpChunkFilename($filename, $chunkNumber)
     {
-        return $filename . '.' . str_pad($chunkNumber, 4, 0, STR_PAD_LEFT);
+        return $this->createSafeName($filename) . '.' . str_pad($chunkNumber, 4, 0, STR_PAD_LEFT);
     }
 
     public function getExclusiveFileHandle($name)

diff --git a/test/src/ResumableTest.php b/test/src/ResumableTest.php
@@ -235,6 +235,55 @@ public function testTmpChunkFile()
         $this->assertEquals($expected, $this->resumbable->tmpChunkFilename($filename,$chunkNumber));
     }
 
+    public static function fileNameProvider(): array
+    {
+        return [
+            ['example-file.png', 'example-file.png'],
+            ['../unsafe-one-level.txt', 'unsafe-one-level.txt'],
+        ];
+    }
+
+    /**
+     * @dataProvider fileNameProvider
+     */
+    public function testResumableSanitizeFileName(string $filename, string $filenameSanitized): void
+    {
+        $resumableParams = array(
+            'resumableChunkNumber'=> 1,
+            'resumableTotalChunks'=> 1,
+            'resumableChunkSize'=>  200,
+            'resumableIdentifier'=> 'identifier',
+            'resumableFilename'=> $filename,
+            'resumableRelativePath'=> 'upload',
+        );
+
+
+        $this->request->method('is')
+            ->willReturn(true);
+
+        $this->request->method('data')
+                ->willReturn($resumableParams);
+
+        $this->request->method('file')
+                ->willReturn(array(
+                    'name'=> 'mock.png',
+                    'tmp_name'=>  'test/files/mock.png.0003',
+                    'error'=> 0,
+                    'size'=> 27000,
+                ));
+
+        $this->resumbable = new Resumable($this->request, $this->response);
+        $this->resumbable->tempFolder = 'test/tmp';
+        $this->resumbable->uploadFolder = 'test/uploads';
+        $this->resumbable->deleteTmpFolder = false;
+        $this->resumbable->handleChunk();
+
+        $this->assertFileExists('test/uploads/' . $filenameSanitized);
+        $this->assertFileExists('test/tmp/identifier/' . $filenameSanitized . '.0001');
+        $this->assertTrue(unlink('test/tmp/identifier/' . $filenameSanitized . '.0001'));
+        $this->assertTrue(unlink('test/uploads/' . $filenameSanitized));
+    }
+
     public function testCreateFileFromChunks()
     {
         $files = array(