Обсуждение: Escaping strings for inclusion into SQL queries

It has come to our attention that many applications which use libpq
are vulnerable to code insertion attacks in strings and identifiers
passed to these applications.  We have collected some evidence which
suggests that this is related to the fact that libpq does not provide
a function to escape strings and identifiers properly.  (Both the
Oracle and MySQL client libraries include such a function, and the
vast majority of applications we examined are not vulnerable to code
insertion attacks because they use this function.)

We therefore suggest that a string escaping function is included in a
future version of PostgreSQL and libpq.  A sample implementation is
provided below, along with documentation.

--
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

On Wed, Aug 22, 2001 at 05:16:44PM +0000, Florian Weimer wrote:
> We therefore suggest that a string escaping function is included in a
> future version of PostgreSQL and libpq.  A sample implementation is
> provided below, along with documentation.

I use Perl, which (through DBD::Pg) has a "quote" function available,
but I think this is a very good idea to include in the library.

I only have one issue - the SQL standard seems to support the use
of '' to escape a single quote, but not \'.  Though PostgreSQL has
an extended notion of character string literals, I think that the
usual policy of using the standard interface when possible should
apply.
-- 
Christopher Masto         Senior Network Monkey      NetMonger Communications
chris@netmonger.net        info@netmonger.net        http://www.netmonger.net

Free yourself, free your machine, free the daemon -- http://www.freebsd.org/

Christopher Masto <chris@netmonger.net> writes:

> I only have one issue - the SQL standard seems to support the use
> of '' to escape a single quote, but not \'.  Though PostgreSQL has
> an extended notion of character string literals, I think that the
> usual policy of using the standard interface when possible should
> apply.

The first version escaped ' with ''.  I changed it when I noticed that
if \' is used instead, the same function can be used for strings
('...') and identifiers ("...").

In addition, you have to replace \ with \\, so you are forced
to leave the grounds of the standard anyway.

-- 
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

Florian Weimer <Florian.Weimer@rus.uni-stuttgart.de> writes:

> We therefore suggest that a string escaping function is included in a
> future version of PostgreSQL and libpq.  A sample implementation is
> provided below, along with documentation.

We have now released a description of the problems which occur when a
string escaping function is not used:

http://cert.uni-stuttgart.de/advisories/apache_auth.php

What further steps are required to make the suggested patch part of
the official libpq library?

Thanks,
-- 
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

> It has come to our attention that many applications which use libpq
> are vulnerable to code insertion attacks in strings and identifiers
> passed to these applications.  We have collected some evidence which
> suggests that this is related to the fact that libpq does not provide
> a function to escape strings and identifiers properly.  (Both the
> Oracle and MySQL client libraries include such a function, and the
> vast majority of applications we examined are not vulnerable to code
> insertion attacks because they use this function.)
> 
> We therefore suggest that a string escaping function is included in a
> future version of PostgreSQL and libpq.  A sample implementation is
> provided below, along with documentation.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

> Florian Weimer <Florian.Weimer@rus.uni-stuttgart.de> writes:
> 
> > We therefore suggest that a string escaping function is included in a
> > future version of PostgreSQL and libpq.  A sample implementation is
> > provided below, along with documentation.
> 
> We have now released a description of the problems which occur when a
> string escaping function is not used:
> 
> http://cert.uni-stuttgart.de/advisories/apache_auth.php
> 
> What further steps are required to make the suggested patch part of
> the official libpq library?

Will be applied soon.  I was waiting for comments before adding it to
the patch queue.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Perhaps I'm not thinking correctly but isn't it the job of the application
that's using the libpq library to escape special characters? I guess I don't
see a down side though, if it's implemented correctly to check and see if
characters are already escaped before escaping them (else major breakage of
existing application would occur).. I didn't see the patch but I assume that
someone took a look to make sure before applying it.


-Mitch

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>
To: "Florian Weimer" <Florian.Weimer@rus.uni-stuttgart.de>
Cc: <pgsql-hackers@postgresql.org>
Sent: Thursday, August 30, 2001 6:43 PM
Subject: Re: [HACKERS] Escaping strings for inclusion into SQL queries


> > Florian Weimer <Florian.Weimer@rus.uni-stuttgart.de> writes:
> >
> > > We therefore suggest that a string escaping function is included in a
> > > future version of PostgreSQL and libpq.  A sample implementation is
> > > provided below, along with documentation.
> >
> > We have now released a description of the problems which occur when a
> > string escaping function is not used:
> >
> > http://cert.uni-stuttgart.de/advisories/apache_auth.php
> >
> > What further steps are required to make the suggested patch part of
> > the official libpq library?
>
> Will be applied soon.  I was waiting for comments before adding it to
> the patch queue.

It is. Application is responsible to call PGescapeString (included in the
patch in question) to escape command that may possibly have user-specified
data... This function isn't called automatically.

On Thu, 30 Aug 2001, Mitch Vincent wrote:

> Perhaps I'm not thinking correctly but isn't it the job of the application
> that's using the libpq library to escape special characters? I guess I don't
> see a down side though, if it's implemented correctly to check and see if
> characters are already escaped before escaping them (else major breakage of
> existing application would occur).. I didn't see the patch but I assume that
> someone took a look to make sure before applying it.
> 
> 
> -Mitch
> 
> ----- Original Message -----
> From: "Bruce Momjian" <pgman@candle.pha.pa.us>
> To: "Florian Weimer" <Florian.Weimer@rus.uni-stuttgart.de>
> Cc: <pgsql-hackers@postgresql.org>
> Sent: Thursday, August 30, 2001 6:43 PM
> Subject: Re: [HACKERS] Escaping strings for inclusion into SQL queries
> 
> 
> > > Florian Weimer <Florian.Weimer@rus.uni-stuttgart.de> writes:
> > >
> > > > We therefore suggest that a string escaping function is included in a
> > > > future version of PostgreSQL and libpq.  A sample implementation is
> > > > provided below, along with documentation.
> > >
> > > We have now released a description of the problems which occur when a
> > > string escaping function is not used:
> > >
> > > http://cert.uni-stuttgart.de/advisories/apache_auth.php
> > >
> > > What further steps are required to make the suggested patch part of
> > > the official libpq library?
> >
> > Will be applied soon.  I was waiting for comments before adding it to
> > the patch queue.
> 
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
> http://www.postgresql.org/search.mpl
> 
> 

"Mitch Vincent" <mvincent@cablespeed.com> writes:

> Perhaps I'm not thinking correctly but isn't it the job of the application
> that's using the libpq library to escape special characters?

Yes, it is.

> I guess I don't see a down side though, if it's implemented
> correctly to check and see if characters are already escaped before
> escaping them (else major breakage of existing application would
> occur)..

You can't do this automatically because the strings needing escaping
are not marked in any way at the moment.

-- 
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

Ok, I misudnerstood, I had long included my own escaping function in
programs that used libpq, I thought the intent was to make escaping happen
automatically..

Thanks!

-Mitch

----- Original Message -----
From: "Alex Pilosov" <alex@pilosoft.com>
To: "Mitch Vincent" <mvincent@cablespeed.com>
Cc: <pgsql-hackers@postgresql.org>
Sent: Thursday, August 30, 2001 7:32 PM
Subject: Re: [HACKERS] Escaping strings for inclusion into SQL queries


> It is. Application is responsible to call PGescapeString (included in the
> patch in question) to escape command that may possibly have user-specified
> data... This function isn't called automatically.
>
> On Thu, 30 Aug 2001, Mitch Vincent wrote:
>
> > Perhaps I'm not thinking correctly but isn't it the job of the
application
> > that's using the libpq library to escape special characters? I guess I
don't
> > see a down side though, if it's implemented correctly to check and see
if
> > characters are already escaped before escaping them (else major breakage
of
> > existing application would occur).. I didn't see the patch but I assume
that
> > someone took a look to make sure before applying it.
> >
> >
> > -Mitch
> >
> > ----- Original Message -----
> > From: "Bruce Momjian" <pgman@candle.pha.pa.us>
> > To: "Florian Weimer" <Florian.Weimer@rus.uni-stuttgart.de>
> > Cc: <pgsql-hackers@postgresql.org>
> > Sent: Thursday, August 30, 2001 6:43 PM
> > Subject: Re: [HACKERS] Escaping strings for inclusion into SQL queries
> >
> >
> > > > Florian Weimer <Florian.Weimer@rus.uni-stuttgart.de> writes:
> > > >
> > > > > We therefore suggest that a string escaping function is included
in a
> > > > > future version of PostgreSQL and libpq.  A sample implementation
is
> > > > > provided below, along with documentation.
> > > >
> > > > We have now released a description of the problems which occur when
a
> > > > string escaping function is not used:
> > > >
> > > > http://cert.uni-stuttgart.de/advisories/apache_auth.php
> > > >
> > > > What further steps are required to make the suggested patch part of
> > > > the official libpq library?
> > >
> > > Will be applied soon.  I was waiting for comments before adding it to
> > > the patch queue.
> >
> >
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 6: Have you searched our list archives?
> >
> > http://www.postgresql.org/search.mpl
> >
> >
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/users-lounge/docs/faq.html
>

Bruce Momjian wrote:
> 
> Your patch has been added to the PostgreSQL unapplied patches list at:
> 
>         http://candle.pha.pa.us/cgi-bin/pgpatches
> 
> I will try to apply it within the next 48 hours.
> 
> > It has come to our attention that many applications which use libpq
> > are vulnerable to code insertion attacks in strings and identifiers
> > passed to these applications.  We have collected some evidence which
> > suggests that this is related to the fact that libpq does not provide
> > a function to escape strings and identifiers properly.  (Both the
> > Oracle and MySQL client libraries include such a function, and the
> > vast majority of applications we examined are not vulnerable to code
> > insertion attacks because they use this function.)

I think the real difference is what I complained in another mail to this
list - 
in postgresql you can't do PREPARE / EXECUTE which could _automatically_
detect 
where string escaping is needed or just eliminate the need for escaping.
In postgreSQL you have to construct all queries yourself by inserting
your 
parameters inside your query strings in right places and escaping them
when 
needed. That is unless you use an interface like ODBC/JDBS that fakes
the 
PREPARE/EXECUTE on the client side and thus does the auto-escaping for
you .


I think that this should be added to TODO

* make portable BINARY representation for frontend-backend protocol by
using  typsend/typreceive functions for binary and typinput typoutput for
ASCII (as currently typinput==typreceive and typoutput==typsend is suspect
the  usage to be inconsistent). 

* make SQL changes to allow PREPARE/EXECUTE in main session, not only in
SPI

* make changes to client libraries to support marshalling arguments to
EXECUTE using BINARY wire protocol or correctly escaped ASCII. The binary
protocol  would be very helpful for BYTEA and other big binary types.


> > We therefore suggest that a string escaping function is included in a
> > future version of PostgreSQL and libpq.  A sample implementation is
> > provided below, along with documentation.

While you are at it you could also supply a standard query delimiter
function
as this is also a thing that seems to vary from db to db.

------------------
Hannu

I agree with Hannu, that:
 * make SQL changes to allow PREPARE/EXECUTE in main session, not only 
in SPI

is an important feature to expose out to the client.  My primary reason 
is a perfomance one.  Allowing the client to parse a SQL statement once 
and then supplying bind values for arguments and executing it multiple 
times can save a significant amount of server CPU, since the parsing and 
planning of the statement is only done once, even though multiple 
executions occur.  This functionality is available in the backend 
(through SPI) and plpgsql uses it, but there isn't anyway to take 
advantage of this SPI functionality on the client (i.e. jdbc, odbc, etc.)

I could see this implemented in different ways.  One, by adding new SQL 
commands to bind or execute an already open statement, or two, by 
changing the FE/BE protocol to allow the client to open, parse, 
describe, bind, execute and close a statement as separate actions that 
can be sent to the server in one or more requests.  (The latter is how 
Oracle does it).

I also would like to see this added to the todo list.

thanks,
--Barry


Hannu Krosing wrote:
> Bruce Momjian wrote:
> 
>>Your patch has been added to the PostgreSQL unapplied patches list at:
>>
>>        http://candle.pha.pa.us/cgi-bin/pgpatches
>>
>>I will try to apply it within the next 48 hours.
>>
>>
>>>It has come to our attention that many applications which use libpq
>>>are vulnerable to code insertion attacks in strings and identifiers
>>>passed to these applications.  We have collected some evidence which
>>>suggests that this is related to the fact that libpq does not provide
>>>a function to escape strings and identifiers properly.  (Both the
>>>Oracle and MySQL client libraries include such a function, and the
>>>vast majority of applications we examined are not vulnerable to code
>>>insertion attacks because they use this function.)
>>>
> 
> I think the real difference is what I complained in another mail to this
> list - 
> in postgresql you can't do PREPARE / EXECUTE which could _automatically_
> detect 
> where string escaping is needed or just eliminate the need for escaping.
> In postgreSQL you have to construct all queries yourself by inserting
> your 
> parameters inside your query strings in right places and escaping them
> when 
> needed. That is unless you use an interface like ODBC/JDBS that fakes
> the 
> PREPARE/EXECUTE on the client side and thus does the auto-escaping for
> you .
> 
> 
> I think that this should be added to TODO
> 
> * make portable BINARY representation for frontend-backend protocol by
> using 
>   typsend/typreceive functions for binary and typinput typoutput for
> ASCII
>   (as currently typinput==typreceive and typoutput==typsend is suspect
> the 
>   usage to be inconsistent). 
> 
> * make SQL changes to allow PREPARE/EXECUTE in main session, not only in
> SPI
> 
> * make changes to client libraries to support marshalling arguments to
> EXECUTE
>   using BINARY wire protocol or correctly escaped ASCII. The binary
> protocol 
>   would be very helpful for BYTEA and other big binary types.
> 
> 
> 
>>>We therefore suggest that a string escaping function is included in a
>>>future version of PostgreSQL and libpq.  A sample implementation is
>>>provided below, along with documentation.
>>>
> 
> While you are at it you could also supply a standard query delimiter
> function
> as this is also a thing that seems to vary from db to db.
> 
> ------------------
> Hannu
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
> http://www.postgresql.org/search.mpl
> 
> 

Barry Lind wrote:
> 
> I agree with Hannu, that:
> 
>   * make SQL changes to allow PREPARE/EXECUTE in main session, not only
> in SPI

A more ambitious project would be 

* develop an ANSI standard SQL/CLI compatible postgreSQL client library, change wire protocol and SQL language as
needed;)
 

> is an important feature to expose out to the client.  My primary reason
> is a perfomance one.  Allowing the client to parse a SQL statement once
> and then supplying bind values for arguments and executing it multiple
> times can save a significant amount of server CPU, since the parsing and
> planning of the statement is only done once, even though multiple
> executions occur.  This functionality is available in the backend
> (through SPI) and plpgsql uses it, but there isn't anyway to take
> advantage of this SPI functionality on the client (i.e. jdbc, odbc, etc.)
> 
> I could see this implemented in different ways.  One, by adding new SQL
> commands to bind or execute an already open statement, or two, by
> changing the FE/BE protocol to allow the client to open, parse,
> describe, bind, execute and close a statement as separate actions that
> can be sent to the server in one or more requests.  (The latter is how
> Oracle does it).

The latter is also the ODBS and JDBC wiew of how it is done. The current 
PG drivers have to fake it all on client side. 

> 
> I also would like to see this added to the todo list.
> 

------------
Hannu

Florian Weimer writes:

> The first version escaped ' with ''.  I changed it when I noticed that
> if \' is used instead, the same function can be used for strings
> ('...') and identifiers ("...").

Last time I checked (15 seconds ago), you could not escape " with \ in
PostgreSQL.  The identifer parsing rules are a bit different from strings.

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter

For consistency with the rest of the libpq API, the function should be
called PQescapeString, not PGescapeString.

Bruce Momjian writes:

>
> Your patch has been added to the PostgreSQL unapplied patches list at:
>
>     http://candle.pha.pa.us/cgi-bin/pgpatches
>
> I will try to apply it within the next 48 hours.
>
> > It has come to our attention that many applications which use libpq
> > are vulnerable to code insertion attacks in strings and identifiers
> > passed to these applications.  We have collected some evidence which
> > suggests that this is related to the fact that libpq does not provide
> > a function to escape strings and identifiers properly.  (Both the
> > Oracle and MySQL client libraries include such a function, and the
> > vast majority of applications we examined are not vulnerable to code
> > insertion attacks because they use this function.)
> >
> > We therefore suggest that a string escaping function is included in a
> > future version of PostgreSQL and libpq.  A sample implementation is
> > provided below, along with documentation.
> >
> > --
> > Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> > University of Stuttgart           http://cert.uni-stuttgart.de/
> > RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
>
> [ Attachment, skipping... ]
>
> [ Attachment, skipping... ]
>
> [ Attachment, skipping... ]
>
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 2: you can get off all lists at once with the unregister command
> >     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
>
>

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:

