ORM - Using limit with a negative number throws a MYSQL error #12419

Closed
jesusfreak3 opened this Issue Nov 22, 2016 · 4 comments

Projects

None yet

3 participants

@jesusfreak3
jesusfreak3 commented Nov 22, 2016 edited

Expected

Calling ->limit($value) on a Query should parse all input to be a non negative number.

Actual Behavior

A user attempted a MYSQL injection where a limit value was passed to the API, the value passed was

Encoded:
limit=%3CMETA%20HTTP-EQUIV%3D%22refresh%22%20CONT ENT%3D
%220%3Burl%3Djavascript%3Aqss%3D7%22%3E

Decoded:

limit=<META HTTP-EQUIV="refresh" CONT ENT= "0;url=javascript:qss=7">
$query = SomeModel::query();   
//removed in between code
$query->limit($limit); 

and the ORM treated LIMIT input as an intval so it stripped all characters except numbers and negatives which then returned "-07"

MYSQL limit does not accept negative numbers, so throws and error, and Phalcon outputs the full query to the user which then exposes more details to the user attempting to sql inject.

Response:
Syntax error, unexpected token -, near to '07', when parsing: SELECT .....

#0 [internal function]: Phalcon\Mvc\Model\Query->parse()
#1 [internal function]: Phalcon\Mvc\Model\Query->execute(Array, NULL)
#2 [internal function]: Phalcon\Mvc\Model::find(Array)
#3 ...... : Phalcon\Mvc\Model\Criteria->execute()
#4 [internal function]: {closure}()
#5 ..../index.php(19): Phalcon\Mvc\Micro->handle()
#6 {main}

Details

  • Phalcon version:
    Version => 3.0.1
    Build Date => Aug 24 2016 11:18:13

  • PHP Version: (php -v)
    PHP 5.6.15

  • Operating System:
    Windows, Ubuntu, Centos

  • Installation type:
    DLL in windows, apt-get in Ubuntu etc..

  • Zephir version (if any):
    Version 0.9.4a-dev-7e304ba18c

  • Server:
    Nginx and Apache

@sergeyklay sergeyklay added this to the 3.0.2 milestone Nov 22, 2016
@sergeyklay sergeyklay self-assigned this Nov 22, 2016
@sergeyklay
Collaborator

Thank for reporting. I'll fix it asap

@jesusfreak3

NP, and thanks for the quick reply!

@sergeyklay sergeyklay modified the milestone: 3.0.2, 3.0.3 Nov 26, 2016
@Izopi4a
Contributor
Izopi4a commented Dec 6, 2016 edited
use Phalcon\Db\Adapter\Pdo\Mysql as DbAdapter;
use Phalcon\DI\FactoryDefault;
use Phalcon\Mvc\Application;

$di = new FactoryDefault();

$di->set('db', function () {
    return new DbAdapter([
        'host' => "127.0.0.1",
        'username' => "root",
        'password' => "123456",
        'dbname' => "koko",
        "options" => [
            \PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8',
            \PDO::ATTR_EMULATE_PREPARES => false,
            \PDO::ATTR_DEFAULT_FETCH_MODE => \PDO::FETCH_ASSOC,
            \PDO::ATTR_STRINGIFY_FETCHES => false
        ]
    ]);
});

class koko extends \Phalcon\Mvc\Model {

    public function initialize()
    {
        $this->setSource("Persons");
    }
}

$application = new Application($di);


$query = koko::query();
//removed in between code
$query->limit(-7);
//$query->limit('<META HTTP-EQUIV="refresh" CONT ENT= "0;url=javascript:qss=7">');
//$query->limit("%3CMETA%20HTTP-EQUIV%3D%22refresh%22%20CONT ENT%3D%220%3Burl%3Djavascript%3Aqss%3D7%22%3E");

$a = $query->execute();

echo "<pre>";
var_dump($a->toArray()); //prints all my db results
echo "</pre>";
exit;

php 7 0 13
mysql 5 7 16

maybe we can tune a little bit here https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model/criteria.zep#L512

and here https://github.com/phalcon/cphalcon/blob/master/phalcon/mvc/model/query/builder.zep#L941

to change var to int and offset only if limit is present stuff like that

what do you think @sergeyklay

@sergeyklay sergeyklay referenced this issue Dec 18, 2016
Merged

Fixed Criteria::limit and Builder::limit #12486

3 of 3 tasks complete
@sergeyklay
Collaborator

Fixed in the 3.0.x branch.

@sergeyklay sergeyklay closed this Dec 18, 2016
@sergeyklay sergeyklay assigned sergeyklay and unassigned sergeyklay Dec 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment