CVE-2023-3491: Unrestricted Upload of File with Dangerous Type (CWE-434)
value: Unrestricted Upload of File with Dangerous Type in GitHub repository fossbilling/fossbilling prior to 0.5.3.
CWE description: The product allows the upload or transfer of dangerous file types that are automatically processed within its environment.
CVSS v3 base score: 8.8
Published: 2023-06-30T22:15Z
Repository: https://github.com/fossbilling/fossbilling
Language: PHP
Files changed: src/library/Box/Pagination.php, src/modules/Client/Service.php, src/modules/Theme/Controller/Admin.php, src/modules/Theme/Service.php, tests/integration/bb-modules/mod_theme/ServiceTest.php, tests/modules/Theme/Controller/AdminTest.php, tests/modules/Theme/ServiceTest.php
Commit date: 2023-06-30 22:51:50 +0200
Commit message: Limit "LIMIT" to numbers only + Disable upload theme  (#1392)

* Prevent non numeric values being used in limits

Potential abuse for Sql injection
Only allow integers to be used


Adjust exception

* Disable upload assets via Theme pages

File upload was removed in an earlier PR

* Make sure the test run fine

* Fix the tests

* Use limit instead of per_page

* And another fix

---------

Co-authored-by: Belle Aerni <belleaerni@gmail.com>
Diff: diff --git a/src/library/Box/Pagination.php b/src/library/Box/Pagination.php
index 3a56d0bd7..d387c9cc2 100644
--- a/src/library/Box/Pagination.php
+++ b/src/library/Box/Pagination.php
@@ -31,21 +31,28 @@ class Box_Pagination implements InjectionAwareInterface
     public function getPer_page()
     {
         return $this->per_page;
     }
 
-    public function getSimpleResultSet($q, $values, $per_page = 100, $page = null)
+    public function getSimpleResultSet($q, $values, ?int $per_page = 100, ?int $page = null)
     {
         if (is_null($page)){
             $page = $_GET['page'] ?? 1;
         }
-        $per_page = $_GET['per_page'] ?? $per_page;
+        $per_page = $_GET['per_page'] ?? $per_page ?? 100;
+
+        if (!is_numeric($page) || $page < 1 ){
+           throw new \Box_Exception('Invalid page number');
+        }
+        if (!is_numeric($per_page) || $per_page < 1 ){
+           throw new \Box_Exception('Invalid per page number');
+        }
 
         $offset = ($page - 1) * $per_page;
 
         $sql = $q;
-        $sql .= sprintf(' LIMIT %s,%s', $offset, $per_page);
+        $sql .= sprintf(' LIMIT %u,%u', $offset, $per_page);
         $result = $this->di['db']->getAll($sql, $values);
 
         $exploded = explode('FROM', $q);
         $sql = 'SELECT count(1) FROM ' . $exploded[1];
         $total = $this->di['db']->getCell($sql , $values);
@@ -58,18 +65,25 @@ class Box_Pagination implements InjectionAwareInterface
             "total"             => $total,
             "list"              => $result,
         );
     }
 
