Skip site navigation (1) Skip section navigation (2)

Re: fix for palloc() of user-supplied length

From: "Serguei Mokhov" <sa_mokho(at)alcor(dot)concordia(dot)ca>
To: "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us>,"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Neil Conway" <neilc(at)samurai(dot)com>,"PostgreSQL Patches" <pgsql-patches(at)postgresql(dot)org>
Subject: Re: fix for palloc() of user-supplied length
Date: 2002-09-02 08:11:17
Message-ID: 006501c25258$532db7e0$0301a8c0@gunnymede.lan (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
Hello,

----- Original Message ----- 
From: "Bruce Momjian" <pgman(at)candle(dot)pha(dot)pa(dot)us>
Sent: September 02, 2002 1:05 AM

> Would someone submit a patch for this?

Attached please find an attempt to fix the volunerability issue below.

Affected files are:

/src/include/libpq/libpq.h
/src/include/libpq/pqformat.h
/src/backend/libpq/pqformat.c
/src/backend/libpq/pqcomm.c
/src/backend/libpq/auth.c

"Briefly" the changes:

Main victims for the change were pq_getstring() and pq_getstr()
(which calls the former) in pqformat.c and pqcomm.c. pq_getstring() is the one reading
until \0 and might possibly render the system run out of memory.

Changing pq_getstring() alone would break a lot code, so I 
added a two more functions: pq_getstring_common() and
pq_getstring_bounded(). The former is a big part of what used to be
pq_getstring() and the latter is a copy of the new pq_getstring()
with the string length check. Creating pq_getstring_common()
was suggested by its reuse in pq_getstring() and pq_getstring_bounded()
avoiding code duplication.

Similar changes were done for pq_getstr(). Its common code converting
to MULTIBYTE was placed in pq_getstr_multibyte() and pq_getstr() and
(newly added) pq_getstr_bounded() both call it before returning a result.

WRT above, two places in auth.c were changed to call pq_getstr_bounded()
instead of pq_getstr() on password read. I'm not sure if
there are other places where that might be needed...

Might look ugly for some, but looks like a not-so-bad solution
to me. If I'm completely wrong, I'd like to have some guidance then :)
Please review with care. I'm off to bed.

Thanks,
-s

PS: The patch also fixes a typo in the be-secure.c comment :)

> Tom Lane wrote:
> > Neil Conway <neilc(at)samurai(dot)com> writes:
> > >         (2) The length supplied by the user is completely ignored by
> > >             the code, and it simply reads the input until it sees a
> > >             NULL terminator (read the comments in the code about 10
> > >             lines down.) Therefore, any sanity checking on the length
> > >             specified by the user is a waste of time.
> > 
> > Agreed; the fact that the protocol requires a length word at all is just
> > a hangover from the past.  We can read the length word and forget it.
> > 
> > I wonder though if it'd be worthwhile to limit the length of the string
> > that we are willing to read from the client in the second step.  We are
> > at this point dealing with an unauthenticated user, so we should be
> > untrusting.  And I think Sir Mordred has a point: forcing a backend to
> > allocate a lot of memory can be a form of DoS attack.
> > 
> > regards, tom lane

Attachment: conn-limit-read.patch.gz
Description: application/x-gzip (2.9 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Christopher Kings-LynneDate: 2002-09-02 08:49:19
Subject: Re: tsearch stop words
Previous:From: Thomas O'DowdDate: 2002-09-02 07:49:56
Subject: Re: current build fail

pgsql-patches by date

Next:From: Peter EisentrautDate: 2002-09-02 22:27:05
Subject: Re: swedish pg_controldata
Previous:From: Joe ConwayDate: 2002-09-02 07:17:39
Subject: one last patch - array lower and upper bound

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group