Обсуждение: Interesting message about printf()'s in PostgreSQL
Hi everyone, Whilst looking around for some more PostgreSQL related stuff, this message turned up: http://mail.wirex.com/pipermail/sardonix/2002-February/000051.html The interesting bit is in an email messages included about halfway down. It speaks of Bad Things in the PostgreSQL source code and of PostgreSQL needing an audit. Any idea if the things mentioned in this are true? :-) Regards and best wishes, Justin Clift -- "My grandfather once told me that there are two kinds of people: those who work and those who take the credit. He told me to try to be in the first group; there was less competition there." - Indira Gandhi
Justin Clift <justin@postgresql.org> writes: > Whilst looking around for some more PostgreSQL related stuff, this > message turned up: > http://mail.wirex.com/pipermail/sardonix/2002-February/000051.html I see one unsubstantiated allegation about PG intermixed with a ton of content-free navel-gazing. Don't waste my time. There was some considerable effort awhile back towards eliminating unsafe printfs in favor of snprintfs and similar constructs; I doubt that the comments in that message postdate that effort. I have no doubt that some problems remain (cf recent agonizing over whether there is a buffer overrun problem in the date parser) ... but unspecific rumors don't help anyone. As always, the best form of criticism is a diff -c patch. regards, tom lane
On Mon, 12 Aug 2002, Justin Clift wrote: > Hi everyone, > > Whilst looking around for some more PostgreSQL related stuff, this > message turned up: > > http://mail.wirex.com/pipermail/sardonix/2002-February/000051.html > > The interesting bit is in an email messages included about halfway > down. It speaks of Bad Things in the PostgreSQL source code and of > PostgreSQL needing an audit. Christoper's point about University access to postgres and possible security/DoS problems is a good one. A thorough security audit of PostgreSQL would be a very good idea. Naturally, the biggest problems are that it is very time consuming to do an audit, just about all parts of the code need to be reviewed and it yields few exciting results. Perhaps Red Hat or another commercial entity would be interested in helping out given that the commercial space is beginning to be dominated by security rhetoric? Gavin
> I see one unsubstantiated allegation about PG intermixed with a ton > of content-free navel-gazing. Don't waste my time. For instance, when I submitted patches for fulltextindex 7.2 it freely used unchecked sprintf's everywhere. Even now I'm not sure what'll happen if a malicious user really tried to crash it. Anyway, who cares about printfs when stuff like select cash_out(2) is documented? > I have no doubt that some problems remain (cf recent agonizing over > whether there is a buffer overrun problem in the date parser) ... > but unspecific rumors don't help anyone. As always, the best form of > criticism is a diff -c patch. Maybe we could form a bunch of people on this list interested in checking for security issues and fixing them. I'd be in, time be willing... Chris
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes: > ... Anyway, who cares about printfs > when stuff like select cash_out(2) is documented? Well, they're two different issues. The cash_out problem is intrinsically difficult to fix, and *will* break user-defined datatypes when we fix it --- so I'm not eager to rush in a half-baked fix. OTOH, sprintf overruns are usually localized fixes, and there's no excuse for letting one go once we've identified it. I've just finished a quick grep through the backend sources for "sprintf", and identified the following files as containing possible problems: src/backend/port/dynloader/freebsd.c src/backend/port/dynloader/netbsd.c src/backend/port/dynloader/nextstep.c src/backend/port/dynloader/openbsd.c src/include/libpq/pqcomm.h src/pl/plpgsql/src/pl_comp.c Will work on these. There are a lot of sprintf's in contrib/ as well; anyone care to eyeball those? Anyone want to look for other trouble spots? BTW, one should distinguish "an already-authorized user is able to force a database restart" from more dire conditions such as "any random cracker can get root on your box". I'm getting fairly tired of chicken-little warnings from people who should know better. regards, tom lane
> I've just finished a quick grep through the backend sources for > "sprintf", and identified the following files as containing possible > problems: > src/backend/port/dynloader/freebsd.c This one is perhaps dodgy. You ahve this: static char error_message[BUFSIZ]; Then you have this: sprintf(error_message, "dlopen (%s) not supported", file); Where file isn't restricted in length I think... So does that mean if you go: CREATE FUNCTION blah AS '/home/chriskl/[90000 characters here].so' LANGUAGE 'C'; Sort of thing you could crash it? Chris
"Christopher Kings-Lynne" <chriskl@familyhealth.com.au> writes: >> src/backend/port/dynloader/freebsd.c > This one is perhaps dodgy. You ahve this: > static char error_message[BUFSIZ]; > Then you have this: > sprintf(error_message, "dlopen (%s) not supported", file); > Where file isn't restricted in length I think... Yeah. In practice I'm not sure there's a problem --- the callers may all limit the filename string to MAXPGPATH, which is well below BUFSIZ. But changing the sprintf to snprintf is a cheap, localized way to be sure. regards, tom lane