-    public function getAdvancedResultSet($q, $values, $per_page = 100)
+    public function getAdvancedResultSet($q, $values, ?int $per_page = 100)
     {
-        $page = $page = $_GET['page'] ?? 1;
-        $per_page = $_GET['per_page'] ?? $per_page;
+        $page = $_GET['page'] ?? 1;
+        $per_page = $_GET['per_page'] ?? $per_page ?? 100;
+
+        if (!is_numeric($page) || $page < 1 ){
+           throw new \Box_Exception('Invalid page number');
+        }
+        if (!is_numeric($per_page) || $per_page < 1 ){
+           throw new \Box_Exception('Invalid per page number');
+        }
 
         $offset = ($page - 1) * $per_page;
         $q = str_replace('SELECT ', 'SELECT SQL_CALC_FOUND_ROWS ', $q);
-        $q .= sprintf(' LIMIT %s,%s', $offset, $per_page);
+        $q .= sprintf(' LIMIT %u,%u', $offset, $per_page);
         $result = $this->di['db']->getAll($q, $values);
         $total = $this->di['db']->getCell('SELECT FOUND_ROWS();');
 
         $pages = ($per_page > 1) ? (int)ceil($total / $per_page) : 1;
         return array(
diff --git a/src/modules/Client/Service.php b/src/modules/Client/Service.php
index a788e1a9b..2f7c6ef85 100644
--- a/src/modules/Client/Service.php
+++ b/src/modules/Client/Service.php
@@ -175,12 +175,16 @@ class Service implements InjectionAwareInterface
     }
 
     public function getPairs($data)
     {
         $limit = $data['per_page'] ?? 30;
+        if (!is_numeric($limit) || $limit < 1 ){
+            throw new \Box_Exception('Invalid per page number');
+        }
+
         [$sql, $params] = $this->getSearchQuery($data, "SELECT c.id, CONCAT_WS('', c.first_name,  ' ', c.last_name) as full_name");
-        $sql = $sql . ' LIMIT ' . $limit;
+        $sql .= sprintf(' LIMIT %u', $limit);
 
         return $this->di['db']->getAssoc($sql, $params);
     }
 
     public function toSessionArray(\Model_Client $model)
diff --git a/src/modules/Theme/Controller/Admin.php b/src/modules/Theme/Controller/Admin.php
index 60bcc1fda..b6ee776a3 100644
--- a/src/modules/Theme/Controller/Admin.php
+++ b/src/modules/Theme/Controller/Admin.php
@@ -61,11 +61,10 @@ class Admin implements \FOSSBilling\InjectionAwareInterface
         try {
             if (!$t->isAssetsPathWritable()) {
                 throw new \Box_Exception('Theme ":name" assets folder is not writable. Files can not be uploaded and settings can not be saved. Set folder permissions to 777', [':name' => $t->getName()]);
             }
             $service->updateSettings($t, $preset, $_POST);
-            $service->uploadAssets($t, $_FILES);
             $service->regenerateThemeCssAndJsFiles($t, $preset, $api);
         } catch (\Exception $e) {
             error_log($e);
             $error = $e->getMessage();
         }
diff --git a/src/modules/Theme/Service.php b/src/modules/Theme/Service.php
index 361a069e4..b9146eb47 100644
--- a/src/modules/Theme/Service.php
+++ b/src/modules/Theme/Service.php
@@ -159,28 +159,10 @@ class Service implements InjectionAwareInterface
         } else {
             return $theme->getPresetFromSettingsDataFile($preset);
         }
     }
 
