Обсуждение: 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.stehule@gmail.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@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 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@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 >
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. regards, tom lane
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 >
On Fri, Dec 25, 2009 at 1:41 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
2009/12/25 Robert Haas <robertmhaas@gmail.com>: > On Fri, Dec 25, 2009 at 1:41 PM, Tom Lane <tgl@sss.pgh.pa.us> 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 >
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. regards, tom lane
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 >
On Fri, Dec 25, 2009 at 2:12 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
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. > 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@sss.pgh.pa.us> 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
On Fri, Dec 25, 2009 at 3:10 PM, Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
2009/12/28 Robert Haas <robertmhaas@gmail.com>: > On Fri, Dec 25, 2009 at 3:10 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> On Fri, Dec 25, 2009 at 2:30 PM, Tom Lane <tgl@sss.pgh.pa.us> 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 >