Warn that quoting a placeholder in a raw SQL string can lead to SQL injection

[Hynek Cernoch]

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 ​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 ​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.

[Tim Graham]

​PR

[GitHub <noreply@…>]

In 327f0f3:

Fixed #28770 -- Warned that quoting a placeholder in a raw SQL string is unsafe.

Thanks Hynek Cernoch for the report and review.

[Tim Graham <timograham@…>]

In c869207e:

[2.0.x] Fixed #28770 -- Warned that quoting a placeholder in a raw SQL string is unsafe.

Thanks Hynek Cernoch for the report and review.

Backport of 327f0f37ce3c1e5ac3a19668add237ddd92266d6 from master