Re: info about patch: using parametrised query in psql

Hello

I try to explain my motivation for creating this patch
https://commitfest.postgresql.org/action/patch_view?id=224 .

Parametrised queries are supported in PostgreSQL long time. Using the
parametrised queries is point of all advices about good programming
style. On application level it is protection to SQL injection. On low
level it is protection to some potential quoting or escaping bugs. It
is paradox, so PostgreSQL doesn't use this techniques in own
applications - mainly in psql.

In psql we have not any quoting, or escaping functionality. We have to
use external tools like awk, sed:

http://www.redhat.com/docs/manuals/database/RHDB-2.1-Manual/admin_user/r1-app-psql-4.html

>
> testdb=> \set content '\'' `cat my_file.txt` '\''
> testdb=> INSERT INTO my_table VALUES (:content);
>
> One possible problem with this approach is that my_file.txt might contain single quotes.
> These need to be escaped so that they do not cause a syntax error when the
> third line is processed. You can do this with the program sed:
>
> testdb=> \set content `sed -e "s/'/\\\\\\'/g" < my_file.txt`

Similar problems could be removed with using parameter queries in psql.

With this parametrised queries feature you can:

\set content `cat my_file.txt`
INSERT INTO my_table VALUES(:content);

and this command will be correct without depending on content my_file.txt file.

This is more: robust, secure, and simpler.

My motivation is simplify life to people who use psql for scripting.
For internal use SQL injection isn't too much terrible. Problem are
some obscure identifiers used some users. Now you have to carefully
check every value, if your scripts have to be robust.

Patch doesn't change default behave. You have to explicitly activate it.

Regards,
merry Christmas

Pavel Stehule

On Thu, Dec 24, 2009 at 2:45 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello
>
> I try to explain my motivation for creating this patch
> https://commitfest.postgresql.org/action/patch_view?id=224 .
>
> Parametrised queries are supported in PostgreSQL long time. Using the
> parametrised queries is point of all advices about good programming
> style. On application level it is protection to SQL injection. On low
> level it is protection to some potential quoting or escaping bugs. It
> is paradox, so PostgreSQL doesn't use this techniques in own
> applications - mainly in psql.
>
> In psql we have not any quoting, or escaping functionality. We have to
> use external tools like awk, sed:
>
> http://www.redhat.com/docs/manuals/database/RHDB-2.1-Manual/admin_user/r1-app-psql-4.html
>
>>
>> testdb=> \set content '\'' `cat my_file.txt` '\''
>> testdb=> INSERT INTO my_table VALUES (:content);
>>
>> One possible problem with this approach is that my_file.txt might contain single quotes.
>> These need to be escaped so that they do not cause a syntax error when the
>> third line is processed. You can do this with the program sed:
>>
>> testdb=> \set content `sed -e "s/'/\\\\\\'/g" < my_file.txt`
>
> Similar problems could be removed with using parameter queries in psql.
>
> With this parametrised queries feature you can:
>
> \set content `cat my_file.txt`
> INSERT INTO my_table VALUES(:content);
>
> and this command will be correct without depending on content my_file.txt file.
>
> This is more: robust, secure, and simpler.
>
> My motivation is simplify life to people who use psql for scripting.
> For internal use SQL injection isn't too much terrible. Problem are
> some obscure identifiers used some users. Now you have to carefully
> check every value, if your scripts have to be robust.
>
> Patch doesn't change default behave. You have to explicitly activate it.

This makes sense now that you've explained it. Personally, I would
not choose to use psql as a scripting language, and I think there has
been some controversy on that point in the past, though I don't
remember the details. In spite of that, though, it seems to me that
it does make some sense to provide a mechanism for escaping the value
stored in a psql variable, since - if nothing else - someone might
easily want to do the sort of thing you're describing here in an
interactive session.

However, I think the approach you've taken in this patch is a
non-starter. You've basically added a global flag that will cause ALL
variables to be passed in a way that removes the need for them to be
escaped. That seems pretty inconvenient and awkward. What happens if
someone wants to do "INSERT INTO :foo VALUES (:bar)"? They're out of
luck. Futhermore, if a psql script that expects the pexec flag to be
set one way is run with it set the other way, it may either work fine,
work OK but with a potential security hole, or fail spectacularly. I
think maybe what we need here is a piece of syntax to indicate that a
specific parameter should be substituted after first being passed
through PQescapeStringConn.

Other thoughts?

...Robert

>
> This makes sense now that you've explained it.  Personally, I would
> not choose to use psql as a scripting language, and I think there has

The scripting are not realised directly in psql - psql missing some
basic features still. Usually is used in combination with bash (any
shell) - like starter stored procedures or source of data.

for x in `echo "sql" | psql params db
do
...
done

this combination is relative very strong.

> been some controversy on that point in the past, though I don't
> remember the details.  In spite of that, though, it seems to me that
> it does make some sense to provide a mechanism for escaping the value
> stored in a psql variable, since - if nothing else - someone might
> easily want to do the sort of thing you're describing here in an
> interactive session.
>
> However, I think the approach you've taken in this patch is a
> non-starter.  You've basically added a global flag that will cause ALL
> variables to be passed in a way that removes the need for them to be
> escaped.  That seems pretty inconvenient and awkward.  What happens if
> someone wants to do "INSERT INTO :foo VALUES (:bar)"?  They're out of

Using a global flags is typical for psql. There are nothing else. I am
thinking about stacked states for epsql, but it isn't some for psql.
psql uses global flags, it uses global variables. I aware of
disadvantages - but I thing so it is in agreement with psql design "do
things simple".

If somebody use variable on wrong place, then result will be a syntax
error. But better fail then be not secure. For full functionality it
needs some explicit syntax for quote_ident - so correct and secure
statement will be:

INSERT INTO :[foo] VALUES (:bar)

There are two ways (three) - both are possible and well, and probably
it is +/- personal preferences who prefer one or second:

a) using parametrised queries - it simple way - bulletproof with limit
- cannot use variable as identifier
b) using some quoting mechanism - it little bit more complex -
PostgreSQL uses two different quoting styles, for somebody isn't
bulletproof, but it could be used everywhere. There are big advantage
- no new global flag - so using should be simpler for beginners.

c) combination

a) INSERT INTO :foo VALUES(:bar) -- isn't possible
b) INSERT INTO :[foo] VALUES(:{bar}) -- I used syntax from epsql fpr
this moment - could be different
c) INSERT INTO :[foo] VALUES(:bar)

I didn't need to (b) or (c), personally I prefer (a), maybe (b). It is
only my personal preference - and I have a good knowledge of
parametrised queries. Typical user can thing different. I am not
strong in it. I'll be satisfied if any form will be supported. I
tested all variants.

> luck.  Futhermore, if a psql script that expects the pexec flag to be
> set one way is run with it set the other way, it may either work fine,
> work OK but with a potential security hole, or fail spectacularly.  I
> think maybe what we need here is a piece of syntax to indicate that a
> specific parameter should be substituted after first being passed
> through PQescapeStringConn.

PQescapeStringConn is good, but it isn't helper for INSERT INTO :foo.
It is analogy for quote_literal function, not for quote_ident. So we
need enhance PQ function sets. Escaping is little bit slower, but it
isn't important in this case. I agree, potential escaping needs
explicit syntax.

?
Regards
Pavel

>
> Other thoughts?
>
> ...Robert
>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> I think maybe what we need here is a piece of syntax to indicate that a
> specific parameter should be substituted after first being passed
> through PQescapeStringConn.

I agree that a global flag that changes the behavior of :foo is a
seriously bad idea. Alternate syntax would be much better, but how
exactly can we shoehorn that in? Maybe something like
:!foo
ie put some non-letter flags between the : and the variable name.
It would obviously not work to use ::foo, but I think many other
punctuation characters would be safe (would not conflict with any
likely SQL usage). We could have a couple of different flags to
signal whether you want single or double quoting of the variable
value.

regards, tom lane

2009/12/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> I think maybe what we need here is a piece of syntax to indicate that a
>> specific parameter should be substituted after first being passed
>> through PQescapeStringConn.
>
> I agree that a global flag that changes the behavior of :foo is a
> seriously bad idea.  Alternate syntax would be much better, but how
> exactly can we shoehorn that in?  Maybe something like
>        :!foo

there are two quoting styles, so we need two syntax. I proposed

:[var] and :{var} - for ident quoting and literal quoting.
Theoretically we could to use :(var) for bytea escaping. :!foo isn't
good idea. It is related to negation operator. Bracket or parenthesis
are good readable and far to some custom pg operators.

Regards
Pavel Stehule

> ie put some non-letter flags between the : and the variable name.
> It would obviously not work to use ::foo, but I think many other
> punctuation characters would be safe (would not conflict with any
> likely SQL usage).  We could have a couple of different flags to
> signal whether you want single or double quoting of the variable
> value.
>
>                        regards, tom lane
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> there are two quoting styles, so we need two syntax. I proposed

> :[var] and :{var} - for ident quoting and literal quoting.
> Theoretically we could to use :(var) for bytea escaping.

And if you need a fourth style, you're at a dead end. I don't think
this is really an improvement over the single-flag-character approach.
Neither one has got any mnemonic value whatever, unfortunately, but
at least the flag character method is fairly extensible.

regards, tom lane

2009/12/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> there are two quoting styles, so we need two syntax. I proposed
>
>> :[var] and :{var} - for ident quoting and literal quoting.
>> Theoretically we could to use :(var) for bytea escaping.
>
> And if you need a fourth style, you're at a dead end.  I don't think
> this is really an improvement over the single-flag-character approach.
> Neither one has got any mnemonic value whatever, unfortunately, but
> at least the flag character method is fairly extensible.

I thing so not.

what:

:'variable'
:"variable"

we could to use any non identifier char without ":"

for me - flag characters looks little bit strange - maybe I have a
quoting joined with some symmetric. Maybe it looks too much like unary
operator

Regards
Pavel

>
>                        regards, tom lane
>

On Fri, Dec 25, 2009 at 1:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> there are two quoting styles, so we need two syntax. I proposed
>
>> :[var] and :{var} - for ident quoting and literal quoting.
>> Theoretically we could to use :(var) for bytea escaping.
>
> And if you need a fourth style, you're at a dead end.  I don't think
> this is really an improvement over the single-flag-character approach.
> Neither one has got any mnemonic value whatever, unfortunately, but
> at least the flag character method is fairly extensible.

The lack of mnemonic value kind of sucks, but I don't see that Pavel's
style is any more or less extensible than your proposed flag
character. Basically, he's saying that the flag characters will be [
and { and adding a closing delimeter to match. If we do want to go
with a single flag character, maybe it should just be a single or
double quote:

:'foo - quote as a literal
:"foo - quote as an ident

I dunno what to do about bytea-escaping under this framework, though.

...Robert

2009/12/25 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Dec 25, 2009 at 1:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>>> there are two quoting styles, so we need two syntax. I proposed
>>
>>> :[var] and :{var} - for ident quoting and literal quoting.
>>> Theoretically we could to use :(var) for bytea escaping.
>>
>> And if you need a fourth style, you're at a dead end.  I don't think
>> this is really an improvement over the single-flag-character approach.
>> Neither one has got any mnemonic value whatever, unfortunately, but
>> at least the flag character method is fairly extensible.
>
> The lack of mnemonic value kind of sucks, but I don't see that Pavel's
> style is any more or less extensible than your proposed flag
> character.  Basically, he's saying that the flag characters will be [
> and { and adding a closing delimeter to match.  If we do want to go
> with a single flag character, maybe it should just be a single or
> double quote:
>
> :'foo - quote as a literal
> :"foo - quote as an ident

I could to live with

:'foo' and :"foo" although ' and " characters are not the best for
readability. But the mnemonic is clear.

Pavel

>
> I dunno what to do about bytea-escaping under this framework, though.
>
> ...Robert
>

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> If we do want to go
> with a single flag character, maybe it should just be a single or
> double quote:

> :'foo - quote as a literal
> :"foo - quote as an ident

I would've proposed that myself if I thought it would work, but I'm
afraid that it will wreak complete chaos from a parsing standpoint.
Half the tools in the world will think this is an incomplete literal,
and I'm not even very sure you could keep psql itself from getting
confused.

Hmm ... actually, though, what about combining the ideas:

:'foo' - quote as a literal
:"foo" - quote as an ident

This leaves us with nothing much as far as extensibility, but from
a mnemonic standpoint it's a large win.

regards, tom lane

2009/12/25 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> If we do want to go
>> with a single flag character, maybe it should just be a single or
>> double quote:
>
>> :'foo - quote as a literal
>> :"foo - quote as an ident
>
> I would've proposed that myself if I thought it would work, but I'm
> afraid that it will wreak complete chaos from a parsing standpoint.
> Half the tools in the world will think this is an incomplete literal,
> and I'm not even very sure you could keep psql itself from getting
> confused.
>

I though about it too.

> Hmm ... actually, though, what about combining the ideas:
>
>        :'foo' - quote as a literal
>        :"foo" - quote as an ident
>
> This leaves us with nothing much as far as extensibility, but from
> a mnemonic standpoint it's a large win.

+1

Pavel

>
>                        regards, tom lane
>

On Fri, Dec 25, 2009 at 2:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> If we do want to go
>> with a single flag character, maybe it should just be a single or
>> double quote:
>
>> :'foo - quote as a literal
>> :"foo - quote as an ident
>
> I would've proposed that myself if I thought it would work, but I'm
> afraid that it will wreak complete chaos from a parsing standpoint.
> Half the tools in the world will think this is an incomplete literal,
> and I'm not even very sure you could keep psql itself from getting
> confused.
>
> Hmm ... actually, though, what about combining the ideas:
>
>        :'foo' - quote as a literal
>        :"foo" - quote as an ident
>
> This leaves us with nothing much as far as extensibility, but from
> a mnemonic standpoint it's a large win.

Works for me. One small problem discussed upthread is that there
currently doesn't appear to be a libpq function that does
ident-quoting. I'm thinking that we will need to add one to make this
work - is that going to be a problem? I'm thinking that since we're
just adding a function it won't force an uncomfortable major-version
bump on libpq.

I guess the other thing that is bad about this is that someone might
be forgiven for thinking that quotation marks were the way to include
a space in the variable name. But that may be a downside that we can
just live with.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Works for me. One small problem discussed upthread is that there
> currently doesn't appear to be a libpq function that does
> ident-quoting. I'm thinking that we will need to add one to make this
> work - is that going to be a problem?

The rules for ident-quoting are simple and haven't changed over the
years, so we don't really *need* a libpq function for it. OTOH you
could argue it's inconsistent that we have one and not the other.

> I'm thinking that since we're
> just adding a function it won't force an uncomfortable major-version
> bump on libpq.

Yeah, we have taken the position in the past that adding new functions
doesn't require a soname bump.

regards, tom lane

On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> Works for me.  One small problem discussed upthread is that there
>> currently doesn't appear to be a libpq function that does
>> ident-quoting.  I'm thinking that we will need to add one to make this
>> work - is that going to be a problem?
>
> The rules for ident-quoting are simple and haven't changed over the
> years, so we don't really *need* a libpq function for it.  OTOH you
> could argue it's inconsistent that we have one and not the other.

Yeah. Plus it seems like a useful thing to have, anyway.

>> I'm thinking that since we're
>> just adding a function it won't force an uncomfortable major-version
>> bump on libpq.
>
> Yeah, we have taken the position in the past that adding new functions
> doesn't require a soname bump.

Good.

...Robert

On Fri, Dec 25, 2009 at 3:10 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> Works for me.  One small problem discussed upthread is that there
>>> currently doesn't appear to be a libpq function that does
>>> ident-quoting.  I'm thinking that we will need to add one to make this
>>> work - is that going to be a problem?
>>
>> The rules for ident-quoting are simple and haven't changed over the
>> years, so we don't really *need* a libpq function for it.  OTOH you
>> could argue it's inconsistent that we have one and not the other.
>
> Yeah.  Plus it seems like a useful thing to have, anyway.
>
>>> I'm thinking that since we're
>>> just adding a function it won't force an uncomfortable major-version
>>> bump on libpq.
>>
>> Yeah, we have taken the position in the past that adding new functions
>> doesn't require a soname bump.
>
> Good.

So it seems we have agreement on a new direction for this work. We
will not add the \pexec option Pavel proposed as part of this patch;
instead, we will consider a patch that makes :'foo' and :"foo" do
literal and identifier quoting of the corresponding value. Based on
this, I am marking the existing patch as Returned with Feedback, since
what is needed here will amount to a totally base of code, and we can
consider the revised patch if any for whichever CommitFest is open at
the time that patch is submitted.

Thanks,

...Robert

2009/12/28 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Fri, Dec 25, 2009 at 3:10 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>>> Works for me.  One small problem discussed upthread is that there
>>>> currently doesn't appear to be a libpq function that does
>>>> ident-quoting.  I'm thinking that we will need to add one to make this
>>>> work - is that going to be a problem?
>>>
>>> The rules for ident-quoting are simple and haven't changed over the
>>> years, so we don't really *need* a libpq function for it.  OTOH you
>>> could argue it's inconsistent that we have one and not the other.
>>
>> Yeah.  Plus it seems like a useful thing to have, anyway.
>>
>>>> I'm thinking that since we're
>>>> just adding a function it won't force an uncomfortable major-version
>>>> bump on libpq.
>>>
>>> Yeah, we have taken the position in the past that adding new functions
>>> doesn't require a soname bump.
>>
>> Good.
>
> So it seems we have agreement on a new direction for this work.  We
> will not add the \pexec option Pavel proposed as part of this patch;
> instead, we will consider a patch that makes :'foo' and :"foo" do
> literal and identifier quoting of the corresponding value.  Based on
> this, I am marking the existing patch as Returned with Feedback, since
> what is needed here will amount to a totally base of code, and we can
> consider the revised patch if any for whichever CommitFest is open at
> the time that patch is submitted.

ok

Pavel

>
> Thanks,
>
> ...Robert
>