Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#28680 closed Cleanup/optimization (fixed)

Document that Func.__init__()'s **extra and as_sql()'s **extra_context aren't escaped

Reported by: Hynek Cernoch Owned by: nobody
Component: Documentation Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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)

Change History (5)

comment:1 by Tim Graham, 7 years ago

Marc Tamlyn says, "An admonition in 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."

comment:2 by Tim Graham, 7 years ago

Has patch: set
Summary: Document that Func's.__init__()'s **extra and as_sql()'s **extra_context aren't escapedDocument that Func.__init__()'s **extra and as_sql()'s **extra_context aren't escaped

comment:3 by Hynek Cernoch, 7 years ago

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

comment:4 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: newclosed

In 1e7dbbde:

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.

comment:5 by Tim Graham <timograham@…>, 7 years ago

In 7d3c02a:

[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

Note: See TracTickets for help on using tickets.
Back to Top