-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

This is a resend of my previous email which was stucked at moderation
approval.. and as I don't know if anyone actually does that in your
list, I'm resending this now.

Hi,

I'm very new to this project and inspired by recent security release, I
started to audit postgresql source against common mistakes with sprintf().

I mostly found problems with sprintf() used on statically allocated
buffers or dynamically allocated buffers with random constant size.

I used lib/stringinfo.h functions when I was sure palloc()-memory
allocation was the right thing to do and I felt like code needed to
construct a complete string no matter how complex.

There were places where I just changes sprintf() to snprintf(). Like in
some *BSD dl loading functions etc.

There were also places where I could identify the possible bug but
didn't know 'the' right way to fix it. As I say, I don't know the
codebase very well so I really didn't know what auxiliarity functions
there are to use. These parts are marked as FIXME and should be easily
identified by looking at the patch (link below - it is a big one).

There were also simple mistakes like in src/backend/tioga/tgRecipe.c
- -	sprintf(qbuf, Q_LOOKUP_EDGES_IN_RECIPE, name);
- -	pqres = PQexec(qbuf);
+	snprintf(qbuf, MAX_QBUF_LENGTH, Q_LOOKUP_EDGES_IN_RECIPE, name);
~ 	pqres = PQexec(qbuf);
~ 	if (*pqres == 'R' || *pqres == 'E')

Notice how previous PQexec() is removed. There were two of them.

Some of my fixes cause code to be a bit slower because of dynamically
allocated mem, but it also fixes a lot of ptr+strlen(ptr) -style
performance problems. I didn't particularly try to fix these but some of
them are corrected by simply using lib/stringinfo.h

Please take look at this patch but since I have worked three long nights
with this one, there probably are bugs. I tried compiling it with
"configure --with-tcl --with-perl --with-python" and at least it
compiled for me :) But that's about all I can promise.

diffstat postgresql-7.2.2-sprintf.patch
~ contrib/cube/cube.c                             |   26 --
~ contrib/cube/cubeparse.y                        |   11
~ contrib/intarray/_int.c                         |   29 +-
~ contrib/rserv/rserv.c                           |   30 +-
~ contrib/seg/segparse.y                          |   18 -
~ contrib/spi/refint.c                            |   39 +--
~ contrib/spi/timetravel.c                        |   12
~ doc/src/sgml/spi.sgml                           |    2
~ src/backend/parser/analyze.c                    |    2
~ src/backend/port/dynloader/freebsd.c            |   10
~ src/backend/port/dynloader/netbsd.c             |   11
~ src/backend/port/dynloader/nextstep.c           |    2
~ src/backend/port/dynloader/openbsd.c            |   10
~ src/backend/postmaster/postmaster.c             |    2
~ src/backend/storage/file/fd.c                   |    1
~ src/backend/storage/ipc/shmqueue.c              |    1
~ src/backend/tioga/tgRecipe.c                    |   11
~ src/backend/utils/adt/ri_triggers.c             |  312
++++++++++++------------
~ src/bin/pg_dump/pg_dump.c                       |   14 -
~ src/bin/pg_passwd/pg_passwd.c                   |    2
~ src/bin/psql/command.c                          |    2
~ src/bin/psql/describe.c                         |    3
~ src/interfaces/ecpg/preproc/pgc.l               |    8
~ src/interfaces/ecpg/preproc/preproc.y           |   24 -
~ src/interfaces/ecpg/preproc/type.c              |   16 -
~ src/interfaces/ecpg/preproc/variable.c          |   12
~ src/interfaces/libpgeasy/examples/pgwordcount.c |    6
~ src/interfaces/libpgtcl/pgtclCmds.c             |    4
~ src/interfaces/libpq/fe-auth.c                  |    2
~ src/interfaces/odbc/connection.c                |    2
~ src/interfaces/odbc/dlg_specific.c              |    5
~ src/interfaces/odbc/info.c                      |   38 +-
~ src/interfaces/odbc/qresult.c                   |    4
~ src/interfaces/odbc/results.c                   |    8
~ src/interfaces/odbc/statement.c                 |    6
~ 35 files changed, 365 insertions, 320 deletions

Patch is about 70k and downloadable from
http://suihkari.baana.suomi.net/postgresql/patches/postgresql-7.2.2-sprintf.patch

At least I didn't just bitch and moan about the bugs. ;)

- - Jukka
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9bRlYYYWM2XTSwX0RAndJAJ9C8KDGjteQ2Edngwifb6C876KDsgCfUon6
PObTTeQfDLmgxkKN7bPnyk4=
=nFa0
-----END PGP SIGNATURE-----


