CVE-2023-22727: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: CakePHP is a development framework for PHP web apps. In affected versions the `Cake\Database\Query::limit()` and `Cake\Database\Query::offset()` methods are vulnerable to SQL injection if passed un-sanitized user request data. This issue has been fixed in 4.2.12, 4.3.11, 4.4.10. Users are advised to upgrade. Users unable to upgrade may mitigate this issue by using CakePHP's Pagination library. Manually validating or casting parameters to these methods will also mitigate the issue.
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
CVSS v3 base score: 9.8
Published: 2023-01-17T21:15Z
Repository: https://github.com/cakephp/cakephp
Language: PHP
Files changed: src/Database/Query.php, tests/TestCase/Database/QueryTest.php
Commit date: 2023-01-03 15:29:38 -0500
Commit message: Disallow strings in limit() and offset()

Back in 68a08a2 int casts were removed in limit() and offset(). This
opened up the possibility for applications to provide unsafe user input
into these methods and create an injection vector.

We've been trying to narrow the accepted types, so instead of adding
casting I've added an exception when strings that are not number shaped
are provided.

Diff: diff --git a/src/Database/Query.php b/src/Database/Query.php
index 98f7bbc1e77..e96e246f175 100644
--- a/src/Database/Query.php
+++ b/src/Database/Query.php
@@ -1532,10 +1532,13 @@ class Query implements ExpressionInterface, IteratorAggregate
      * @param \Cake\Database\ExpressionInterface|int|null $limit number of records to be returned
      * @return $this
      */
     public function limit($limit)
     {
+        if (is_string($limit) && !is_numeric($limit)) {
+            throw new InvalidArgumentException('Invalid value for `limit()`');
+        }
         $this->_dirty();
         $this->_parts['limit'] = $limit;
 
         return $this;
     }
@@ -1558,10 +1561,13 @@ class Query implements ExpressionInterface, IteratorAggregate
      * @param \Cake\Database\ExpressionInterface|int|null $offset number of records to be skipped
      * @return $this
      */
     public function offset($offset)
     {
+        if (is_string($offset) && !is_numeric($offset)) {
+            throw new InvalidArgumentException('Invalid value for `offset()`');
+        }
         $this->_dirty();
         $this->_parts['offset'] = $offset;
 
         return $this;
     }
diff --git a/tests/TestCase/Database/QueryTest.php b/tests/TestCase/Database/QueryTest.php
index 3b80b07bfad..9169a39ac02 100644
--- a/tests/TestCase/Database/QueryTest.php
+++ b/tests/TestCase/Database/QueryTest.php
@@ -2216,10 +2216,36 @@ class QueryTest extends TestCase
         $query = new Query($this->connection);
         $result = $query->select('id')->from('articles')->limit(2)->execute();
         $this->assertCount(2, $result);
     }
 
+    /**
+     * Tests selecting rows with string offset/limit
+     */
+    public function testSelectLimitInvalid(): void
+    {
+        $query = new Query($this->connection);
+        $this->expectException(InvalidArgumentException::class);
+        $query->select('id')->from('comments')
+            ->limit('1 --more')
+            ->order(['id' => 'ASC'])
+            ->execute();
+    }
+
+    /**
+     * Tests selecting rows with string offset/limit
+     */
+    public function testSelectOffsetInvalid(): void
+    {
+        $query = new Query($this->connection);
+        $this->expectException(InvalidArgumentException::class);
+        $query->select('id')->from('comments')
+            ->offset('1 --more')
+            ->order(['id' => 'ASC'])
+            ->execute();
+    }
+
     /**
      * Tests selecting rows combining a limit and offset clause
      */
     public function testSelectOffset(): void
     {

Fixed code:         if (is_string($limit) && !is_numeric($limit)) {
            throw new InvalidArgumentException('Invalid value for `limit()`');
        }
        if (is_string($offset) && !is_numeric($offset)) {
            throw new InvalidArgumentException('Invalid value for `offset()`');
        }
    /**
     * Tests selecting rows with string offset/limit
     */
    public function testSelectLimitInvalid(): void
    {
        $query = new Query($this->connection);
        $this->expectException(InvalidArgumentException::class);
        $query->select('id')->from('comments')
            ->limit('1 --more')
            ->order(['id' => 'ASC'])
            ->execute();
    }

    /**
     * Tests selecting rows with string offset/limit
     */
    public function testSelectOffsetInvalid(): void
    {
        $query = new Query($this->connection);
        $this->expectException(InvalidArgumentException::class);
        $query->select('id')->from('comments')
            ->offset('1 --more')
            ->order(['id' => 'ASC'])
            ->execute();
    }