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

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

From: Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>
To: Serguei Mokhov <mokhov(at)cs(dot)concordia(dot)ca>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, 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-12 00:04:18
Message-ID: 200209120004.g8C04IH12257@candle.pha.pa.us (view raw or flat)
Thread:
Lists: pgsql-hackerspgsql-patches
This was already applied by Tom Lane.  I was not sure you were informed.

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


revision 1.91
date: 2002/09/04 23:31:34;  author: tgl;  state: Exp;  lines: +4 -5
Guard against send-lots-and-lots-of-data DoS attack from unauthenticated
users, by limiting the length of string we will accept for a password.
Patch by Serguei Mokhov, some editorializing by Tom Lane.

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

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

In response to

Responses

pgsql-hackers by date

Next:From: Bruce MomjianDate: 2002-09-12 00:19:44
Subject: Re: [HACKERS] About connectby()
Previous:From: Bruce MomjianDate: 2002-09-11 23:53:16
Subject: Re:

pgsql-patches by date

Next:From: Bruce MomjianDate: 2002-09-12 00:14:33
Subject: Re: contrib/tablefunc regression test
Previous:From: Bruce MomjianDate: 2002-09-11 23:56:09
Subject: Re: fmgr.h: PG_NARGS() for number of arguments passed

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