Wed, 28 Aug 2002 21:41:28 +0300
Jukka Holappa <jukkaho@mail.student.oulu.fi>
[Resend] Sprintf() auditing and a patch

I have reviewed your patch, and it is a thorough job.  Unfortunately,
our code has drifted dramatically since 7.2 in the areas you patched. 
Would you be able to download our CVS or current snapshot and submit a
patch based on that code?

In fact, we have applied a batch of snprintf fixes already so some of
them may already be fixed.  You found quite a few so you probably have
some fixes we don't have.

---------------------------------------------------------------------------

Jukka Holappa wrote:
[ PGP not available, raw data follows ]
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> This is a resend of my previous email which was stucked at moderation
> approval.. and as I don't know if anyone actually does that in your
> list, I'm resending this now.
> 
> Hi,
> 
> I'm very new to this project and inspired by recent security release, I
> started to audit postgresql source against common mistakes with sprintf().
> 
> I mostly found problems with sprintf() used on statically allocated
> buffers or dynamically allocated buffers with random constant size.
> 
> I used lib/stringinfo.h functions when I was sure palloc()-memory
> allocation was the right thing to do and I felt like code needed to
> construct a complete string no matter how complex.
> 
> There were places where I just changes sprintf() to snprintf(). Like in
> some *BSD dl loading functions etc.
> 
> There were also places where I could identify the possible bug but
> didn't know 'the' right way to fix it. As I say, I don't know the
> codebase very well so I really didn't know what auxiliarity functions
> there are to use. These parts are marked as FIXME and should be easily
> identified by looking at the patch (link below - it is a big one).
> 
> There were also simple mistakes like in src/backend/tioga/tgRecipe.c
> - -	sprintf(qbuf, Q_LOOKUP_EDGES_IN_RECIPE, name);
> - -	pqres = PQexec(qbuf);
> +	snprintf(qbuf, MAX_QBUF_LENGTH, Q_LOOKUP_EDGES_IN_RECIPE, name);
> ~ 	pqres = PQexec(qbuf);
> ~ 	if (*pqres == 'R' || *pqres == 'E')
> 
> Notice how previous PQexec() is removed. There were two of them.
> 
> Some of my fixes cause code to be a bit slower because of dynamically
> allocated mem, but it also fixes a lot of ptr+strlen(ptr) -style
> performance problems. I didn't particularly try to fix these but some of
> them are corrected by simply using lib/stringinfo.h
> 
> Please take look at this patch but since I have worked three long nights
> with this one, there probably are bugs. I tried compiling it with
> "configure --with-tcl --with-perl --with-python" and at least it
> compiled for me :) But that's about all I can promise.
> 
> diffstat postgresql-7.2.2-sprintf.patch
> ~ contrib/cube/cube.c                             |   26 --
> ~ contrib/cube/cubeparse.y                        |   11
> ~ contrib/intarray/_int.c                         |   29 +-
> ~ contrib/rserv/rserv.c                           |   30 +-
> ~ contrib/seg/segparse.y                          |   18 -
> ~ contrib/spi/refint.c                            |   39 +--
> ~ contrib/spi/timetravel.c                        |   12
> ~ doc/src/sgml/spi.sgml                           |    2
> ~ src/backend/parser/analyze.c                    |    2
> ~ src/backend/port/dynloader/freebsd.c            |   10
> ~ src/backend/port/dynloader/netbsd.c             |   11
> ~ src/backend/port/dynloader/nextstep.c           |    2
> ~ src/backend/port/dynloader/openbsd.c            |   10
> ~ src/backend/postmaster/postmaster.c             |    2
> ~ src/backend/storage/file/fd.c                   |    1
> ~ src/backend/storage/ipc/shmqueue.c              |    1
> ~ src/backend/tioga/tgRecipe.c                    |   11
> ~ src/backend/utils/adt/ri_triggers.c             |  312
> ++++++++++++------------
> ~ src/bin/pg_dump/pg_dump.c                       |   14 -
> ~ src/bin/pg_passwd/pg_passwd.c                   |    2
> ~ src/bin/psql/command.c                          |    2
> ~ src/bin/psql/describe.c                         |    3
> ~ src/interfaces/ecpg/preproc/pgc.l               |    8
> ~ src/interfaces/ecpg/preproc/preproc.y           |   24 -
> ~ src/interfaces/ecpg/preproc/type.c              |   16 -
> ~ src/interfaces/ecpg/preproc/variable.c          |   12
> ~ src/interfaces/libpgeasy/examples/pgwordcount.c |    6
> ~ src/interfaces/libpgtcl/pgtclCmds.c             |    4
> ~ src/interfaces/libpq/fe-auth.c                  |    2
> ~ src/interfaces/odbc/connection.c                |    2
> ~ src/interfaces/odbc/dlg_specific.c              |    5
> ~ src/interfaces/odbc/info.c                      |   38 +-
> ~ src/interfaces/odbc/qresult.c                   |    4
> ~ src/interfaces/odbc/results.c                   |    8
> ~ src/interfaces/odbc/statement.c                 |    6
> ~ 35 files changed, 365 insertions, 320 deletions
> 
> Patch is about 70k and downloadable from
> http://suihkari.baana.suomi.net/postgresql/patches/postgresql-7.2.2-sprintf.patch
> 
> At least I didn't just bitch and moan about the bugs. ;)
> 
> - - Jukka
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.0.7 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
> 
> iD8DBQE9bRlYYYWM2XTSwX0RAndJAJ9C8KDGjteQ2Edngwifb6C876KDsgCfUon6
> PObTTeQfDLmgxkKN7bPnyk4=
> =nFa0
> -----END PGP SIGNATURE-----
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster
> 
[ End of raw data]

