Re: Feature request: Settings to disable comments and multiple statements in a connection - Mailing list pgsql-general

Glen K <glenk1973@hotmail.com> writes:
>> I don't believe that this would move the needle on SQL-injection
>> safety by enough to be worth doing.  An injection attack is normally
>> trying to break out of a quoted string, not a comment.

> If 90% of injection attacks make use of comments (together with quoted string escapes), it seems to me that a
connectionconfiguration option to disable comments would "move the needle" substantially. 

There are a few reasons why this proposal is less likely to improve
matters and more likely to cause trouble than you realize:

* As you yourself noted, an attacker can certainly craft the attack
to cause the remainder of the original statement to be taken as
some fairly harmless separate SQL statement.  So disabling comments
is unhelpful in isolation.  I concede that it could help if you
*also* reject multiple-statements-per-message, but that reduces
the number of clients that can use the feature.

* What will actually happen if we implement such a feature is that
client-side stacks will have to put in code to remove comments from
submitted SQL, so that their applications aren't broken against
servers that enforce this option.  (This is something we've learned
the hard way: if we materially change the server's behavior, that
doesn't only affect "installations that choose to enable the option".
General-purpose client code has to be prepared to cope with both
behaviors.  So the people who really pay the cost of such things are
client-side driver authors.)  Now, that is work they don't really
want to do, and it is work they could well get wrong.  But even if
they do not mess up, an attack that passes through such a frontend
would succeed just fine, because the server would never know that
there'd been a comment there.  As an illustration that that's not
hypothetical, psql used to remove embedded comments up till not
too long ago, so an attack feeding through an older psql would
work fine even if the server rejected comments.

* There's a standards-compliance argument: comments are not
optional according to the SQL standard.

* As a practical matter, this'd require raw parsing to change behavior
according to a server GUC parameter, which is something that is pretty
horrid.  There's a comment in our gram.y explaining why:

 *      In general, nothing in this file should initiate database accesses
 *      nor depend on changeable state (such as SET variables).  If you do
 *      database accesses, your code will fail when we have aborted the
 *      current transaction and are just parsing commands to find the next
 *      ROLLBACK or COMMIT.  If you make use of SET variables, then you
 *      will do the wrong thing in multi-query strings like this:
 *            SET constraint_exclusion TO off; SELECT * FROM foo;
 *      because the entire string is parsed by gram.y before the SET gets
 *      executed.  Anything that depends on the database or changeable state
 *      should be handled during parse analysis so that it happens at the
 *      right time not the wrong time.

We do have a couple of existing GUCs like that, notably
standard_conforming_strings, and they are gotchas and/or security
hazards already.  We'd do better to remove them, not add more.


So in short, while it would not be terribly hard to put in what
you suggest, we'd be creating a lot of work for people other than
ourselves.  And the end result when all the dust had settled would
likely be just marginal security gains for a small subset of users.

            regards, tom lane