Re: [Resend] Sprintf() auditing and a patch - Mailing list pgsql-hackers

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,
Pennsylvania19073