Re: 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(at)postgresql(dot)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(at)familyhealth(dot)com(dot)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(at)familyhealth(dot)com(dot)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