Debian Bug report logs - #369351
exim4-daemon-heavy: Insecure quote escaping in PostgreSQL backend

[Message part 1 (text/plain, inline)]
Package: exim4-daemon-heavy
Severity: important
Version: 4.60-3
Tags: security

Hi!

Recently, a security hole has been discovered in PostgreSQL client
applications, see http://www.postgresql.org/docs/techdocs.50 for
details. In short, using \' for quote escaping is insecure and now not
allowed any more in some encodings which are prone to this SQL
injection attack. This has been assigned CVE-2006-2314.

./src/lookups/pgsql.c, pgsql_quote() currently uses \' to
escape quoting, which makes it vulnerable against this attack with
earlier PostgreSQL versions, and will break with the current one
(since it disables this method of quote escaping by default in
affected client encodings). A quick fix is to change the function to
use '' instead of \', but a better fix is to completely replace the
loop with an invocation of PQescapeString() from libpq. 

Please be aware that this also affects other database backends in
principle (unless they do not support the affected encodings). Also,
'' is the SQL standard escape for ', not \'.

Please also pass this to upstream.

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

In a world without walls and fences, who needs Windows and Gates?

[signature.asc (application/pgp-signature, inline)]
tags #369351 upstream
forwarded #369351 http://www.exim.org/bugzilla/show_bug.cgi?id=107
thanks

On Mon, May 29, 2006 at 12:11:57PM +0200, Martin Pitt wrote:
> Please also pass this to upstream.

Done.

Greetings
Marc

-- 
-----------------------------------------------------------------------------
Marc Haber         | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany  |  lose things."    Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature |  How to make an American Quilt | Fax: *49 621 72739835

* Martin Pitt:

> ./src/lookups/pgsql.c, pgsql_quote() currently uses \' to
> escape quoting, which makes it vulnerable against this attack with
> earlier PostgreSQL versions, and will break with the current one
> (since it disables this method of quote escaping by default in
> affected client encodings). A quick fix is to change the function to
> use '' instead of \', but a better fix is to completely replace the
> loop with an invocation of PQescapeString() from libpq. 

PQescapeString is deprecated because given its interface, the security
bug cannot be closed completely.  You really should use
PQescapeStringConn.

Would you add this information to the other bug reports, too?

[Message part 1 (text/plain, inline)]
Hi Florian,

Florian Weimer [2006-05-29 20:49 +0200]:
> * Martin Pitt:
> 
> > ./src/lookups/pgsql.c, pgsql_quote() currently uses \' to
> > escape quoting, which makes it vulnerable against this attack with
> > earlier PostgreSQL versions, and will break with the current one
> > (since it disables this method of quote escaping by default in
> > affected client encodings). A quick fix is to change the function to
> > use '' instead of \', but a better fix is to completely replace the
> > loop with an invocation of PQescapeString() from libpq. 
> 
> PQescapeString is deprecated because given its interface, the security
> bug cannot be closed completely.  You really should use
> PQescapeStringConn.

Thanks for the reminder, sorry that I forgot that. However, this is
just necessary if the application uses several postmaster connections
concurrently. With a single connection (which should be the usual
case) PQescapeString() and PQescapeBytea() will do the right thing.

> Would you add this information to the other bug reports, too?

Done.

Martin

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

In a world without walls and fences, who needs Windows and Gates?

[signature.asc (application/pgp-signature, inline)]
[Message part 1 (text/plain, inline)]
Hi,

http://patches.ubuntu.com/patches/exim4.sql_quote_escaping.diff is a
quick band aid patch (minimally intrusive) suitable for a sarge
security update. It also fixes the same issue for the mysql backend.
However, in Sid exim4 should still be changed to use
PQescapeStringConn() and mysql_real_escape().

Martin

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

In a world without walls and fences, who needs Windows and Gates?

[signature.asc (application/pgp-signature, inline)]
On 2006-06-02 Martin Pitt <martin.pitt@ubuntu.com> wrote:
> http://patches.ubuntu.com/patches/exim4.sql_quote_escaping.diff is a
> quick band aid patch (minimally intrusive) suitable for a sarge
> security update. It also fixes the same issue for the mysql backend.

Thanks a lot.

> However, in Sid exim4 should still be changed to use
> PQescapeStringConn() and mysql_real_escape().

