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

I haven't seen any. If no one comments in a few days, I will apply it
because I need a fix before 7.3 final. I will add it to the patches
queue in a day or two.

---------------------------------------------------------------------------

Serguei Mokhov wrote:
> Hello again,
>
> *any* comment on this at all?
>
> thanks,
> -s
>
> ----- Original Message -----
> From: "Serguei Mokhov" <sa_mokho(at)alcor(dot)concordia(dot)ca>
> Sent: September 02, 2002 4:11 AM
>
> > 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
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: Have you searched our list archives?
>
> http://archives.postgresql.org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073