> Florian Weimer writes:
> 
> > The first version escaped ' with ''.  I changed it when I noticed that
> > if \' is used instead, the same function can be used for strings
> > ('...') and identifiers ("...").
> 
> Last time I checked (15 seconds ago), you could not escape " with \ in
> PostgreSQL.  The identifer parsing rules are a bit different from strings.

Yes, we misread the lexer description.  I'm sorry about that.

In addition, there seems to be a bug in the treatment of "" escapes in
identifiers. 'SELECT """";' yields the error message 'Attribute '""'
not found ' (not '"'!) or even 'Attribute '""\' not found', depending
on the queries executed before.

For identifiers, comparing the characters to a white list is probably
a more reasonable approach.

-- 
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

OK, can you supply an updated patch?


> Peter Eisentraut <peter_e@gmx.net> writes:
> 
> > Florian Weimer writes:
> > 
> > > The first version escaped ' with ''.  I changed it when I noticed that
> > > if \' is used instead, the same function can be used for strings
> > > ('...') and identifiers ("...").
> > 
> > Last time I checked (15 seconds ago), you could not escape " with \ in
> > PostgreSQL.  The identifer parsing rules are a bit different from strings.
> 
> Yes, we misread the lexer description.  I'm sorry about that.
> 
> In addition, there seems to be a bug in the treatment of "" escapes in
> identifiers. 'SELECT """";' yields the error message 'Attribute '""'
> not found ' (not '"'!) or even 'Attribute '""\' not found', depending
> on the queries executed before.
> 
> For identifiers, comparing the characters to a white list is probably
> a more reasonable approach.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
> 
> http://www.postgresql.org/users-lounge/docs/faq.html
> 

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Bruce Momjian <pgman@candle.pha.pa.us> writes:

> OK, can you supply an updated patch?

Yes, I'm going to update it.  Shall I post it here?

Could anybody have a look at the parser issue?

-- 
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> 
> > OK, can you supply an updated patch?
> 
> Yes, I'm going to update it.  Shall I post it here?

Sure, or patches list.

> Could anybody have a look at the parser issue?

I am unsure how it is supposed to behave.  Comments?  Does the standard
say anything?

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Florian Weimer writes:

> In addition, there seems to be a bug in the treatment of "" escapes in
> identifiers. 'SELECT """";' yields the error message 'Attribute '""'
> not found ' (not '"'!) or even 'Attribute '""\' not found', depending
> on the queries executed before.

A bug indeed.

RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/parser/scan.l,v
retrieving revision 1.88
diff -u -r1.88 scan.l
--- scan.l      2001/03/22 17:41:47     1.88
+++ scan.l      2001/09/03 22:11:46
@@ -375,7 +375,7 @@                                       return IDENT;                               }<xd>{xddouble}
{
-                                       addlit(yytext, yyleng-1);
+                                       addlit(yytext+1, yyleng-1);                               }<xd>{xdinside} {
                                 addlit(yytext, yyleng);
 
===end

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:
> A bug indeed.

>  <xd>{xddouble} {
> -                                       addlit(yytext, yyleng-1);
> +                                       addlit(yytext+1, yyleng-1);
>                                 }

I don't follow.  xddouble can only expand to two quote marks, so how
does it matter which one we use as the result?  This seems unlikely
to change the behavior.  If it does, I think the real bug is elsewhere.

I do see a bug here --- I get

regression=# select """";
NOTICE:  identifier """ [ lots o' rubouts ] @;�" will be truncated to
""""
ERROR:  Attribute '""' not found
regression=#
        regards, tom lane

Tom Lane writes:

> Peter Eisentraut <peter_e@gmx.net> writes:
> > A bug indeed.
>
> >  <xd>{xddouble} {
> > -                                       addlit(yytext, yyleng-1);
> > +                                       addlit(yytext+1, yyleng-1);
> >                                 }
>
> I don't follow.  xddouble can only expand to two quote marks, so how
> does it matter which one we use as the result?

addlit() expects the first argument to be null-terminated and implicitly
uses that null byte at the end of the supplied argument to terminate its
own buffer.  It expects to copy <doublequote><null> (new version), whereas
it got (old version) <doublequote><doublequote> and left the buffer
unterminated, which leads to random behavior, as you saw.

Since there are only a few calls to addlit(), I didn't feel like
re-engineering the whole interface to be prettier.  It does look like a
performance-beneficial implementation.

A concern related to the matter is that if you actually put such an
identifier into your database you basically make it undumpable (well,
unrestorable) because no place is prepared to handle such a thing.

-- 
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter

Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane writes:
>> I don't follow.  xddouble can only expand to two quote marks, so how
>> does it matter which one we use as the result?

> addlit() expects the first argument to be null-terminated and implicitly
> uses that null byte at the end of the supplied argument to terminate its
> own buffer.

Hmm, so I see:
/* append data --- note we assume ytext is null-terminated */memcpy(literalbuf+literallen, ytext, yleng+1);literallen
+=yleng;
 

Given that we are passing the length of the desired string, it seems
bug-prone for addlit to *also* expect null termination.  I'd suggest
memcpy(literalbuf+literallen, ytext, yleng);literallen += yleng;literalbuf[literallen] = '\0';

instead.
        regards, tom lane

I am going to apply this patch with the change that the function name
will be PQ* not PG*.

> It has come to our attention that many applications which use libpq
> are vulnerable to code insertion attacks in strings and identifiers
> passed to these applications.  We have collected some evidence which
> suggests that this is related to the fact that libpq does not provide
> a function to escape strings and identifiers properly.  (Both the
> Oracle and MySQL client libraries include such a function, and the
> vast majority of applications we examined are not vulnerable to code
> insertion attacks because they use this function.)
> 
> We therefore suggest that a string escaping function is included in a
> future version of PostgreSQL and libpq.  A sample implementation is
> provided below, along with documentation.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Patch removed at the request of the author.  Author will resubmit.

> It has come to our attention that many applications which use libpq
> are vulnerable to code insertion attacks in strings and identifiers
> passed to these applications.  We have collected some evidence which
> suggests that this is related to the fact that libpq does not provide
> a function to escape strings and identifiers properly.  (Both the
> Oracle and MySQL client libraries include such a function, and the
> vast majority of applications we examined are not vulnerable to code
> insertion attacks because they use this function.)
> 
> We therefore suggest that a string escaping function is included in a
> future version of PostgreSQL and libpq.  A sample implementation is
> provided below, along with documentation.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

[ Attachment, skipping... ]

[ Attachment, skipping... ]

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Bruce Momjian <pgman@candle.pha.pa.us> writes:

> Patch removed at the request of the author.  Author will resubmit.

I've attached the fixed version of the patch below.  After the
discussion on pgsql-hackers (especially the frightening memory dump in
<12273.999562219@sss.pgh.pa.us>), we decided that it is best not to
use identifiers from an untrusted source at all.  Therefore, all
claims of the suitability of PQescapeString() for identifiers have
been removed.

--
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.68
diff -u -r1.68 libpq.sgml
--- doc/src/sgml/libpq.sgml    2001/09/04 00:18:18    1.68
+++ doc/src/sgml/libpq.sgml    2001/09/04 18:32:05
@@ -827,6 +827,42 @@
 </itemizedlist>
 </sect2>

+<sect2 id="libpq-exec-escape-string">
+  <title>Escaping strings for inclusion in SQL queries</title>
+<para>
+<function>PQescapeString</function>
+          Escapes a string for use within an SQL query.
+<synopsis>
+size_t PQescapeString (char *to, const char *from, size_t length);
+</synopsis>
+If you want to include strings which have been received
+from a source which is not trustworthy (for example, because they were
+transmitted across a network), you cannot directly include them in SQL
+queries for security reasons.  Instead, you have to quote special
+characters which are otherwise interpreted by the SQL parser.
+</para>
+<para>
+<function>PQescapeString</> performs this operation.  The
+<parameter>from</> points to the first character of the string which
+is to be escaped, and the <parameter>length</> parameter counts the
+number of characters in this string (a terminating NUL character is
+neither necessary nor counted).  <parameter>to</> shall point to a
+buffer which is able to hold at least one more character than twice
+the value of <parameter>length</>, otherwise the behavior is
+undefined.  A call to <function>PQescapeString</> writes an escaped
+version of the <parameter>from</> string to the <parameter>to</>
+buffer, replacing special characters so that they cannot cause any
+harm, and adding a terminating NUL character.  The single quotes which
+must surround PostgreSQL string literals are not part of the result
+string.
+</para>
+<para>
+<function>PQescapeString</> returns the number of characters written
+to <parameter>to</>, not including the terminating NUL character.
+Behavior is undefined when the <parameter>to</> and <parameter>from</>
+strings overlap.
+</para>
+
 <sect2 id="libpq-exec-select-info">
   <title>Retrieving SELECT Result Information</title>

Index: src/interfaces/libpq/fe-exec.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.108
diff -u -r1.108 fe-exec.c
--- src/interfaces/libpq/fe-exec.c    2001/08/21 20:39:53    1.108
+++ src/interfaces/libpq/fe-exec.c    2001/09/04 18:32:09
@@ -56,6 +56,62 @@
 static int    getNotify(PGconn *conn);
 static int    getNotice(PGconn *conn);

+/* ---------------
+ * Escaping arbitrary strings to get valid SQL strings/identifiers.
+ *
+ * Replaces "\\" with "\\\\", "\0" with "\\0", and "'" with "''".
+ * length is the length of the buffer pointed to by
+ * from.  The buffer at to must be at least 2*length + 1 characters
+ * long.  A terminating NUL character is written.
+ * ---------------
+ */
+
+size_t
+PQescapeString (char *to, const char *from, size_t length)
+{
+    const char *source = from;
+    char *target = to;
+    unsigned int remaining = length;
+
+    while (remaining > 0) {
+        switch (*source) {
+        case '\0':
+            *target = '\\';
+            target++;
+            *target = '0';
+            /* target and remaining are updated below. */
+            break;
+
+        case '\\':
+            *target = '\\';
+            target++;
+            *target = '\\';
+            /* target and remaining are updated below. */
+            break;
+
+        case '\'':
+            *target = '\'';
+            target++;
+            *target = '\'';
+            /* target and remaining are updated below. */
+            break;
+
+        default:
+            *target = *source;
+            /* target and remaining are updated below. */
+        }
+        source++;
+        target++;
+        remaining--;
+    }
+
+    /* Write the terminating NUL character. */
+    *target = '\0';
+
+    return target - to;
+}
+
+

 /* ----------------
  * Space management for PGresult.
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.72
diff -u -r1.72 libpq-fe.h
--- src/interfaces/libpq/libpq-fe.h    2001/08/21 20:39:54    1.72
+++ src/interfaces/libpq/libpq-fe.h    2001/09/04 18:32:09
@@ -251,6 +251,9 @@

 /* === in fe-exec.c === */

+    /* Quoting strings before inclusion in queries. */
+    extern size_t PQescapeString (char *to, const char *from, size_t length);
+
     /* Simple synchronous query */
     extern PGresult *PQexec(PGconn *conn, const char *query);
     extern PGnotify *PQnotifies(PGconn *conn);

Has this been resolved?


> Peter Eisentraut <peter_e@gmx.net> writes:
> > Tom Lane writes:
> >> I don't follow.  xddouble can only expand to two quote marks, so how
> >> does it matter which one we use as the result?
> 
> > addlit() expects the first argument to be null-terminated and implicitly
> > uses that null byte at the end of the supplied argument to terminate its
> > own buffer.
> 
> Hmm, so I see:
> 
>     /* append data --- note we assume ytext is null-terminated */
>     memcpy(literalbuf+literallen, ytext, yleng+1);
>     literallen += yleng;
> 
> Given that we are passing the length of the desired string, it seems
> bug-prone for addlit to *also* expect null termination.  I'd suggest
> 
>     memcpy(literalbuf+literallen, ytext, yleng);
>     literallen += yleng;
>     literalbuf[literallen] = '\0';
> 
> instead.
> 
>             regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
> 
> http://www.postgresql.org/search.mpl
> 

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Has this been resolved?

Peter applied his patch, but I am planning to also change addlit to not
require null termination, because I believe we'll get bit again if we
don't.
        regards, tom lane

Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> 
> > Patch removed at the request of the author.  Author will resubmit.
> 
> I've attached the fixed version of the patch below.  After the
> discussion on pgsql-hackers (especially the frightening memory dump in
> <12273.999562219@sss.pgh.pa.us>), we decided that it is best not to
> use identifiers from an untrusted source at all.  Therefore, all
> claims of the suitability of PQescapeString() for identifiers have
> been removed.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
> 

> Index: doc/src/sgml/libpq.sgml
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
> retrieving revision 1.68
> diff -u -r1.68 libpq.sgml
> --- doc/src/sgml/libpq.sgml    2001/09/04 00:18:18    1.68
> +++ doc/src/sgml/libpq.sgml    2001/09/04 18:32:05
> @@ -827,6 +827,42 @@
>  </itemizedlist>
>  </sect2>
>  
> +<sect2 id="libpq-exec-escape-string">
> +  <title>Escaping strings for inclusion in SQL queries</title>
> +<para>
> +<function>PQescapeString</function>
> +          Escapes a string for use within an SQL query.
> +<synopsis>
> +size_t PQescapeString (char *to, const char *from, size_t length);
> +</synopsis>
> +If you want to include strings which have been received
> +from a source which is not trustworthy (for example, because they were
> +transmitted across a network), you cannot directly include them in SQL
> +queries for security reasons.  Instead, you have to quote special
> +characters which are otherwise interpreted by the SQL parser.
> +</para>
> +<para>
> +<function>PQescapeString</> performs this operation.  The
> +<parameter>from</> points to the first character of the string which
> +is to be escaped, and the <parameter>length</> parameter counts the
> +number of characters in this string (a terminating NUL character is
> +neither necessary nor counted).  <parameter>to</> shall point to a
> +buffer which is able to hold at least one more character than twice
> +the value of <parameter>length</>, otherwise the behavior is
> +undefined.  A call to <function>PQescapeString</> writes an escaped
> +version of the <parameter>from</> string to the <parameter>to</>
> +buffer, replacing special characters so that they cannot cause any
> +harm, and adding a terminating NUL character.  The single quotes which
> +must surround PostgreSQL string literals are not part of the result
> +string.
> +</para>
> +<para>
> +<function>PQescapeString</> returns the number of characters written
> +to <parameter>to</>, not including the terminating NUL character.
> +Behavior is undefined when the <parameter>to</> and <parameter>from</>
> +strings overlap.
> +</para>
> +
>  <sect2 id="libpq-exec-select-info">
>    <title>Retrieving SELECT Result Information</title>
>  
> Index: src/interfaces/libpq/fe-exec.c
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
> retrieving revision 1.108
> diff -u -r1.108 fe-exec.c
> --- src/interfaces/libpq/fe-exec.c    2001/08/21 20:39:53    1.108
> +++ src/interfaces/libpq/fe-exec.c    2001/09/04 18:32:09
> @@ -56,6 +56,62 @@
>  static int    getNotify(PGconn *conn);
>  static int    getNotice(PGconn *conn);
>  
> +/* ---------------
> + * Escaping arbitrary strings to get valid SQL strings/identifiers.
> + *
> + * Replaces "\\" with "\\\\", "\0" with "\\0", and "'" with "''".
> + * length is the length of the buffer pointed to by
> + * from.  The buffer at to must be at least 2*length + 1 characters
> + * long.  A terminating NUL character is written.
> + * ---------------
> + */
> +
> +size_t
> +PQescapeString (char *to, const char *from, size_t length)
> +{
> +    const char *source = from;
> +    char *target = to;
> +    unsigned int remaining = length;
> +
> +    while (remaining > 0) {
> +        switch (*source) {
> +        case '\0':
> +            *target = '\\';
> +            target++;
> +            *target = '0';
> +            /* target and remaining are updated below. */
> +            break;
> +            
> +        case '\\':
> +            *target = '\\';
> +            target++;
> +            *target = '\\';
> +            /* target and remaining are updated below. */
> +            break;
> +
> +        case '\'':
> +            *target = '\'';
> +            target++;
> +            *target = '\'';
> +            /* target and remaining are updated below. */
> +            break;
> +
> +        default:
> +            *target = *source;
> +            /* target and remaining are updated below. */
> +        }
> +        source++;
> +        target++;
> +        remaining--;
> +    }
> +
> +    /* Write the terminating NUL character. */
> +    *target = '\0';
> +    
> +    return target - to;
> +}
> +
> +
>  
>  /* ----------------
>   * Space management for PGresult.
> Index: src/interfaces/libpq/libpq-fe.h
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
> retrieving revision 1.72
> diff -u -r1.72 libpq-fe.h
> --- src/interfaces/libpq/libpq-fe.h    2001/08/21 20:39:54    1.72
> +++ src/interfaces/libpq/libpq-fe.h    2001/09/04 18:32:09
> @@ -251,6 +251,9 @@
>  
>  /* === in fe-exec.c === */
>  
> +    /* Quoting strings before inclusion in queries. */
> +    extern size_t PQescapeString (char *to, const char *from, size_t length);
> +
>      /* Simple synchronous query */
>      extern PGresult *PQexec(PGconn *conn, const char *query);
>      extern PGnotify *PQnotifies(PGconn *conn);
> 

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Patch applied.  Thanks.

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> 
> > Patch removed at the request of the author.  Author will resubmit.
> 
> I've attached the fixed version of the patch below.  After the
> discussion on pgsql-hackers (especially the frightening memory dump in
> <12273.999562219@sss.pgh.pa.us>), we decided that it is best not to
> use identifiers from an untrusted source at all.  Therefore, all
> claims of the suitability of PQescapeString() for identifiers have
> been removed.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
> 

> Index: doc/src/sgml/libpq.sgml
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
> retrieving revision 1.68
> diff -u -r1.68 libpq.sgml
> --- doc/src/sgml/libpq.sgml    2001/09/04 00:18:18    1.68
> +++ doc/src/sgml/libpq.sgml    2001/09/04 18:32:05
> @@ -827,6 +827,42 @@
>  </itemizedlist>
>  </sect2>
>  
> +<sect2 id="libpq-exec-escape-string">
> +  <title>Escaping strings for inclusion in SQL queries</title>
> +<para>
> +<function>PQescapeString</function>
> +          Escapes a string for use within an SQL query.
> +<synopsis>
> +size_t PQescapeString (char *to, const char *from, size_t length);
> +</synopsis>
> +If you want to include strings which have been received
> +from a source which is not trustworthy (for example, because they were
> +transmitted across a network), you cannot directly include them in SQL
> +queries for security reasons.  Instead, you have to quote special
> +characters which are otherwise interpreted by the SQL parser.
> +</para>
> +<para>
> +<function>PQescapeString</> performs this operation.  The
> +<parameter>from</> points to the first character of the string which
> +is to be escaped, and the <parameter>length</> parameter counts the
> +number of characters in this string (a terminating NUL character is
> +neither necessary nor counted).  <parameter>to</> shall point to a
> +buffer which is able to hold at least one more character than twice
> +the value of <parameter>length</>, otherwise the behavior is
> +undefined.  A call to <function>PQescapeString</> writes an escaped
> +version of the <parameter>from</> string to the <parameter>to</>
> +buffer, replacing special characters so that they cannot cause any
> +harm, and adding a terminating NUL character.  The single quotes which
> +must surround PostgreSQL string literals are not part of the result
> +string.
> +</para>
> +<para>
> +<function>PQescapeString</> returns the number of characters written
> +to <parameter>to</>, not including the terminating NUL character.
> +Behavior is undefined when the <parameter>to</> and <parameter>from</>
> +strings overlap.
> +</para>
> +
>  <sect2 id="libpq-exec-select-info">
>    <title>Retrieving SELECT Result Information</title>
>  
> Index: src/interfaces/libpq/fe-exec.c
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
> retrieving revision 1.108
> diff -u -r1.108 fe-exec.c
> --- src/interfaces/libpq/fe-exec.c    2001/08/21 20:39:53    1.108
> +++ src/interfaces/libpq/fe-exec.c    2001/09/04 18:32:09
> @@ -56,6 +56,62 @@
>  static int    getNotify(PGconn *conn);
>  static int    getNotice(PGconn *conn);
>  
> +/* ---------------
> + * Escaping arbitrary strings to get valid SQL strings/identifiers.
> + *
> + * Replaces "\\" with "\\\\", "\0" with "\\0", and "'" with "''".
> + * length is the length of the buffer pointed to by
> + * from.  The buffer at to must be at least 2*length + 1 characters
> + * long.  A terminating NUL character is written.
> + * ---------------
> + */
> +
> +size_t
> +PQescapeString (char *to, const char *from, size_t length)
> +{
> +    const char *source = from;
> +    char *target = to;
> +    unsigned int remaining = length;
> +
> +    while (remaining > 0) {
> +        switch (*source) {
> +        case '\0':
> +            *target = '\\';
> +            target++;
> +            *target = '0';
> +            /* target and remaining are updated below. */
> +            break;
> +            
> +        case '\\':
> +            *target = '\\';
> +            target++;
> +            *target = '\\';
> +            /* target and remaining are updated below. */
> +            break;
> +
> +        case '\'':
> +            *target = '\'';
> +            target++;
> +            *target = '\'';
> +            /* target and remaining are updated below. */
> +            break;
> +
> +        default:
> +            *target = *source;
> +            /* target and remaining are updated below. */
> +        }
> +        source++;
> +        target++;
> +        remaining--;
> +    }
> +
> +    /* Write the terminating NUL character. */
> +    *target = '\0';
> +    
> +    return target - to;
> +}
> +
> +
>  
>  /* ----------------
>   * Space management for PGresult.
> Index: src/interfaces/libpq/libpq-fe.h
> ===================================================================
> RCS file: /home/projects/pgsql/cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
> retrieving revision 1.72
> diff -u -r1.72 libpq-fe.h
> --- src/interfaces/libpq/libpq-fe.h    2001/08/21 20:39:54    1.72
> +++ src/interfaces/libpq/libpq-fe.h    2001/09/04 18:32:09
> @@ -251,6 +251,9 @@
>  
>  /* === in fe-exec.c === */
>  
> +    /* Quoting strings before inclusion in queries. */
> +    extern size_t PQescapeString (char *to, const char *from, size_t length);
> +
>      /* Simple synchronous query */
>      extern PGresult *PQexec(PGconn *conn, const char *query);
>      extern PGnotify *PQnotifies(PGconn *conn);
> 

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

> Patch applied.  Thanks.
>
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >
> > > Patch removed at the request of the author.  Author will resubmit.
> >
> > I've attached the fixed version of the patch below.  After the
> > discussion on pgsql-hackers (especially the frightening memory dump in
> > <12273.999562219@sss.pgh.pa.us>), we decided that it is best not to
> > use identifiers from an untrusted source at all.  Therefore, all
> > claims of the suitability of PQescapeString() for identifiers have
> > been removed.

I found a problem with PQescapeString (I think). Since it escapes
null bytes to be literally '\0', the following can happen:
1. User inputs string value as "<null byte>##" where ## are digits in the
range of 0 to 7.
2. PQescapeString converts this to "\0##"
3. Escaped string is used in a context that causes "\0##" to be evaluated as
an octal escape sequence.

For example, if the user enters a null byte followed by "47", and escapes
it, it becomes "\071" which gets translated into a single digit "9" by the
general parser. Along the same lines, if there is a null byte in a string,
and it is not followed by digits, the resulting "\0" gets converted back
into a null byte by the parser and the string gets truncated.

If the goal is to "safely" encode null bytes, and preserve the rest of the
string as it was entered, I think the null bytes should be escaped as \\000
(note that if you simply use \000 the same string truncation problem
occurs).

-- Joe

"Joe Conway" <joseph.conway@home.com> writes:

> I found a problem with PQescapeString (I think). Since it escapes
> null bytes to be literally '\0', the following can happen:
> 1. User inputs string value as "<null byte>##" where ## are digits in the
> range of 0 to 7.
> 2. PQescapeString converts this to "\0##"
> 3. Escaped string is used in a context that causes "\0##" to be evaluated as
> an octal escape sequence.

I agree that this is a problem, though it is not possible to do
anything harmful with it.  In addition, it only occurs if there are
any NUL characters in its input, which is very unlikely if you are
using C strings.

The patch below addresses the issue by removing escaping of \0
characters entirely.

> If the goal is to "safely" encode null bytes, and preserve the rest of the
> string as it was entered, I think the null bytes should be escaped as \\000
> (note that if you simply use \000 the same string truncation problem
> occurs).

We can't do that, this would require 4n + 1 bytes of storage for the
result, breaking the interface.

--
Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
University of Stuttgart           http://cert.uni-stuttgart.de/
RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898

I think we need this patch.  Bytea encoding will be changed to accept
\000 rather than \0 for the same reason.  I also agree that the libpq
enescaping of a C string doesn't need to deal with NULL like bytea does.

Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

> "Joe Conway" <joseph.conway@home.com> writes:
> 
> > I found a problem with PQescapeString (I think). Since it escapes
> > null bytes to be literally '\0', the following can happen:
> > 1. User inputs string value as "<null byte>##" where ## are digits in the
> > range of 0 to 7.
> > 2. PQescapeString converts this to "\0##"
> > 3. Escaped string is used in a context that causes "\0##" to be evaluated as
> > an octal escape sequence.
> 
> I agree that this is a problem, though it is not possible to do
> anything harmful with it.  In addition, it only occurs if there are
> any NUL characters in its input, which is very unlikely if you are
> using C strings.
> 
> The patch below addresses the issue by removing escaping of \0
> characters entirely.
> 
> > If the goal is to "safely" encode null bytes, and preserve the rest of the
> > string as it was entered, I think the null bytes should be escaped as \\000
> > (note that if you simply use \000 the same string truncation problem
> > occurs).
> 
> We can't do that, this would require 4n + 1 bytes of storage for the
> result, breaking the interface.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
> 

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026
 

Patch applied.  Thanks.

> "Joe Conway" <joseph.conway@home.com> writes:
> 
> > I found a problem with PQescapeString (I think). Since it escapes
> > null bytes to be literally '\0', the following can happen:
> > 1. User inputs string value as "<null byte>##" where ## are digits in the
> > range of 0 to 7.
> > 2. PQescapeString converts this to "\0##"
> > 3. Escaped string is used in a context that causes "\0##" to be evaluated as
> > an octal escape sequence.
> 
> I agree that this is a problem, though it is not possible to do
> anything harmful with it.  In addition, it only occurs if there are
> any NUL characters in its input, which is very unlikely if you are
> using C strings.
> 
> The patch below addresses the issue by removing escaping of \0
> characters entirely.
> 
> > If the goal is to "safely" encode null bytes, and preserve the rest of the
> > string as it was entered, I think the null bytes should be escaped as \\000
> > (note that if you simply use \000 the same string truncation problem
> > occurs).
> 
> We can't do that, this would require 4n + 1 bytes of storage for the
> result, breaking the interface.
> 
> -- 
> Florian Weimer                       Florian.Weimer@RUS.Uni-Stuttgart.DE
> University of Stuttgart           http://cert.uni-stuttgart.de/
> RUS-CERT                          +49-711-685-5973/fax +49-711-685-5898
> 

[ Attachment, skipping... ]

> 
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026