Thread: [Resend] Sprintf() auditing and a patch
-----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-----
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
-----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-----
[ 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
stringbut no more".
(b) it is more maintainable: if someone were to change the size of the char array to, say, 512 bytes
butdidn'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
-----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-----
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, Pennsylvania19073