-    public function uploadAssets(Model\Theme $theme, array $files)
-    {
-        $dest = $theme->getPathAssets() . DIRECTORY_SEPARATOR;
-
-        foreach ($files as $filename => $f) {
-            if (UPLOAD_ERR_NO_FILE == $f['error']) {
-                continue;
-            }
-
-            $filename = str_replace('_', '.', $filename);
-            if (UPLOAD_ERR_OK != $f['error']) {
-                throw new \Box_Exception('Error uploading file :file Error code: :error', [':file' => $filename, ':error' => $f['error']]);
-            }
-
-            move_uploaded_file($f['tmp_name'], $dest . $filename);
-        }
-    }
-
     public function updateSettings(Model\Theme $theme, $preset, array $params)
     {
         $meta = $this->di['db']->findOne(
             'ExtensionMeta',
             "extension = 'mod_theme' AND rel_type = 'settings' AND rel_id = :theme AND meta_key = :preset",
diff --git a/tests/integration/bb-modules/mod_theme/ServiceTest.php b/tests/integration/bb-modules/mod_theme/ServiceTest.php
index 0f09ba27d..f5ff8a4f5 100644
--- a/tests/integration/bb-modules/mod_theme/ServiceTest.php
+++ b/tests/integration/bb-modules/mod_theme/ServiceTest.php
@@ -32,30 +32,10 @@ class Box_Mod_Theme_ServiceTest extends BBDbApiTestCase
         $themeModel = $service->getTheme('huraga');
         $result = $service->getThemeSettings($themeModel);
         $this->assertIsArray($result);
     }
 
-    public function testuploadAssets()
-    {
-        $files = array(
-            'file1' => array(
-                    'error' => UPLOAD_ERR_NO_FILE,
-                ),
-            'file2' => array(
-                'error' => UPLOAD_ERR_OK,
-                'tmp_name' => 'tmpName',
-            ),
-
-        );
-
-        $service = new \Box\Mod\Theme\Service();
-        $service->setDi($this->di);
-
-        $themeModel = $service->getTheme('huraga');
-        $service->uploadAssets($themeModel, $files);
-    }
-
     public function testupdateSettings()
     {
         $service = new \Box\Mod\Theme\Service();
         $service->setDi($this->di);
 
@@ -109,6 +89,6 @@ class Box_Mod_Theme_ServiceTest extends BBDbApiTestCase
 
         $result = $service->getCurrentClientAreaTheme();
         $this->assertInstanceOf('\Box\Mod\Theme\Model\Theme', $result);
         $this->assertEquals('huraga', $result->getName());
     }
-}
\ No newline at end of file
+}
diff --git a/tests/modules/Theme/Controller/AdminTest.php b/tests/modules/Theme/Controller/AdminTest.php
index 16c6162b3..763d3c66d 100644
--- a/tests/modules/Theme/Controller/AdminTest.php
+++ b/tests/modules/Theme/Controller/AdminTest.php
@@ -105,12 +105,10 @@ class AdminTest extends \BBTestCase {
             ->willReturn('default');
         $themeServiceMock->expects($this->atLeastOnce())
             ->method('setCurrentThemePreset');
         $themeServiceMock->expects($this->atLeastOnce())
             ->method('updateSettings');
-        $themeServiceMock->expects($this->atLeastOnce())
-            ->method('uploadAssets');
         $themeServiceMock->expects($this->atLeastOnce())
             ->method('regenerateThemeCssAndJsFiles');
         $themeServiceMock->expects($this->atLeastOnce())
             ->method('regenerateThemeSettingsDataFile');
 
@@ -199,6 +197,5 @@ class AdminTest extends \BBTestCase {
         $_POST['save-current-setting'] = true;
         $controller->save_theme_settings($boxAppMock, 'huraga');
     }
 
 }
- 
\ No newline at end of file
diff --git a/tests/modules/Theme/ServiceTest.php b/tests/modules/Theme/ServiceTest.php
index b34ee69c5..be1943986 100644
--- a/tests/modules/Theme/ServiceTest.php
+++ b/tests/modules/Theme/ServiceTest.php
@@ -373,38 +373,7 @@ class ServiceTest extends \BBTestCase {
         $result = $this->service->getCurrentClientAreaThemeCode();
         $this->assertIsString($result);
         $this->assertEquals('huraga', $result);
     }
 
-    public function testuploadAssets()
-    {
-        $themeMock = $this->getMockBuilder('\Box\Mod\Theme\Model\Theme')->disableOriginalConstructor()->getMock();
-        $themeMock->expects($this->atLeastOnce())
-            ->method('getPathAssets');
-        $files = array(
-            'test2' => array(
-                'error' => UPLOAD_ERR_NO_FILE
-                ),
-            'test1' => array(
-                    'error' => UPLOAD_ERR_OK,
-                    'tmp_name' => 'tempName',
-                ),
-        );
-        $this->service->uploadAssets($themeMock, $files);
-    }
-
-    public function testuploadAssets_Exception()
-    {
-        $themeMock = $this->getMockBuilder('\Box\Mod\Theme\Model\Theme')->disableOriginalConstructor()->getMock();
-        $themeMock->expects($this->atLeastOnce())
-            ->method('getPathAssets');
-        $files = array(
-            'test0' => array(
-                'error' => UPLOAD_ERR_CANT_WRITE
-            ),
-        );
-        $this->expectException(\Box_Exception::class);
-        $this->expectExceptionMessage(sprintf("Error uploading file %s Error code: %d", 'test0', UPLOAD_ERR_CANT_WRITE));
-        $this->service->uploadAssets($themeMock, $files);
-    }
 
 }

