SQL injection possible with limit() on MySQL

[mpetrovich]

The limit() query method is susceptible to catastrophic SQL injection with MySQL.

For example, given an ActiveRecord model User for a table users:

UserQuery::create()->limit('1;DROP TABLE users')->find();

This will drop the users table!

The cause appears to be a lack of integer casting of the limit input in either Propel\Runtime\ActiveQuery\Criteria::setLimit() or in Propel\Runtime\Adapter\Pdo\MysqlAdapter::applyLimit(). The code comments there seem to imply that casting was avoided due to overflow issues with 32-bit integers.

This is surprising behavior since one of the primary purposes of an ORM is to prevent basic SQL injection.

This affects all versions of Propel: 1.x, 2.x, and 3.

Related:

• propelorm/Propel#1052
• propelorm/Propel2#1463

[mpetrovich]

Out of curiosity, why not use a similar guard for the MySQL adapter as there is for the MSSQL one:

        // make sure offset and limit are numeric
        if (!is_numeric($offset) || !is_numeric($limit)) {
            throw new InvalidArgumentException('MssqlAdapter::applyLimit() expects a number for argument 2 and 3');
        }

Alternatively, in applyLimit() the limit (and offset too) could be set to zero if non-numeric:

    public function applyLimit(&$sql, $offset, $limit)
    {
        $offset = is_numeric($offset) ? $offset : 0;
        $limit = is_numeric($limit) ? $limit : 0;

        if ($limit >= 0) {
            $sql .= ' LIMIT ' . ($offset > 0 ? $offset . ', ' : '') . $limit;
        } elseif ($offset > 0) {
            $sql .= ' LIMIT ' . $offset . ', 18446744073709551615';
        }
    }

[mpetrovich]

@marcj, what would be the preferred approach to handling invalid limits or offsets? Throw an exception or default to zero? (I'm working on a PR for this and want to make sure I'm following Propel best practices here)

[marcj]

I'd go with 0

[halfer]

For general readers, I'm prepping a security advisory on this ticket.

[mpetrovich]

Pull request with a fix: #74