CVE-2023-3491 (PHP, published 2023-06-30T22:15Z)
Description: value: Unrestricted Upload of File with Dangerous Type in GitHub repository fossbilling/fossbilling prior to 0.5.3.
Severity: nan (CVSS v3 base score: 8.8)
Weakness: CWE-434 \u2014 Unrestricted Upload of File with Dangerous Type
The product allows the upload or transfer of dangerous file types that are automatically processed within its environment.
Repository: https://github.com/fossbilling/fossbilling
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 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>

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);
}