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

This patch fixes the so-called DoS possibility when processing the
password packet in recv_and_check_passwordv0(). Nothing fancy, I just
added a sanity check to ensure that we bail out if the client enters
an obviously-bogus length.

Cheers,

Neil

--
Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

Neil Conway <neilc(at)samurai(dot)com> writes:
> This patch fixes the so-called DoS possibility when processing the
> password packet in recv_and_check_passwordv0().

If len is signed, then something like "len < 1" needs to be in there
as well.

More generally, though, I was thinking that the appropriate answer at
this point is to rip out support for version-0 authentication
altogether. I can't believe anyone will be trying to connect to a 7.3
or beyond server with 6.2 client libraries (v0 went away in 6.3 as best
I can tell from the CVS logs). And if they try, it's not unreasonable
to force them to upgrade --- those old client libraries have got to be
pretty buggy themselves. So the utility of the v0 backend code is
dubious, while its potential for more problems is real.

Anyone want to argue that we should keep the v0 protocol support
any longer?

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> More generally, though, I was thinking that the appropriate answer
> at this point is to rip out support for version-0 authentication
> altogether. I can't believe anyone will be trying to connect to a
> 7.3 or beyond server with 6.2 client libraries (v0 went away in 6.3
> as best I can tell from the CVS logs).

Further, has this code actually been tested within recent memory? If
not, I wouldn't be surprised to learn that it's suffered some
bitrot...

> Anyone want to argue that we should keep the v0 protocol support any
> longer?

Nope, exactly the same thought crossed my mind while I was reading
through the code...

Cheers,

Neil

--
Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

Neil Conway <neilc(at)samurai(dot)com> writes:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> More generally, though, I was thinking that the appropriate answer
>> at this point is to rip out support for version-0 authentication
>> altogether.

> Further, has this code actually been tested within recent memory? If
> not, I wouldn't be surprised to learn that it's suffered some
> bitrot...

Yup, that's another good point. I don't think we *have* a way of
testing it any longer, unless someone cares to pull a 6.2 psql from the
archives ...

regards, tom lane

Neil Conway wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> > More generally, though, I was thinking that the appropriate answer
> > at this point is to rip out support for version-0 authentication
> > altogether. I can't believe anyone will be trying to connect to a
> > 7.3 or beyond server with 6.2 client libraries (v0 went away in 6.3
> > as best I can tell from the CVS logs).
>
> Further, has this code actually been tested within recent memory? If
> not, I wouldn't be surprised to learn that it's suffered some
> bitrot...
>
> > Anyone want to argue that we should keep the v0 protocol support any
> > longer?
>
> Nope, exactly the same thought crossed my mind while I was reading
> through the code...

Feel free to rip it out.

--
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

Neil, is this the one Sir-* complained about?

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

Neil Conway wrote:
> This patch fixes the so-called DoS possibility when processing the
> password packet in recv_and_check_passwordv0(). Nothing fancy, I just
> added a sanity check to ensure that we bail out if the client enters
> an obviously-bogus length.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

[ Attachment, skipping... ]