-- 
  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, Pennsylvania 19073

Wed, 28 Aug 2002 17:32:19 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [Resend] Sprintf() auditing and a patch
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Bruce Momjian wrote:
| I have reviewed your patch, and it is a thorough job.  Unfortunately,
| our code has drifted dramatically since 7.2 in the areas you patched.
| Would you be able to download our CVS or current snapshot and submit a
| patch based on that code?
|
| In fact, we have applied a batch of snprintf fixes already so some of
| them may already be fixed.  You found quite a few so you probably have
| some fixes we don't have.

Sure, I take a look at CVS.

- - Jukka
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9bUWuYYWM2XTSwX0RAlQqAJwMNlwOJLYFnOb3xqUHE/BRZYRVPwCdGu4o
XJl98uXJlg5ZLhNlfX04pow=
=cdmM
-----END PGP SIGNATURE-----


Thu, 29 Aug 2002 00:50:38 +0300
Jukka Holappa <jukkaho@mail.student.oulu.fi>
Re: [Resend] Sprintf() auditing and a patch
[ Sorry, never saw the original email ]

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Jukka Holappa wrote:
> > I'm very new to this project and inspired by recent security
> > release, I started to audit postgresql source against common
> > mistakes with sprintf().

If you're interested, another common source of problems is integer
overflow when dealing with numeric input from the user. In fact, far
more security problems have been caused by insufficient integer
overflow checking than by string handling bugs.

FYI, we prefer patches in context diff format (diff -c). Also, there
are some code style rules that most of the backend code follows. For
example,

for (i = 0; i < x; i++) { ....

rather than:

for(i=0;i<x;++i) {

And indented using tabs. In any case, these should be automatically
corrected by Bruce before a release is made, but it would be nice if
patches followed this style.

> > There were also simple mistakes like in
> > src/backend/tioga/tgRecipe.c

That code is long dead, BTW.

> > Some of my fixes cause code to be a bit slower because of
> > dynamically allocated mem

Given that you're not using StringInfo in any performance-critical
areas AFAICT (mostly in contrib/, for example), I would suspect the
performance difference wouldn't be too steep (although it's worth
verifying that before the patch is applied). I briefly benchmarked
snprintf() versus sprintf() a couple days ago and found no performance
difference, but using StringInfo may impose a higher penalty.

I'd agree that StringInfo is appropriate when the string is frequently
being appended to (and the code using the strlen() pointer arithmetic
technique you mentioned); however, you've converted the code to use
StringInfo on situations in which it is clearly not warranted. To pick
one example at random, seg_atof(char *) in contrib/seg/segparse.y
doesn't require anything more than a statically sized buffer and
snprintf().

Also, that routine happens to leak memory, since you forgot to call
pfree(buf.data) -- I believe you made the same mistake in several
other places, such as seg_yyerror(char *) in the same file.

Personally, I prefer this:

        char *buf[1024];

        snprintf(buf, sizeof(buf), "...");

rather than this:

        char *buf[1024];

        snprintf(buf, 1024, "...");

(even if the size of the char array is a preprocessor constant).

The reason being that

        (a) it is more clear: the code plainly states "write to this
            string, up to the declared size of the string but no
            more".

        (b) it is more maintainable: if someone were to change the
            size of the char array to, say, 512 bytes but didn't
            change the snprintf(), you'd have a potential bug.

You used sizeof(...) in some places but not in others.

That's all I noticed briefly eye-balling the patch; please re-diff
against CVS HEAD and submit a context diff and I'll take another look.

> > Please take look at this patch but since I have worked three long
> > nights with this one, there probably are bugs. I tried compiling
> > it with "configure --with-tcl --with-perl --with-python" and at
> > least it compiled for me :) But that's about all I can promise.