PQescapeStringConn() is newly introduced in postgresql-8.1 8.1.4,
afaict from
http://packages.qa.debian.org/p/postgresql-8.1/news/20060602T042331Z.html
so I gather that switching to it would either be need to be done
conditionally (at build-time) or building against older versions would
be impossible.

Is this correct?

thanks, cu andreas
-- 
The 'Galactic Cleaning' policy undertaken by Emperor Zhark is a personal
vision of the emperor's, and its inclusion in this work does not constitute
tacit approval by the author or the publisher for any such projects,
howsoever undertaken.                                (c) Jasper Ffforde

[Message part 1 (text/plain, inline)]
Hi Andreas,

Andreas Metzler [2006-06-02 18:27 +0200]:
> 
> PQescapeStringConn() is newly introduced in postgresql-8.1 8.1.4,
> afaict from
> http://packages.qa.debian.org/p/postgresql-8.1/news/20060602T042331Z.html
> so I gather that switching to it would either be need to be done
> conditionally (at build-time) or building against older versions would
> be impossible.
> 
> Is this correct?

Correct. In 8.1.4-2 I even managed to bump the libpq4 shlibs for that.

My advice would be to use the autoconf magic and #defines and use
-Conn() if it's available, and just PQescapeString() if not.
Otherwise, if you are sure that exim only ever uses one connection at
a time, then PQescapeString() is completely safe, too.

Martin

-- 
Martin Pitt        http://www.piware.de
Ubuntu Developer   http://www.ubuntu.com
Debian Developer   http://www.debian.org

In a world without walls and fences, who needs Windows and Gates?

[signature.asc (application/pgp-signature, inline)]
* Martin Pitt:

> My advice would be to use the autoconf magic and #defines and use
> -Conn() if it's available, and just PQescapeString() if not.
> Otherwise, if you are sure that exim only ever uses one connection at
> a time, then PQescapeString() is completely safe, too.

It's not, no matter what Exim does.  Exim uses NSS, and there is an
NSS module for PostgreSQL which could clobber the internal global
variable.

On Mon, May 29, 2006 at 08:49:57PM +0200, Florian Weimer wrote:
> PQescapeString is deprecated because given its interface, the security
> bug cannot be closed completely.  You really should use
> PQescapeStringConn.

