SQL injection in quote_name()

[Ivan]

183 	    def quote_name(self, name):
184 	        if name.startswith("`") and name.endswith("`"):
185 	            return name # Quoting once is enough.
186 	        return "`%s`" % name

​http://code.djangoproject.com/browser/django/trunk/django/db/backends/mysql/base.py#L183

name = '`column_name!`; DROP database `dbname!`' # take from request for sort table. Insert value to extra() or order_by()

sql='SELECT * FROM... ORDER BY `column_name!`; DROP database `dbname!`'

[Russell Keith-Magee]

First off, for future reference: if you want to report a security issue, PLEASE use our ​security issue reporting procedure. Reporting potential security issues into the wild is very poor form.

Secondly, as far as I can make out from the information you've provided, this isn't a plausible injection attack.

Yes, quote_name is weak, and easily exploited. Which would be a problem if it was used anywhere to sanitize user-provided input. Which it isn't. At least, not anywhere that I can find, providing you follow the advice of the documentation.

order_by() only accepts existing columns (annotated with a very small number of allowed extra bits like '-' and '?') for sorting. You can't insert arbitrary content, even if you *were* using user-provided data to drive that clause -- which it itself a pretty unlikely set of circumstances. The error is hidden under some circumstances by #14766, but if you inspect the SQL log, or you attempt to print the underlying query, you'll see the error that is generated:

>>> print MyModel.objects.order_by('`column_name!`; DROP database `dbname!`').query
FieldError: "Invalid order_by arguments: ['`column_name!`; DROP database `dbname!`']"

extra() is a slightly different beast, but as long as you use it right (that is to say, as documented), you're safe. If, for example, you allow user-provided input to be used in the "where=" argument, you can construct an injection attack:

MyModel.objects.extra(where=['"column_name!"="foo"; DROP database "dbname!"'])

...but if you use params, the user-provided data is correctly escaped by the database cursor. Our docs tell you to do this, too. They could say it a little more emphatically, perhaps, but it is there in black and white, with an example. Given that extra() is one step away from raw SQL, there really isn't anything else we can do here. Raw SQL is, by definition, exposing the bare metal, so we rely on people using it the right way. If you hold a sword by the blade, you're going to cut your hand, no matter how many warnings and safety catches are on the scabbard.

In summary: I (and several other members of the core team) can't find an in-principle attack here, or in any code related to what you describe. The examples you have provided are either incomplete or incorrect. Closing this ticket as invalid.

If you think we have missed something, and you can present an actual in-principle or in-practice attack (including a complete example, not just vague handwaving at the order_by clause), we're happy to reopen this. But, repeating the first point -- if you even *think* you have found a security issue, *PLEASE* report it to security@…, not on Trac.

[Ivan]

Ok, Thanks.
But I think, the better way is:

def quote_name(self, name):
    if name.startswith("`") and name.endswith("`"):
        name = name.strip('`')
    return "`%s`" % name.replace('`', '``')

This code does not depend on other checks.