propelorm/Propel

SQL injection possible with limit() on MySQL

mpetrovich opened this issue · 6 comments

mpetrovich commented

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

For example, given a 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 Criteria::setLimit() or in DBMySQL::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:

halfer commented

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

mpetrovich commented

Pull request with the fix for Propel 1.x: #1054

  • 👍2
mentalstring commented

@marcj @willdurand Any chance this security fix can get merged into master? I understand this version is no longer maintained, and I don't even hope for a new release, but there's still quite a few people using this out there and this bug is pretty nasty.

  • 👍4
mpetrovich commented

Wouldn't we still need a new release for most clients to reliably get it? Unless they install it in a non-standard way to get the latest commit, they won't get the fix unless we tag a new patch release.

  • 👍2
nosnickid commented

In case anyone is interested, I've finally got around to forking and making available a version of the propel-bundle with the fix from @mpetrovich (thanks!) and a couple of other minor tweaks relating to concrete_inheritance I was manually patching for many years (the shame)...

Amend your composer.json with:

    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/TonicWorks/Propel.git"
        },
        {
            "type": "vcs",
            "url": "https://github.com/TonicWorks/PropelBundle.git"
        }
    ]

And replace your propel-bundle requirement with:

"tonic-works/propel-bundle": "^1.5.0",

No guarantees of course, but this will at least allow you to keep the roave/security-advisories in your projects.