Vulnerable code:     public function getSimpleResultSet($q, $values, $per_page = 100, $page = null)
        $per_page = $_GET['per_page'] ?? $per_page;
        $sql .= sprintf(' LIMIT %s,%s', $offset, $per_page);
    public function getAdvancedResultSet($q, $values, $per_page = 100)
        $page = $page = $_GET['page'] ?? 1;
        $per_page = $_GET['per_page'] ?? $per_page;
        $q .= sprintf(' LIMIT %s,%s', $offset, $per_page);
        $sql = $sql . ' LIMIT ' . $limit;
            $service->uploadAssets($t, $_FILES);
    public function uploadAssets(Model\Theme $theme, array $files)
    {
        $dest = $theme->getPathAssets() . DIRECTORY_SEPARATOR;

        foreach ($files as $filename => $f) {
            if (UPLOAD_ERR_NO_FILE == $f['error']) {
                continue;
            }

            $filename = str_replace('_', '.', $filename);
            if (UPLOAD_ERR_OK != $f['error']) {
                throw new \Box_Exception('Error uploading file :file Error code: :error', [':file' => $filename, ':error' => $f['error']]);
            }

            move_uploaded_file($f['tmp_name'], $dest . $filename);
        }
    }

    public function testuploadAssets()
    {
        $files = array(
            'file1' => array(
                    'error' => UPLOAD_ERR_NO_FILE,
                ),
            'file2' => array(
                'error' => UPLOAD_ERR_OK,
                'tmp_name' => 'tmpName',
            ),

        );

        $service = new \Box\Mod\Theme\Service();
        $service->setDi($this->di);

        $themeModel = $service->getTheme('huraga');
        $service->uploadAssets($themeModel, $files);
    }

}
        $themeServiceMock->expects($this->atLeastOnce())
            ->method('uploadAssets');
 
    public function testuploadAssets()
    {
        $themeMock = $this->getMockBuilder('\Box\Mod\Theme\Model\Theme')->disableOriginalConstructor()->getMock();
        $themeMock->expects($this->atLeastOnce())
            ->method('getPathAssets');
        $files = array(
            'test2' => array(
                'error' => UPLOAD_ERR_NO_FILE
                ),
            'test1' => array(
                    'error' => UPLOAD_ERR_OK,
                    'tmp_name' => 'tempName',
                ),
        );
        $this->service->uploadAssets($themeMock, $files);
    }

    public function testuploadAssets_Exception()
    {
        $themeMock = $this->getMockBuilder('\Box\Mod\Theme\Model\Theme')->disableOriginalConstructor()->getMock();
        $themeMock->expects($this->atLeastOnce())
            ->method('getPathAssets');
        $files = array(
            'test0' => array(
                'error' => UPLOAD_ERR_CANT_WRITE
            ),
        );
        $this->expectException(\Box_Exception::class);
        $this->expectExceptionMessage(sprintf("Error uploading file %s Error code: %d", 'test0', UPLOAD_ERR_CANT_WRITE));
        $this->service->uploadAssets($themeMock, $files);
    }
Fixed code:     public function getSimpleResultSet($q, $values, ?int $per_page = 100, ?int $page = null)
        $per_page = $_GET['per_page'] ?? $per_page ?? 100;

        if (!is_numeric($page) || $page < 1 ){
           throw new \Box_Exception('Invalid page number');
        }
        if (!is_numeric($per_page) || $per_page < 1 ){
           throw new \Box_Exception('Invalid per page number');
        }
        $sql .= sprintf(' LIMIT %u,%u', $offset, $per_page);
    public function getAdvancedResultSet($q, $values, ?int $per_page = 100)
        $page = $_GET['page'] ?? 1;
        $per_page = $_GET['per_page'] ?? $per_page ?? 100;

        if (!is_numeric($page) || $page < 1 ){
           throw new \Box_Exception('Invalid page number');
        }
        if (!is_numeric($per_page) || $per_page < 1 ){
           throw new \Box_Exception('Invalid per page number');
        }
        $q .= sprintf(' LIMIT %u,%u', $offset, $per_page);
        if (!is_numeric($limit) || $limit < 1 ){
            throw new \Box_Exception('Invalid per page number');
        }

        $sql .= sprintf(' LIMIT %u', $limit);
}