Re: fix for palloc() of user-supplied length - Mailing list pgsql-patches
From | Bruce Momjian |
---|---|
Subject | Re: fix for palloc() of user-supplied length |
Date | |
Msg-id | 200209030404.g8344L624827@candle.pha.pa.us Whole thread Raw |
In response to | Re: fix for palloc() of user-supplied length ("Serguei Mokhov" <mokhov@cs.concordia.ca>) |
List | pgsql-patches |
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@alcor.concordia.ca> > Sent: September 02, 2002 4:11 AM > > > Hello, > > > > ----- Original Message ----- > > From: "Bruce Momjian" <pgman@candle.pha.pa.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@samurai.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@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
pgsql-patches by date: