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

Search Discussions

  • Robert Haas at Dec 25, 2009 at 2:28 am

    On Thu, Dec 24, 2009 at 2:45 AM, Pavel Stehule 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
  • Pavel Stehule at Dec 25, 2009 at 8:55 am

    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
  • Tom Lane at Dec 25, 2009 at 5:26 pm

    Robert Haas 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
  • Pavel Stehule at Dec 25, 2009 at 6:03 pm

    2009/12/25 Tom Lane <tgl@sss.pgh.pa.us>:
    Robert Haas <robertmhaas@gmail.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
  • Tom Lane at Dec 25, 2009 at 6:41 pm

    Pavel Stehule 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
  • Pavel Stehule at Dec 25, 2009 at 6:54 pm

    2009/12/25 Tom Lane <tgl@sss.pgh.pa.us>:
    Pavel Stehule <pavel.stehule@gmail.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
  • Robert Haas at Dec 25, 2009 at 7:06 pm

    On Fri, Dec 25, 2009 at 1:41 PM, Tom Lane wrote:
    Pavel Stehule <pavel.stehule@gmail.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
  • Pavel Stehule at Dec 25, 2009 at 7:09 pm

    2009/12/25 Robert Haas <robertmhaas@gmail.com>:
    On Fri, Dec 25, 2009 at 1:41 PM, Tom Lane wrote:
    Pavel Stehule <pavel.stehule@gmail.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
  • Tom Lane at Dec 25, 2009 at 7:13 pm

    Robert Haas 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
  • Pavel Stehule at Dec 25, 2009 at 7:16 pm

    2009/12/25 Tom Lane <tgl@sss.pgh.pa.us>:
    Robert Haas <robertmhaas@gmail.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
  • Robert Haas at Dec 25, 2009 at 7:18 pm

    On Fri, Dec 25, 2009 at 2:12 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.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
  • Tom Lane at Dec 25, 2009 at 7:30 pm

    Robert Haas 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
  • Robert Haas at Dec 25, 2009 at 8:11 pm

    On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.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
  • Robert Haas at Dec 28, 2009 at 7:59 am

    On Fri, Dec 25, 2009 at 3:10 PM, Robert Haas wrote:
    On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.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
  • Pavel Stehule at Dec 28, 2009 at 8:08 am

    2009/12/28 Robert Haas <robertmhaas@gmail.com>:
    On Fri, Dec 25, 2009 at 3:10 PM, Robert Haas wrote:
    On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane wrote:
    Robert Haas <robertmhaas@gmail.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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedDec 24, '09 at 11:31a
activeDec 28, '09 at 8:08a
posts16
users3
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase