CODEWITHSUNDEEP
postgresqlsql-injection
404
404

Reputation: 8572

Is this actually open to SQL injection?

We have a function that builds some XML and uses EXECUTE/USING to try to prevent SQL injection. It's along the lines of:

CREATE OR REPLACE FUNCTION public.f1(t TEXT)
    RETURNS XML AS
$BODY$
DECLARE
    ret_val XML;
BEGIN
    EXECUTE 'SELECT ''<'' || $1 || ''>'''
    --rest of xml building
    INTO ret_val
    USING t;
    RETURN ret_val;
END
$BODY$
    LANGUAGE plpgsql IMMUTABLE;

I don't like this much due to the concatenation. It would be much better if we could just do SELECT '''<$1>'' but that ends up with a literal $1 rather than replacing it with the value.

Due to the concatenation, it's making me wonder whether we even need SQL injection prevention here. It's not reading from a table, just building an XML string which is returned.

Is there any actual risk from not using USING in this case? Does concatenating $1 negate the effects of USING, or does USING even have any effect on a statement that doesn't use a table?

Upvotes: 2

Views: 78

Answers (2)

IMSoP
IMSoP

Reputation: 97718

There are a few things to unpack here.

Firstly, the SQL you have there is actually a fixed string:

'SELECT ''<'' || $1 || ''>'''

So nothing can be directly injected here, because there's no dynamic SQL. As Laurenz Albe pointed out, the SQL in this particular example could be written as a non-dynamic statement:

SELECT '<' || t || '>'

There is still no SQL injection here, because you're not evaluating the contents of t, just manipulating it as a string, just as SELECT a + 1 would manipulate a as a number.

The key point is that the actual SQL is hard-coded, and the concatenation is just the instruction in that SQL.

Note that this similar-looking query would be dangerous (the syntax highlighting gives a clue to the difference):

EXECUTE 'SELECT ''<' || t || '>''' -- DON'T DO THIS!

Here, the value of t is being used as part of the SQL string - the concatenation is happening first, and then the result is executed. So a value of '1'; DROP TABLE users; --' would result in the query SELECT '<1'; DROP TABLE users; -- which is clearly undesirable.

Secondly, as explained in the docs, the $1 is a parameter, supplied by the USING clause as data, so it too is safe from SQL injection. This is similar to using a parameterised query in a programming language outside the database - you build up the query as a string, carefully whitelisting the tables and columns referenced, then provide the variable data separately, where it cannot be reinterpreted as SQL. Or to put it another way, it's like another function "one level deeper", with the parameters specified by the USING clause acting just like the parameters to an actual function.

Finally, though, a note of caution: you are vulnerable to XML injection: if nothing else has validated or escaped t, you could generate invalid or dangerous XML. For instance, consider what would happen if the value of t was 'foo><script>alert("Hello, world!");</script></foo' and the result ended up parsed as HTML.

Upvotes: 1

Laurenz Albe
Laurenz Albe

Reputation: 246653

There is no danger for SQL injection here, because you are using USING.

Note that you could have been using static SQL to achieve the same thing:

SELECT '<' || t || '>' INTO ret_val;

Upvotes: 1

Related Questions