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

I wish there was an easier way to fix this, but it seems you have done
the research and this is what is required.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://207.106.42.251/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

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

Serguei Mokhov wrote:
> 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, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo(at)postgresql(dot)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