Re: [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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073