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

[Hynek Cernoch]

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)

[Tim Graham]

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

[Tim Graham]

​PR

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

[Tim Graham <timograham@…>]

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.

[Tim Graham <timograham@…>]

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