id	summary	reporter	owner	description	type	status	component	version	severity	resolution	keywords	cc	stage	has_patch	needs_docs	needs_tests	needs_better_patch	easy	ui_ux
28770	Warn that quoting a placeholder in a raw SQL string can lead to SQL injection	Hynek Cernoch	nobody	"This is a follow up to #28680 which addresses the the security of `Func()` query expression if used with unsafe `extra` or `extra_context`.

I have found a similar issue in:
{{{
models.query.QuerySet.extra(... params=...)
models.expressions.RawSQL(... params=...)
    models.Manager.raw(... params=...)
    cursor.execute(... params=...)
}}}
if the SQL is constructed carelessly with apostrophes around the parameter placeholder "" '%s' "" and the parameter was obtained from a user input by:
a) by JSON without a type control, or
b) by guessing the type
c) with an obsoleted unused parameter in a more complicated query that has not been ever tested.

Example:
{{{
from django.contrib.auth.models import User

class Data(models.Model):
    user = models.ForeignKey(User, models.DO_NOTHING)
    option = models.CharField(max_length=20)
    val = models.IntegerField()
    secret = models.CharField(max_length=100)
}}}
--- view common part ---
{{{
    # common part - get Data for the current authenticated user
    qs = Data.objects.filter(user=request.user)

    # attack to get all Data
}}}
A) vulnerable code - added filter by JSON data without a type control
{{{
    user_data = """"""{""val"": 1}""""""                      # expected example
user_data = """"""{""val"": "") OR TRUE OR (val=""}""""""'  # attack example
val= json.loads(json_data)['val']
    qs = qs.extra(where=[""val='%s'""], params=[val]])
}}}
B) vulnerable code - guess a type  (very similar)
{{{
user_data = ""1"" # expected example
user_data = "") OR TRUE OR (val=""# attack example
    val = int(user_data) if user_data.isdigit() else user_data
qs = qs.extra(where=[""val='%s'""], params=[val])
}}}
C) vulnerable due to an unused obsoleted request variable 'user_data'
{{{
    user_data = ''                           # usual value of an unused field
    user_data = ')); DROP TABLE ...; --'     # attack example
qs = qs.extra(where=[""val > 0 OR val < 0 AND option='%s'""], params=[user_data])
    # if negative values `val` are not expected seriously then the query could be untested enough
}}}
Backends:
- mysql - all A, B, C) vulnerable
- postgresql - vulnerable C)
- sqlite3 - seems safe

A complete code with a test is in attachment.

If I compare these vulnerabilities with #28680 Func(), it is less probable that the invalid SQL will work and some circumstances like A), B) or C) are necessary. On the other hand these functions are probably much more used (by my experience at StackOverflow)

Generally, escaping of parameters by a database driver is a safe technique if the SQL is carefully constructed. That is still OK for ORM generated queries, but generally not for manually constructed parts of SQL like in the functions above.

A separation of SQL and data by parameterized queries is also a safe technique if it is done consistently from the beginning up to a parameterized call to database server. But it is not a case of most Django db drivers.

I expect that also this issue will be solved by documentation first. A security scanner for SQL can be used later. It can identify suspicious SQL at run-time in debug mode and write warnings.

The [https://docs.djangoproject.com/en/dev/ref/models/querysets/#django.db.models.query.QuerySet.extra current docs for extra()]:

> Warning:
> You should be very careful whenever you use extra(). Every time you use it, you should escape any parameters that the user can control by using params in order to protect against SQL injection attacks . Please read more about SQL injection protection.

Similar texts are for `RawSQL()` and `raw()`.

something similar to [http://initd.org/psycopg/docs/usage.html#the-problem-with-the-query-parameters psycopg's docs] is useful:

     ...   ('%s');"" # NEVER DO THIS
     ...   (%s);"" # Note: no quotes

This can be reported as an issue to driver development. A check there is more effective."	Cleanup/optimization	closed	Documentation	dev	Normal	fixed			Accepted	1	0	0	0	0	0