FYI, running the regression tests is an easy way to do some basic
testing. Since code that causes regression tests to fail won't be
accepted (period), you may as well run them now, rather now later.

> > At least I didn't just bitch and moan about the bugs. ;)

Thank you; frankly, I wish your attitude was more common.

Cheers,

Neil

-- 
Neil Conway <neilc@samurai.com> || PGP Key ID: DB3C29FC


28 Aug 2002 23:49:19 -0400
Neil Conway <neilc@samurai.com>
Re: [Resend] Sprintf() auditing and a patch
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Neil Conway wrote:
| [ Sorry, never saw the original email ]

Because it is still hanging in moderation queue ;)

| FYI, we prefer patches in context diff format (diff -c). Also, there
| are some code style rules that most of the backend code follows. For
| example,

I tried to use the same style that was used in the code previously.
Apparently I forgot it in some places.

|
|>>There were also simple mistakes like in
|>>src/backend/tioga/tgRecipe.c
|>
|
| That code is long dead, BTW.

Well, we'll se what I can dig out of CVS version :) I think string
handling can be very nasty in some places but the problems are so much
easier to find than with integer overflows.

| I'd agree that StringInfo is appropriate when the string is frequently
| being appended to (and the code using the strlen() pointer arithmetic
| technique you mentioned); however, you've converted the code to use
| StringInfo on situations in which it is clearly not warranted. To pick
| one example at random, seg_atof(char *) in contrib/seg/segparse.y
| doesn't require anything more than a statically sized buffer and
| snprintf().

I'm sure I did that, because I really didn't know in all places, what
would be the right thing to do.

Using snprintf() there would cause a log message of "using numeric value
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx..." when trying to overflow this. I
agree, being told number to be unpresentable (coming after errorious
string) is not actually necessary when seeing this :)

|
| Also, that routine happens to leak memory, since you forgot to call
| pfree(buf.data) -- I believe you made the same mistake in several
| other places, such as seg_yyerror(char *) in the same file.

I checked and this is true. However code leaks already in the same
place. (although less bytes).

|
| Personally, I prefer this:
|
|         char *buf[1024];

You don't prefer an array of pointers, but I got the point.

|
|         snprintf(buf, sizeof(buf), "...");
|
| rather than this:
|
|         char *buf[1024];
|
|         snprintf(buf, 1024, "...");
[snip]
| You used sizeof(...) in some places but not in others.

Very true. I did all my checking and fixing in three nights and didn't
think about the maintainability at first but started using
sizeof(later). I just wanted to get them fixed at first. These should
all be using sizeof(buf) when the target is an array.

There were also places where a simple pointer to a buffer was passed to
another function which then appended some string to it. I think this was
date/time handling in somewhere. That kind of things are impossible to
fix (without changing the function definition) if the appended string
doesn't have a certain maximum size. Dates/times sure have that limit,
but I hope no one copies that code to handle any some variable length
strings..

| FYI, running the regression tests is an easy way to do some basic
| testing. Since code that causes regression tests to fail won't be
| accepted (period), you may as well run them now, rather now later.

All true.:)

- - Jukka
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQE9ba3nYYWM2XTSwX0RAoBJAJwK4eA5iPDNaQFF3TCL09MD/dkBwgCdHGmi
b4RCkBnOPBfQMQAX7wJk4U4=
=7hvG
-----END PGP SIGNATURE-----


Thu, 29 Aug 2002 08:15:20 +0300
Jukka Holappa <jukkaho@mail.student.oulu.fi>
Re: [Resend] Sprintf() auditing and a patch
Neil Conway wrote:
> If you're interested, another common source of problems is integer
> overflow when dealing with numeric input from the user. In fact, far
> more security problems have been caused by insufficient integer
> overflow checking than by string handling bugs.

One other things that bothers me are cases where we allocate memory to
hold the ASCII representation of an integer, but instead of using a
macro that documents this fact, we use a constant, and different
constants in different places.  That should be cleaned up.

-- 
  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, Pennsylvania 19073

Thu, 29 Aug 2002 14:43:28 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [Resend] Sprintf() auditing and a patch