I have added this to the upstream bugzilla
(http://www.exim.org/bugzilla/show_bug.cgi?id=107) and hope that this
will give upstream a friendly nudge to act on the report.

Greetings
Marc

-- 
-----------------------------------------------------------------------------
Marc Haber         | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany  |  lose things."    Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature |  How to make an American Quilt | Fax: *49 621 72739835

Hi,

this is upstream's patch for this issue. Philip decided not to use the
libpq functions.

Martin, Florian, can you comment?

Greetings
Marc

----- Forwarded message from Philip Hazel <ph10@sesame.csx.cam.ac.uk> -----

> From: Philip Hazel <ph10@sesame.csx.cam.ac.uk>
> Subject: [exim-cvs] cvs commit: exim/exim-doc/doc-txt ChangeLog
>  exim/exim-src/src/lookups pgsql.c exim/exim-test/scripts/9200-PostgreSQL
>  9200 exim/exim-test/stderr 9200 exim/exim-test/stdout 9200
> To: exim-cvs@exim.org
> Reply-To: exim-dev@exim.org
> Date: Fri, 30 Jun 2006 14:57:46 +0100
> X-Spam-Score: (--) -2.8
> X-Spam-Report: torres.zugschlus.de
> 	Content analysis details:   (-2.8 points, 5.0 required)
> 	pts  rule name              description
> 	---- ---------------------- -------------------------------------------
> 	-2.6 BAYES_00               BODY: Bayesian spam probability is 0 to 1%
> 	[score: 0.0000]
> 	-0.2 AWL                    AWL: From: address is in the auto white-list
> 
> ph10        2006/06/30 14:57:46 BST
> 
>   Modified files:
>     exim-doc/doc-txt     ChangeLog 
>     exim-src/src/lookups pgsql.c 
>     exim-test/scripts/9200-PostgreSQL 9200 
>     exim-test/stderr     9200 
>     exim-test/stdout     9200 
>   Log:
>   Change ${quote_pgsql to quote ' as '' instead of \' because of a
>   security issue.
>   
>   Revision  Changes    Path
>   1.364     +5 -0      exim/exim-doc/doc-txt/ChangeLog
>   1.5       +16 -1     exim/exim-src/src/lookups/pgsql.c
>   1.2       +1 -0      exim/exim-test/scripts/9200-PostgreSQL/9200
>   1.3       +12 -1     exim/exim-test/stderr/9200
>   1.2       +1 -0      exim/exim-test/stdout/9200
>   
>   Index: ChangeLog
>   ===================================================================
>   RCS file: /home/cvs/exim/exim-doc/doc-txt/ChangeLog,v
>   retrieving revision 1.363
>   retrieving revision 1.364
>   diff -u -r1.363 -r1.364
>   --- ChangeLog	28 Jun 2006 16:00:23 -0000	1.363
>   +++ ChangeLog	30 Jun 2006 13:57:46 -0000	1.364
>   @@ -1,4 +1,4 @@
>   -$Cambridge: exim/exim-doc/doc-txt/ChangeLog,v 1.363 2006/06/28 16:00:23 ph10 Exp $
>   +$Cambridge: exim/exim-doc/doc-txt/ChangeLog,v 1.364 2006/06/30 13:57:46 ph10 Exp $
>    
>    Change log file for Exim from version 4.21
>    -------------------------------------------
>   @@ -55,6 +55,11 @@
>    PH/06 Added acl_not_smtp_start, based on Johannes Berg's patch, and set the
>          bit to forbid control=suppress_local_fixups in the acl_not_smtp ACL,
>          because it is too late at that time, and has no effect.
>   +
>   +PH/07 Changed ${quote_pgsql to quote ' as '' instead of \' because of a
>   +      security issue with \' (bugzilla #107). I could not use the
>   +      PQescapeStringConn() function, because it needs a PGconn value as one of
>   +      its arguments.
>    
>    
>    Exim version 4.62
>   
>   Index: pgsql.c
>   ===================================================================
>   RCS file: /home/cvs/exim/exim-src/src/lookups/pgsql.c,v
>   retrieving revision 1.4
>   retrieving revision 1.5
>   diff -u -r1.4 -r1.5
>   --- pgsql.c	7 Feb 2006 11:19:01 -0000	1.4
>   +++ pgsql.c	30 Jun 2006 13:57:46 -0000	1.5
>   @@ -1,4 +1,4 @@
>   -/* $Cambridge: exim/exim-src/src/lookups/pgsql.c,v 1.4 2006/02/07 11:19:01 ph10 Exp $ */
>   +/* $Cambridge: exim/exim-src/src/lookups/pgsql.c,v 1.5 2006/06/30 13:57:46 ph10 Exp $ */
>    
>    /*************************************************
>    *     Exim - an Internet mail transport agent    *
>   @@ -422,6 +422,16 @@
>    does treat the string as "ab%cd". So we can safely quote percent and
>    underscore. [This is different to MySQL, where you can't do this.]
>    
>   +The original code quoted single quotes as \' which is documented as valid in
>   +the O'Reilly book "Practical PostgreSQL" (first edition) as an alternative to
>   +the SQL standard '' way of representing a single quote as data. However, in
>   +June 2006 there was some security issue with using \' and so this has been
>   +changed.
>   +
>   +[Note: There is a function called PQescapeStringConn() that quotes strings.
>   +This cannot be used because it needs a PGconn argument (the connection handle).
>   +Why, I don't know. Seems odd for just string escaping...]
>   +
>    Arguments:
>      s          the string to be quoted
>      opt        additional option text or NULL if none
>   @@ -447,7 +457,12 @@
>    
>    while ((c = *s++) != 0)
>      {
>   -  if (Ustrchr("\n\t\r\b\'\"\\%_", c) != NULL)
>   +  if (c == '\'')
>   +    {
>   +    *t++ = '\'';
>   +    *t++ = '\'';
>   +    }
>   +  else if (Ustrchr("\n\t\r\b\"\\%_", c) != NULL)
>        {
>        *t++ = '\\';
>        switch(c)
>   
>   Index: 9200
>   ===================================================================
>   RCS file: /home/cvs/exim/exim-test/scripts/9200-PostgreSQL/9200,v
>   retrieving revision 1.1
>   retrieving revision 1.2
>   diff -u -r1.1 -r1.2
>   --- 9200	7 Feb 2006 10:54:51 -0000	1.1
>   +++ 9200	30 Jun 2006 13:57:46 -0000	1.2
>   @@ -9,6 +9,7 @@
>    ${lookup pgsql {select * from them where id='quote2';}}
>    ${lookup pgsql {select * from them where id='newline';}}
>    ${lookup pgsql {select * from them where id='tab';}}
>   +${lookup pgsql {select * from them where name='${quote_pgsql:'stquot}';}}
>    ****
>    exim -d -bh 10.0.0.0
>    mail from:<a@b>
>   
>   Index: 9200
>   ===================================================================
>   RCS file: /home/cvs/exim/exim-test/stderr/9200,v
>   retrieving revision 1.2
>   retrieving revision 1.3
>   diff -u -r1.2 -r1.3
>   --- 9200	18 Apr 2006 15:53:58 -0000	1.2
>   +++ 9200	30 Jun 2006 13:57:46 -0000	1.3
>   @@ -100,7 +100,18 @@
>    database lookup required for select * from them where id='tab';
>    PGSQL query: select * from them where id='tab';
>    PGSQL using cached connection for localhost/test/CALLER
>   -lookup yielded: name="x x" id=tab 
>   +lookup yielded: name="x	x" id=tab 
>   +search_open: pgsql "NULL"
>   +  cached open
>   +search_find: file="NULL"
>   +  key="select * from them where name='''stquot';" partial=-1 affix=NULL starflags=0
>   +LRU list:
>   +internal_search_find: file="NULL"
>   +  type=pgsql key="select * from them where name='''stquot';"
>   +database lookup required for select * from them where name='''stquot';
>   +PGSQL query: select * from them where name='''stquot';
>   +PGSQL using cached connection for localhost/test/CALLER
>   +lookup yielded: name='stquot id=quote1 
>    search_tidyup called
>    close PGSQL connection: localhost/test/CALLER
>    >>>>>>>>>>>>>>>> Exim pid=pppp terminating with rc=0 >>>>>>>>>>>>>>>>
>   
>   Index: 9200
>   ===================================================================
>   RCS file: /home/cvs/exim/exim-test/stdout/9200,v
>   retrieving revision 1.1
>   retrieving revision 1.2
>   diff -u -r1.1 -r1.2
>   --- 9200	7 Feb 2006 10:47:37 -0000	1.1
>   +++ 9200	30 Jun 2006 13:57:46 -0000	1.2
>   @@ -8,6 +8,7 @@
>    > name="before
>    after" id=newline 
>    > name="x	x" id=tab 
>   +> name='stquot id=quote1 
>    > 
>    
>    **** SMTP testing session as if from host 10.0.0.0
>   
> 
> _______________________________________________
> exim-cvs mailing list
> exim-cvs@exim.org
> http://www.exim.org/mailman/listinfo/exim-cvs

----- End forwarded message -----

-- 
-----------------------------------------------------------------------------
Marc Haber         | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany  |  lose things."    Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature |  How to make an American Quilt | Fax: *49 621 72739835

* Marc Haber:

>>   +The original code quoted single quotes as \' which is documented as valid in
>>   +the O'Reilly book "Practical PostgreSQL" (first edition) as an alternative to
>>   +the SQL standard '' way of representing a single quote as data. However, in
>>   +June 2006 there was some security issue with using \' and so this has been
>>   +changed.

This is still not correct.  You need to deal with multi-byte character
encodings while quoting, otherwise you still suffer from the
vulnerability for certain encodings.

>>   +[Note: There is a function called PQescapeStringConn() that quotes strings.
>>   +This cannot be used because it needs a PGconn argument (the connection handle).
>>   +Why, I don't know. Seems odd for just string escaping...]

PQescapeStringConn uses the connection handle to determine the
encoding of the passed string.  If you can't supply the handle,
PQescapeString is the better choice, but it relies on an internal
global variable.

I'm going to have a look at how Exim deals with SQL backends.  Perhaps
there is a reasonably portable way to do away with all that quoting.

package exim4-daemon-heavy
tags #369351 - fixed-upstream
user bts-link-upstream@lists.alioth.debian.org
usertags 369351 - status-RESOLVED resolution-FIXED
usertags 369351 + status-REOPENED
thanks

On Fri, Jun 30, 2006 at 06:18:37PM +0200, Florian Weimer wrote:
> * Marc Haber:
> 
> >>   +The original code quoted single quotes as \' which is documented as valid in
> >>   +the O'Reilly book "Practical PostgreSQL" (first edition) as an alternative to
> >>   +the SQL standard '' way of representing a single quote as data. However, in
> >>   +June 2006 there was some security issue with using \' and so this has been
> >>   +changed.
> 
> This is still not correct.  You need to deal with multi-byte character
> encodings while quoting, otherwise you still suffer from the
> vulnerability for certain encodings.
> 
> >>   +[Note: There is a function called PQescapeStringConn() that quotes strings.
> >>   +This cannot be used because it needs a PGconn argument (the connection handle).
> >>   +Why, I don't know. Seems odd for just string escaping...]
> 
> PQescapeStringConn uses the connection handle to determine the
> encoding of the passed string.  If you can't supply the handle,
> PQescapeString is the better choice, but it relies on an internal
> global variable.
> 
> I'm going to have a look at how Exim deals with SQL backends.  Perhaps
> there is a reasonably portable way to do away with all that quoting.

I have forwarded this to the exim bugzilla bug. 

-- 
-----------------------------------------------------------------------------
Marc Haber         | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany  |  lose things."    Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature |  How to make an American Quilt | Fax: *49 621 72739835

tags #369351 help
thanks

On Mon, May 29, 2006 at 08:49:57PM +0200, Florian Weimer wrote:
> * Martin Pitt:
> > ./src/lookups/pgsql.c, pgsql_quote() currently uses \' to
> > escape quoting, which makes it vulnerable against this attack with
> > earlier PostgreSQL versions, and will break with the current one
> > (since it disables this method of quote escaping by default in
> > affected client encodings). A quick fix is to change the function to
> > use '' instead of \', but a better fix is to completely replace the
> > loop with an invocation of PQescapeString() from libpq. 
> 
> PQescapeString is deprecated because given its interface, the security
> bug cannot be closed completely.  You really should use
> PQescapeStringConn.
> 
> Would you add this information to the other bug reports, too?

We need help to have this solved upsteam. See
http://www.exim.org/bugzilla/show_bug.cgi?id=107

To me, it looks like the issue is that PQescapeStringConn needs an
established connection to the database daemon, while exim needs the
escape function well before it actually talks to the database due to
its design.

The PostgreSQL code for exim was contributed by a third party and
upstream doesn't exactly know how to solve the issue at hand.

If anybody having PostgreSQL programming experience, please give help
either here or in upsteam's bugzilla.

Any help will be appreciated.

Greetings
Marc

-- 
-----------------------------------------------------------------------------
Marc Haber         | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany  |  lose things."    Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature |  How to make an American Quilt | Fax: *49 621 72739835

Package: exim4-daemon-heavy

I'm commenting on this after a post on postgresql-devel. There's seems
to be some confusion here about the real issue, which is as follows:

  You cannot perform quoting safely unless you know the encoding of the
  string you're quoting.

And in the specific case of exim, the issue is that you don't know what
encoding you're using because the lookup code never checks. That's why
the "escape string" function needs a connection, so it can check the
encoding and perform the quoting safely.

I've looked at the spec quickly but see no indication of what encoding
is expected of the configuration file, nor of the query results. This
seems somewhat of an omission, since some encodings use the ':'
character as part of multibyte characters, and Exim splits on that
quite often.

PostgreSQL can accept a large number of encodings, including utf8 and
the latin series. Once you have decided what encoding you're expecting
for input/output, use PQsetClientEncoding() to inform the database of
your decision, and perform the quoting yourself for the encoding you
choose. Problem solved.

Hope this helps,
-- 
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Version: 4.63-1

On Tue, Aug 01, 2006 at 04:32:26AM -0700, Marc Haber wrote:
> tag 369351 + fixed-in-experimental
> tag 378131 + fixed-in-experimental
> tag 379155 + fixed-in-experimental
> 
> quit
> 
> This message was generated automatically in response to an
> upload to the experimental distribution.  The .changes file follows.

Greetings
Marc

-- 
-----------------------------------------------------------------------------
Marc Haber         | "I don't trust Computers. They | Mailadresse im Header
Mannheim, Germany  |  lose things."    Winona Ryder | Fon: *49 621 72739834
Nordisch by Nature |  How to make an American Quilt | Fax: *49 621 72739835

Debian Bug report logs - #369351 exim4-daemon-heavy: Insecure quote escaping in PostgreSQL backend

Debian Bug report logs - #369351
exim4-daemon-heavy: Insecure quote escaping in PostgreSQL backend