[Django] #28680: Document that Func's.__init__()'s **extra and as_sql()'s **extra_context aren't escaped
Django
aren't escaped
------------------------------------------------+------------------------
Reporter: Hynek Cernoch | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.11
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 |
------------------------------------------------+------------------------
I found an SQL injection possibility due to unclear documentation about
query expression:
`class Func(*expressions, **extra)`
if unsafe user input is passed by keyword parameters `extra` it is
unprotected, while positional parameters are protected by compile and
passing through sql execute parameters, never merged to SQL by % format.
{{{
class Position(Func):
function = 'POSITION'
template = "%(function)s('%(substring)s' in %(expressions)s)"
def __init__(self, expression, substring):
super(Position, self).__init__(expression, substring=substring)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28680>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
aren't escaped
Reporter: Hynek Cernoch | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Marc Tamlyn says, "An admonition in
[https://docs.djangoproject.com/en/dev/ref/models/expressions/#func-
expressions the docs for Func()] should be sufficient I think. At present
those docs are mostly about using the "simple" template syntax rather than
handling SQL and params "the long way" and returning `(sql, params)` from
the `as_sql()` method. I think this might be a dangerous approach to the
docs and we should be more explicit earlier about separating params from
chunks of sql. There's a warning in the docs for `RawSQL` but it's not
really discussed enough in the expressions reference IMO."
--
Ticket URL: <https://code.djangoproject.com/ticket/28680#comment:1>
Django
aren't escaped
--------------------------------------+------------------------------------
Reporter: Hynek Cernoch | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.11
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/9202 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28680#comment:2>
Django
aren't escaped
--------------------------------------+------------------------------------
Reporter: Hynek Cernoch | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.11
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 Hynek Cernoch):
It is sufficient to be committed anything at first glance it was very
nice, but probably not to close the issue.
Another place where it should be added is `docs/topics/security.txt` to be
easier memorable.
I think that the word "escaping" is vague and it should be used
[https://www.owasp.org/index.php/SQL_Injection_Prevention_Cheat_Sheet#Defense_Option_4
:_Escaping_All_User-Supplied_Input "as the last resort" (OWASP)] and an
ORM is considered more secure than escaping. Escaping means usually to
protect only some special characters. A simple string `"0) OR (TRUE"` can
be also dangerous if parsed into `WHERE some_function(field_a,
%(number)s)`. Extra and extra_context can seem as if safe for simple
values like "ASC"/"DESC" or pseudo numeric values that are not actually
converted to number by mistake, but they are not. It is still more risky
than RawSQL due to frequent underestimation and misunderstanding what is a
trusted user input. The string can not be passed insecure in RawSQL by
"params" because all modern database backends substitute it to SQL by
after parsing SQL syntax. It is better than to escape by apostrophes at
the beginning and at the end and to protect apostrophes etc. inside.
The user could still expect an analogy that `template` and `extra` are
similar to `RawSQL` and `params` and that the warning is also very
similar, that is:
"RawSQL()... You should be very careful to escape any parameters that the
user can control by using `params` in order to protect against *SQL
injection*..."
It should be emphasized that this works differently.
"as these values aren't escaped when they're inserted into the SQL string"
+ "(unlike RawSQL params)"
What can be a future improvement: (maybe I will write something)
* `arg_joiner` can help to not need `extra` params. Maybe a short example
of `arg_joiner` will be useful, otherwise the user thinks that he
understands `extra` params better.
* There is nothing that helps to protect correctly with "extra". The
escaping depends on the backend, different for MySQL and for other db.
--
Ticket URL: <https://code.djangoproject.com/ticket/28680#comment:3>
Django
aren't escaped
--------------------------------------+------------------------------------
Reporter: Hynek Cernoch | Owner: nobody
Component: Documentation | Version: 1.11
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 Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"1e7dbbdec5ca2bd236b8ab64fa172a8fc0abcb1e" 1e7dbbde]:
{{{
#!CommitTicketReference repository=""
revision="1e7dbbdec5ca2bd236b8ab64fa172a8fc0abcb1e"
Fixed #28680 -- Doc'd Func.__init__()'s **extra and as_sql()'s
**extra_context aren't escaped.
Thanks Hynek Cernoch for the report and review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28680#comment:4>
Django
aren't escaped
--------------------------------------+------------------------------------
Reporter: Hynek Cernoch | Owner: nobody
Component: Documentation | Version: 1.11
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:"7d3c02a214ae7f59d56c8620b3bb31728d04cd99" 7d3c02a]:
{{{
#!CommitTicketReference repository=""
revision="7d3c02a214ae7f59d56c8620b3bb31728d04cd99"
[2.0.x] Fixed #28680 -- Doc'd Func.__init__()'s **extra and as_sql()'s
**extra_context aren't escaped.
Thanks Hynek Cernoch for the report and review.
Backport of 1e7dbbdec5ca2bd236b8ab64fa172a8fc0abcb1e from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28680#comment:5>