>
> ---------------------------(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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Neil, is this the one Sir-* complained about?

Yes.

I've attached a revised patch that includes the additional check Tom
suggested (len < 1). Unless anyone else steps forward, I'm inclined to
rip out support for version 0 of the protocol -- but I have more
urgent things to do for the beta, so it will likely need to wait for
7.4.

Cheers,

Neil

--
Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

Quoting Neil Conway <neilc(at)samurai(dot)com>:

> I've attached a revised patch that includes the additional check Tom
> suggested (len < 1). Unless anyone else steps forward, I'm inclined to

+ if (len < 1 || len > 8192)
+ {
+ elog(LOG, "Password packet length too long: %d", len);
^^^^^^^^
Shouldn't it be changed to 'too long || too long' then? ;)

And also for the message to be more descriptive for the innocent, I'd included
the current boundaries in it (like: "expected: 1 <= len <= 8192")
(a question: isn't hardcoding an evil?)

But I guess it's not a must-to-do on your list :)

--
Serguei A. Mokhov, <mailto: mokhov @ cs.concordia.ca>
Computer Science, Concordia University

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

I wrote:

> > I've attached a revised patch that includes the additional check Tom
> > suggested (len < 1). Unless anyone else steps forward, I'm inclined to
>
> + if (len < 1 || len > 8192)
> + {
> + elog(LOG, "Password packet length too long: %d", len);
> ^^^^^^^^
> Shouldn't it be changed to 'too long || too long' then? ;)

A typo: [too short or too short] :)

--
Serguei A. Mokhov, <mailto: mokhov @ cs.concordia.ca>
Computer Science, Concordia University

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

Serguei Mokhov <mokhov(at)cs(dot)concordia(dot)ca> writes:
> + if (len < 1 || len > 8192)
> + {
> + elog(LOG, "Password packet length too long: %d", len);
> ^^^^^^^^
> Shouldn't it be changed to 'too long || too long' then? ;)

Woops, sorry for being careless. Changed the wording to refer to
'invalid' rather than 'too long' or 'too short'.

> And also for the message to be more descriptive for the innocent, I'd included
> the current boundaries in it (like: "expected: 1 <= len <= 8192")

Also fixed, although I'm not sure it's worth worrying about.

> (a question: isn't hardcoding an evil?)

Yes, probably -- as the comment notes, it is just an arbitrary
limitation. But given that (a) it is extremely unlikely to ever be
encountered in a real-life situation (b) the limits it imposes are
very lax (c) it is temporary code that will be ripped out shortly, I'm
not too concerned...

Thanks for taking a look at the code, BTW.

Cheers,

Neil

--
Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

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

http://candle.pha.pa.us/cgi-bin/pgpatches

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

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

Neil Conway wrote:
> Serguei Mokhov <mokhov(at)cs(dot)concordia(dot)ca> writes:
> > + if (len < 1 || len > 8192)
> > + {
> > + elog(LOG, "Password packet length too long: %d", len);
> > ^^^^^^^^
> > Shouldn't it be changed to 'too long || too long' then? ;)
>
> Woops, sorry for being careless. Changed the wording to refer to
> 'invalid' rather than 'too long' or 'too short'.
>
> > And also for the message to be more descriptive for the innocent, I'd included
> > the current boundaries in it (like: "expected: 1 <= len <= 8192")
>
> Also fixed, although I'm not sure it's worth worrying about.
>
> > (a question: isn't hardcoding an evil?)
>
> Yes, probably -- as the comment notes, it is just an arbitrary
> limitation. But given that (a) it is extremely unlikely to ever be
> encountered in a real-life situation (b) the limits it imposes are
> very lax (c) it is temporary code that will be ripped out shortly, I'm
> not too concerned...
>
> Thanks for taking a look at the code, BTW.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

[ Attachment, skipping... ]

>
> ---------------------------(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

> > > Anyone want to argue that we should keep the v0 protocol support any
> > > longer?
> >
> > Nope, exactly the same thought crossed my mind while I was reading
> > through the code...
>
> Feel free to rip it out.

Should probably be mentioned in the release notes.

It will, if a patch is supplied. Anything significant that is mentioned
in the CVS logs gets shown in the release notes.

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

Matthew T. O'Connor wrote:
> > > > Anyone want to argue that we should keep the v0 protocol support any
> > > > longer?
> > >
> > > Nope, exactly the same thought crossed my mind while I was reading
> > > through the code...
> >
> > Feel free to rip it out.
>
> Should probably be mentioned in the release notes.
>
>
> ---------------------------(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

I have applied the following modified version of your patch. The
original version would not apply to CVS.

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

Neil Conway wrote:
> Serguei Mokhov <mokhov(at)cs(dot)concordia(dot)ca> writes:
> > + if (len < 1 || len > 8192)
> > + {
> > + elog(LOG, "Password packet length too long: %d", len);
> > ^^^^^^^^
> > Shouldn't it be changed to 'too long || too long' then? ;)
>
> Woops, sorry for being careless. Changed the wording to refer to
> 'invalid' rather than 'too long' or 'too short'.
>
> > And also for the message to be more descriptive for the innocent, I'd included
> > the current boundaries in it (like: "expected: 1 <= len <= 8192")
>
> Also fixed, although I'm not sure it's worth worrying about.
>
> > (a question: isn't hardcoding an evil?)
>
> Yes, probably -- as the comment notes, it is just an arbitrary
> limitation. But given that (a) it is extremely unlikely to ever be
> encountered in a real-life situation (b) the limits it imposes are
> very lax (c) it is temporary code that will be ripped out shortly, I'm
> not too concerned...
>
> Thanks for taking a look at the code, BTW.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

[ Attachment, skipping... ]

>
> ---------------------------(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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I have applied the following modified version of your patch. The
> original version would not apply to CVS.

Yes, the reason being that Tom removed the entire section of code that
my patch modified (and that is the better solution, IMHO).

The patch you've applied does something rather different, and is
unrelated to the "vulnerability" reported by Mordred and referred to
in the Subject -- your patch adds some additional sanity checking when
reading the password packet from v1 protocol clients. This is
unnecessary for two reasons:

(1) We use a StringInfo to hold the input data, which is
dynamically allocated as necessary. Since there's no
palloc() with user-supplied data, you'd need to write x
bytes to the backend to force it to allocate x bytes of
memory (i.e. potential for DoS is low).

(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.

You should probably back out your patch.

Cheers,

Neil

--
Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC

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

Patch backed out. Thanks.

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

Neil Conway wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I have applied the following modified version of your patch. The
> > original version would not apply to CVS.
>
> Yes, the reason being that Tom removed the entire section of code that
> my patch modified (and that is the better solution, IMHO).
>
> The patch you've applied does something rather different, and is
> unrelated to the "vulnerability" reported by Mordred and referred to
> in the Subject -- your patch adds some additional sanity checking when
> reading the password packet from v1 protocol clients. This is
> unnecessary for two reasons:
>
> (1) We use a StringInfo to hold the input data, which is
> dynamically allocated as necessary. Since there's no
> palloc() with user-supplied data, you'd need to write x
> bytes to the backend to force it to allocate x bytes of
> memory (i.e. potential for DoS is low).
>
> (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.
>
> You should probably back out your patch.
>
> Cheers,
>
> Neil
>
> --
> Neil Conway <neilc(at)samurai(dot)com> || PGP Key ID: DB3C29FC
>
>

--
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

Would someone submit a patch for this?

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

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 1: subscribe and unsubscribe commands go 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

----- 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?

Working on it.

-s

> 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

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

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

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

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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> I wish there was an easier way to fix this, but it seems you have done
> the research and this is what is required.

This is awfully messy. There's got to be a cleaner way of divvying up
this code...

regards, tom lane

Quoting Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I wish there was an easier way to fix this, but it seems you have done
> > the research and this is what is required.
>
> This is awfully messy. There's got to be a cleaner way of divvying up
> this code...

Could you point out, what's exactly unclean? Most importantly,
what would be the way you'd fix it?

Thank you,

--
Serguei A. Mokhov, <mailto: mokhov @ cs.concordia.ca>
Computer Science, Concordia University

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

Quoting Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > I wish there was an easier way to fix this, but it seems you have done
> > the research and this is what is required.
>
> This is awfully messy. There's got to be a cleaner way of divvying up
> this code...

Just to clarify a bit on my solution in case my English didn't get through
properly the first time...

I simply provided two versions of pq_getstr - pq_getstr() with the same
behaviour as before (read until input isn't over by \0) and pq_getstr_bounded()
that reads up to a certain limit or till \0. Functions needed split, IMNSHO,
because grep of the source gave more invocations of pq_getstr, which I was
afaraid to break, so that's why two functions.

I can justify the rest as well, if you wish. If you are positive, be the change
in one function only it would not break anything, then the cleaner solution is
just to change that one function - pg_gestring() invoked directly by
pg_getstr().

-s

--
Serguei A. Mokhov, <mailto: mokhov @ cs.concordia.ca>
Computer Science, Concordia University

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/

Serguei Mokhov <mokhov(at)cs(dot)concordia(dot)ca> writes:
> Could you point out, what's exactly unclean? Most importantly,
> what would be the way you'd fix it?

What's bugging me is that even though the patch goes out of its way to
share code, there still seems to be a lot of duplicate code. You're not
getting the full benefit of sharing code between both cases, yet you
still seem to be paying the price of extra code complexity compared to
just copy-paste-and-modify.

What I'm thinking about is

-- pq_getstr takes a length limit parameter, which is (say) 0 for "no
limit". Since it's only called in one place, we can just change its
API; there's hardly any point in providing a backward-compatible routine.
(BTW, I agree with your implementation choice to check the limit only
once per bufferload, and thus have a fuzzy limit, but this needs to be
documented.)

-- pq_getstring becomes pq_getstring_bounded, with a limit parameter
that it just passes down.

-- We can "#define pq_getstring(buf) pq_getstring_bounded(buf, 0)" to
avoid changing the call sites that want unbounded input (not that there
are that many of 'em, but we may as well provide the macro).

Will adjust your patch in this way and apply.

regards, tom lane

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

Quoting Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us>:

>
> This was already applied by Tom Lane. I was not sure you were informed.

Yes, I was. Thank you.

-s

> ---------------------------------------------------------------------------
>
>
> 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 A. Mokhov, <mailto: mokhov @ cs.concordia.ca>
Computer Science, Concordia University

-------------------------------------------------
This mail sent through IMP: http://horde.org/imp/