Thread: Feature request: Settings to disable comments and multiple statements in a connection

• Provide a client connection option (and/or implement the backend support) to disallow comments in SQL statements
• Provide a client connection option (and/or implement the backend support) to allow only one statement in an execute request
• Provide an option in the client execute functions (and/or implement the backend support) to specify the expected number of statements. This would override the client connection option and would inhibit attackers from injecting additional statements

Glen K <glenk1973@hotmail.com> writes:
> My feature requests are thus:

> Provide a client connection option (and/or implement the backend support) to disallow comments in SQL statements

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.

> Provide a client connection option (and/or implement the backend support) to allow only one statement in an execute
request

This exists already; you just have to use the extended query protocol.

> Provide an option in the client execute functions (and/or implement
> the backend support) to specify the expected number of statements.

I don't see the need for this given #2.

            regards, tom lane

On Thu, 5 Jun 2025 at 01:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> ... An injection attack is normally
> trying to break out of a quoted string, not a comment.

I think the comments he refers to are more used to do "bobby tables"
like stuff, as helpers in correct statement forming, not to inject per
se.

( I do not think the feature request is worth doing either, just commenting ).

Francisco Olarte.

Am Wed, Jun 04, 2025 at 10:41:15PM +0000 schrieb Glen K:

> In my experience developing backends for APIs, I have
> never come across a case where comments were needed or
> desired within SQL statements generated for API requests,

Being able to garnish with comments the SQL being sent to a
backend greatly aids in debugging queries (such as which
query originated from which part of remote code [or even
when]).

Not that it didn't take me quite a few years
to chance upon that idea ...

Best regards,
Karsten
--
GPG  40BE 5B0E C98E 1713 AFA6  5BC0 3BEA AC80 7D4F C89B

On 6/7/25 14:18, Glen K wrote:
>>  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.
> 
> Yes, SQL injections frequently involve escaping quoted strings, but if 
> you do a search for SQL injection examples, you will find that most of 
> them (I would say 90% or more) also use comments to remove the remainder 
> of the SQL statement from consideration. Here is one example where an 
> attacker specifies "admin'--;" as the username:
> 
> SELECT * FROM members WHERE username = 'admin'--;' AND password = 
> 'password';
> 
> The comment in this example removes the password from inclusion in the 
> statement, allowing the attacker to login as admin without a password.

Really?

select username, first_name, last_name from auth_user where username = 
'aklaver';

  username | first_name | last_name
----------+------------+-----------
  aklaver  | Adrian     | Klaver

  select username, first_name, last_name from auth_user where username = 
'aklaver--;' and password = 'password';

  username | first_name | last_name
----------+------------+-----------
(0 rows)

What authentication system are you using that does not actually verify 
the password and allows entry for a zero return result?


-- 
Adrian Klaver
adrian.klaver@aklaver.com

On 6/7/25 14:56, Adrian Klaver wrote:
> On 6/7/25 14:18, Glen K wrote:
>>>  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.
>>
>> Yes, SQL injections frequently involve escaping quoted strings, but if 
>> you do a search for SQL injection examples, you will find that most of 
>> them (I would say 90% or more) also use comments to remove the 
>> remainder of the SQL statement from consideration. Here is one example 
>> where an attacker specifies "admin'--;" as the username:
>>
>> SELECT * FROM members WHERE username = 'admin'--;' AND password = 
>> 'password';
>>
>> The comment in this example removes the password from inclusion in the 
>> statement, allowing the attacker to login as admin without a password.
> 
> Really?
> 
> select username, first_name, last_name from auth_user where username = 
> 'aklaver';
> 
>   username | first_name | last_name
> ----------+------------+-----------
>   aklaver  | Adrian     | Klaver
> 
>   select username, first_name, last_name from auth_user where username = 
> 'aklaver--;' and password = 'password';
> 
>   username | first_name | last_name
> ----------+------------+-----------
> (0 rows)

Oops, missed a quote:

select username, first_name, last_name from auth_user where username = 
'aklaver'--;' and password = 'password';
production-#

Still I don't see how this would work, even if you add another ';' and got:

production=# select username, first_name, last_name from auth_user where 
username = 'aklaver'--;' and password = 'password';
production-# ;
  username | first_name | last_name
----------+------------+-----------
  aklaver  | Adrian     | Klaver



> 
> What authentication system are you using that does not actually verify 
> the password and allows entry for a zero return result?
> 
> 

-- 
Adrian Klaver
adrian.klaver@aklaver.com

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

On Thu, Jun 5, 2025 at 1:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Provide a client connection option (and/or implement the backend support) to allow only one statement in an execute
request
>
> This exists already; you just have to use the extended query protocol.

Hi Tom. Can you be more specific please?
In the context of LibPQ, in case it matters.
TIA, --DD

On Tue, 2025-06-10 at 10:02 +0200, Dominique Devienne wrote:
> On Thu, Jun 5, 2025 at 1:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Provide a client connection option (and/or implement the backend support) to allow only one statement in an
executerequest 
> >
> > This exists already; you just have to use the extended query protocol.
>
> Hi Tom. Can you be more specific please?
> In the context of LibPQ, in case it matters.

PQexecParams()

Yours,
Laurenz Albe