[Django] #28770: Warn that quoting a placeholder in a raw SQL string can lead to SQL injection
Django
injection
------------------------------------------------+------------------------
Reporter: Hynek Cernoch | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28770>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
injection
Reporter: Hynek Cernoch | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/9327 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28770#comment:1>
Django
injection
Reporter: Hynek Cernoch | Owner: nobody
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"327f0f37ce3c1e5ac3a19668add237ddd92266d6" 327f0f3]:
{{{
#!CommitTicketReference repository=""
revision="327f0f37ce3c1e5ac3a19668add237ddd92266d6"
Fixed #28770 -- Warned that quoting a placeholder in a raw SQL string is
unsafe.
Thanks Hynek Cernoch for the report and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28770#comment:2>
Django
injection
Reporter: Hynek Cernoch | Owner: nobody
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"c869207ea29822c81d5a242a1e401135d813a96c" c869207e]:
{{{
#!CommitTicketReference repository=""
revision="c869207ea29822c81d5a242a1e401135d813a96c"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28770#comment:3>