fix for palloc() of user-supplied length

Neil Conway <neilc@samurai.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@sss.pgh.pa.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@samurai.com> || PGP Key ID: DB3C29FC

Neil Conway <neilc@samurai.com> writes:

Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.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@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

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@samurai.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@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

Bruce Momjian <pgman@candle.pha.pa.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@samurai.com> || PGP Key ID: DB3C29FC

Quoting Neil Conway <neilc@samurai.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@cs.concordia.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@samurai.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@cs.concordia.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@samurai.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@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

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

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

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

Neil Conway wrote:

Serguei Mokhov <mokhov@cs.concordia.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@samurai.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@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

Bruce Momjian <pgman@candle.pha.pa.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@samurai.com> || PGP Key ID: DB3C29FC

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

Patch backed out. Thanks.

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

Neil Conway wrote:

Bruce Momjian <pgman@candle.pha.pa.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@samurai.com> || PGP Key ID: DB3C29FC

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

Would someone submit a patch for this?

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

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

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>
Sent: September 02, 2002 1:05 AM

Would someone submit a patch for this?

Working on it.

-s

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