Thread: Re: [PATCHES] Escape handling in strings
[switched to -hackers]
Tom Lane wrote:
>Rod Taylor <pg@rbt.ca> writes:
>
>
>>It probably won't be any worse than when '' was rejected for an integer
>>0.
>>
>>
>
>That analogy is *SO* far off the mark that I have to object.
>
>Fooling with quoting rules will not simply cause clean failures, which
>is what you got from ''-no-longer-accepted-by-atoi. What it will cause
>is formerly valid input being silently interpreted as something else.
>That's bad enough, but it gets worse: formerly secure client code may
>now be vulnerable to SQL-injection attacks, because it doesn't know how
>to quote text properly.
>
>What we are talking about here is an extremely significant change with
>extremely serious consequences, and imagining that it is not will be
>a recipe for disaster.
>
>
>
>
All true. Conversely, there does need to be a path for us to get to
standard behaviour.
I think we're going to need to provide for switchable behaviour, as ugly
as that might be (looking briefly at scan.l it looks like the simplest
way would be a separate state for being inside standard strings, with
the choice of state being made conditionally in the {xqstart} rule).
We can't just break backwards compatibility overnight like this.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> All true. Conversely, there does need to be a path for us to get to
> standard behaviour.
Yes --- but the important word there is "path". I think we have to do
this in stages over a number of releases, to give people time to
migrate.
Assuming that the end result we want to get to is:1. Plain '...' literals are per SQL spec: '' for embedded quotes,
backslashesare not special.2. We add a construct E'...' that handles backslash escapes the same way '...' literals do
today.
I think what would be reasonable for 8.1 is to create the E'...'
construct --- which will not cause any backwards compatibility issues
that I can see --- document it and encourage people to migrate,
and start throwing warnings about use of \' in non-E literals.
(We could have a GUC variable to suppress the warnings; I'm of
the opinion that it would be better not to, though, because the point
is to get people out of that habit sooner rather than later.)
I would be inclined to leave things like that for a couple of release
cycles before we disable backslashes in regular literals. By the time
we do that, we should have at least flushed out the cases where
disabling backslashes will create security holes.
> I think we're going to need to provide for switchable behaviour, as ugly
> as that might be (looking briefly at scan.l it looks like the simplest
> way would be a separate state for being inside standard strings, with
> the choice of state being made conditionally in the {xqstart} rule).
I really really dislike that idea; it is a recipe for creating problems
not solving them.
The hard part in all this is to create apps that will survive the
transition gracefully. I think the only way for that is to implement
a reporting feature that lets the app know whether backslahes are
special in plain literals or not. We already have the mechanism for
that, ie read-only GUC variables with GUC_REPORT enabled (which we use
for integer datetimes, for instance). But I really believe it is
important that this be a *read only* thing not something that can be
flipped around at runtime. Anyway, the reporting variable is another
thing that should appear in 8.1.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > All true. Conversely, there does need to be a path for us to get to > > standard behaviour. > > Yes --- but the important word there is "path". I think we have to do > this in stages over a number of releases, to give people time to > migrate. > > Assuming that the end result we want to get to is: > 1. Plain '...' literals are per SQL spec: '' for embedded > quotes, backslashes are not special. > 2. We add a construct E'...' that handles backslash escapes > the same way '...' literals do today. > > I think what would be reasonable for 8.1 is to create the E'...' > construct --- which will not cause any backwards compatibility issues > that I can see --- document it and encourage people to migrate, > and start throwing warnings about use of \' in non-E literals. > (We could have a GUC variable to suppress the warnings; I'm of > the opinion that it would be better not to, though, because the point > is to get people out of that habit sooner rather than later.) OK, the current patch warns about two things, \' with one message, and any backslash in a non-E string with a different message. The \' message can easily be avoided in clients even in 8.0 by using '', but for E'', there is no way to prepare an application before upgrading to 8.1 because 8.0 doesn't have E''. (We can add E'' in a subrelease, but what percentage of users are going to upgrade to that?) This is why I think we need to add a GUC to allow the warning to be turned off. To be clear, the GUC is to control the warning, not the query behavior. We could go with the second warning only in 8.2, but that seems too confusing --- we should deal with the escape issue in two stages, rather than three. > The hard part in all this is to create apps that will survive the > transition gracefully. I think the only way for that is to implement > a reporting feature that lets the app know whether backslahes are > special in plain literals or not. We already have the mechanism for > that, ie read-only GUC variables with GUC_REPORT enabled (which we use > for integer datetimes, for instance). But I really believe it is > important that this be a *read only* thing not something that can be > flipped around at runtime. Anyway, the reporting variable is another > thing that should appear in 8.1. OK, adding. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian said: > OK, the current patch warns about two things, \' with one message, and > any backslash in a non-E string with a different message. The \' > message can easily be avoided in clients even in 8.0 by using '', but > for E'', there is no way to prepare an application before upgrading to > 8.1 because 8.0 doesn't have E''. (We can add E'' in a subrelease, but > what percentage of users are going to upgrade to that?) This is why I > think we need to add a GUC to allow the warning to be turned off. To > be clear, the GUC is to control the warning, not the query behavior. > > We could go with the second warning only in 8.2, but that seems too > confusing --- we should deal with the escape issue in two stages, > rather than three. > So you don't agree with Tom's suggestion to implement E'' a full cycle before removing backslash processing in standard strings? Or have I misunderstood again? cheers andrew
Andrew Dunstan wrote: > Bruce Momjian said: > > > OK, the current patch warns about two things, \' with one message, and > > any backslash in a non-E string with a different message. The \' > > message can easily be avoided in clients even in 8.0 by using '', but > > for E'', there is no way to prepare an application before upgrading to > > 8.1 because 8.0 doesn't have E''. (We can add E'' in a subrelease, but > > what percentage of users are going to upgrade to that?) This is why I > > think we need to add a GUC to allow the warning to be turned off. To > > be clear, the GUC is to control the warning, not the query behavior. ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > We could go with the second warning only in 8.2, but that seems too > > confusing --- we should deal with the escape issue in two stages, > > rather than three. > > > > So you don't agree with Tom's suggestion to implement E'' a full cycle > before removing backslash processing in standard strings? Or have I > misunderstood again? I think you misunderstood. There is no scheduled date to change the actual behavior. The issue is whether we delay one release before issuing a warning for backslashes in non-E strings. I have highlighted the sentence where I say we are talking about when to add the warning, not when to change the behavior. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> OK, the current patch warns about two things, \' with one message, and
> any backslash in a non-E string with a different message.
Those are two very different things. \' is easy to get around and
there's no very good reason not to send '' instead. But avoiding all
use of \anything is impossible (think \\) so a non-suppressable warning
for that would be quite unacceptable IMHO. I think it's much too early
to be throwing a warning for \anything anyway. 8.2 or so, OK, but not
in this cycle.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > OK, the current patch warns about two things, \' with one message, and > > any backslash in a non-E string with a different message. > > Those are two very different things. \' is easy to get around and > there's no very good reason not to send '' instead. But avoiding all > use of \anything is impossible (think \\) so a non-suppressable warning > for that would be quite unacceptable IMHO. I think it's much too early > to be throwing a warning for \anything anyway. 8.2 or so, OK, but not > in this cycle. I am concerned we are going to generate confusing if we warn about one use of backslashes in strings but not another. I am thinking we will just add the infrastructure for E'' in 8.1 (with the warning turned off), and state we will warn about all backslashes in non-E strings in 8.2, and maybe go for literal strings in 8.3 or 8.4 depending on user feedback. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Jun 17, 2005, at 12:33 PM, Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> OK, the current patch warns about two things, \' with one message, >> and >> any backslash in a non-E string with a different message. >> > > Those are two very different things. \' is easy to get around and > there's no very good reason not to send '' instead. But avoiding all > use of \anything is impossible (think \\) so a non-suppressable > warning > for that would be quite unacceptable IMHO. I think it's much too > early > to be throwing a warning for \anything anyway. 8.2 or so, OK, but not > in this cycle. I think giving users a longer period of time to make the necessary changes to their apps is very useful. If (as I understand) we're giving them the opportunity to use E'' strings if they want to continue to use \ for escaping, they can get rid of the warnings now, by using E'' strings or using '' to escape. Getting people to migrate something such as this is difficult and will take them quite a while, I imagine. Giving them a longer time to change their behavior as well as reinforcing it with a warning is helpful. They can also easily check if they've got places they've missed in changing their code, because the warnings will be prominent in their logs. Michael Glaesemann grzm myrealbox com
Note that issuing warnings due to normal DML SQL queries is much more severe than the typical DDL warnings. Many people have queries strewn throughout the application so updating them may be a *lot* of work. And for an app issuing hundreds or thousands of queries per minute (or even second) a warning could effectively be a showstopper. It could require disabling all warnings in their config to avoid filling their disk with Postgres logs in minutes. I would suggest this warning be disable-able with a GUC variable. Otherwise you're effectively giving no advance warning time to those users. If postgres keeps advancing at the pace it's advancing now I might suggest waiting two release cycles instead of just one. Judging by the mailing list it seems a lot of users aren't able to keep up with the Postgres development team and are often upgrading two versions at a time. -- greg
On Jun 17, 2005, at 4:34 PM, Greg Stark wrote: > And for an app issuing > hundreds or thousands of queries per minute (or even second) a > warning could > effectively be a showstopper. It could require disabling all > warnings in their > config to avoid filling their disk with Postgres logs in minutes. Good point. > I would suggest this warning be disable-able with a GUC variable. > Otherwise > you're effectively giving no advance warning time to those users. Perhaps NOTICE would be better, at least for the first step? People might be more comfortable with that, as using backslash escaping isn't really going to cause problems with this particular version, but rather for future versions. > If postgres keeps advancing at the pace it's advancing now I might > suggest > waiting two release cycles instead of just one. How's this for an idea? Step 1 (8.1) NOTICE level (or some other level, lower than WARNING), E'' and \' are available Step 2 (8.2?) WARNING level, E'' and \' are available Step 3 (8.3? 8.4?) E'' available, plain '' interpreted literally. Michael Glaesemann grzm myrealbox com
Michael Glaesemann wrote: > > On Jun 17, 2005, at 4:34 PM, Greg Stark wrote: > > > And for an app issuing > > hundreds or thousands of queries per minute (or even second) a > > warning could > > effectively be a showstopper. It could require disabling all > > warnings in their > > config to avoid filling their disk with Postgres logs in minutes. > > Good point. > > > I would suggest this warning be disable-able with a GUC variable. > > Otherwise > > you're effectively giving no advance warning time to those users. > > Perhaps NOTICE would be better, at least for the first step? People > might be more comfortable with that, as using backslash escaping > isn't really going to cause problems with this particular version, > but rather for future versions. I am thinking changing the level of the message isn't going to help people much because it still displays and fills up the server logs. > > If postgres keeps advancing at the pace it's advancing now I might > > suggest > > waiting two release cycles instead of just one. > > How's this for an idea? > > Step 1 (8.1) NOTICE level (or some other level, lower than WARNING), > E'' and \' are available > Step 2 (8.2?) WARNING level, E'' and \' are available > Step 3 (8.3? 8.4?) E'' available, plain '' interpreted literally. Right now I am thinking we would have the warning available in 8.1, but not turn it on by default. Perhaps we can tell users to enable the warning at some time during 8.1 so they are ready for it in 8.2. If we get a significant must-upgrade 8.0.X release a few months before 8.1, we can tell them to change \' to '' and perhaps have the \' warning be enabled always in 8.1. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Friday 17 June 2005 08:55, Bruce Momjian wrote:
> Michael Glaesemann wrote:
> > On Jun 17, 2005, at 4:34 PM, Greg Stark wrote:
> > > And for an app issuing
> > > hundreds or thousands of queries per minute (or even second) a
> > > warning could
> > > effectively be a showstopper. It could require disabling all
> > > warnings in their
> > > config to avoid filling their disk with Postgres logs in minutes.
> >
> > Good point.
> >
> > > I would suggest this warning be disable-able with a GUC variable.
> > > Otherwise
> > > you're effectively giving no advance warning time to those users.
> >
> > Perhaps NOTICE would be better, at least for the first step? People
> > might be more comfortable with that, as using backslash escaping
> > isn't really going to cause problems with this particular version,
> > but rather for future versions.
>
> I am thinking changing the level of the message isn't going to help
> people much because it still displays and fills up the server logs.
>
> > > If postgres keeps advancing at the pace it's advancing now I might
> > > suggest
> > > waiting two release cycles instead of just one.
> >
> > How's this for an idea?
> >
> > Step 1 (8.1) NOTICE level (or some other level, lower than WARNING),
> > E'' and \' are available
> > Step 2 (8.2?) WARNING level, E'' and \' are available
> > Step 3 (8.3? 8.4?) E'' available, plain '' interpreted literally.
>
> Right now I am thinking we would have the warning available in 8.1, but
> not turn it on by default. Perhaps we can tell users to enable the
> warning at some time during 8.1 so they are ready for it in 8.2.
>
I think it is worth restating in stronger language, the potential overhead of
raising notices or warning in such a large number of queries will be an
upgrading show stopper for some people. (To the extent that for some, the
release where this is a mandatory warning will be as much a show stopper as
the release where the behavior is changed)
IMHO we need at least 1 release with a GUC to control the warning (defaulting
off initial, if people want the next release to default on, thats ok, but is
probably a waste), so that people can turn it on/off in order to debug thier
applications and make them compliant for upgrading to the next version. It
doesnt much matter to me where you put this... 8.0.x, 8.1... it's just a
question of where do you want to create a roadblock to upgrading, because the
release where you force the warning always on your going to have raised the
barrier to entry too high for some people.
--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
Robert Treat wrote: > I think it is worth restating in stronger language, the potential overhead of > raising notices or warning in such a large number of queries will be an > upgrading show stopper for some people. (To the extent that for some, the > release where this is a mandatory warning will be as much a show stopper as > the release where the behavior is changed) > > IMHO we need at least 1 release with a GUC to control the warning (defaulting > off initial, if people want the next release to default on, thats ok, but is > probably a waste), so that people can turn it on/off in order to debug thier > applications and make them compliant for upgrading to the next version. It > doesnt much matter to me where you put this... 8.0.x, 8.1... it's just a > question of where do you want to create a roadblock to upgrading, because the > release where you force the warning always on your going to have raised the > barrier to entry too high for some people. The GUC will always be around to turn the warning on or off, until we go with standard SQL strings, at which point there will be no warnings generated. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073