Re: Buffer overflow in psql

От:

Martijn van Oosterhout <kleptog@svana.org>

Дата:

22 ноября 2006 г. в 12:42:37

On Wed, Nov 22, 2006 at 11:11:09AM -0500, Jack Orenstein wrote:
> I'm using Postgrseql 7.4.8. In January, I reported a psql bug. The
> problem was that an INSERT issued through psql would cause a
> crash. There was no problem with other operations I tried, or with the
> same INSERT submitted through JDBC. The discussion thread begins here:
> http://archives.postgresql.org/pgsql-bugs/2006-01/msg00071.php


> 		case PGRES_COMMAND_OK:
> 			{
> 				char		buf[10];
> 
> 				success = true;
> 				sprintf(buf, "%u", (unsigned int) 
> 				PQoidValue(results));
> 
> In 8.1.5, the sprintf is replaced by an snprintf, resulting in a less
> serious form of the bug.

Looks like you found something.

> 1) Is one of the postgresql developers willing to get this fix into
>    the next release? (We're patching our own 7.4.8 build.)

Probably, though I don't know the release cycle for backpatches.

> 2) If no one else has hit this, then it suggests I might be in
>    uncharted territory with OIDs getting this high.  Do I need to
>    review my vacuuming strategy? (I can summarize my vacuuming
>    strategy for anyone interested.)

I think most people have OIDs disabled, which avoids the problem
entirely. Perhaps that's why it hasn't been run into before.

Have a nice day,
-- 
Martijn van Oosterhout      http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: Buffer overflow in psql

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

22 ноября 2006 г. в 13:07:31

Jack Orenstein  writes:
> The problem has occurred again, and I've found a buffer overflow in
> psql that explains it. Here is code from src/bin/psql/common.c, from
> the PrintQueryResults function:

> 		case PGRES_COMMAND_OK:
> 			{
> 				char		buf[10];

> 				success = true;
> 				sprintf(buf, "%u", (unsigned int) PQoidValue(results));

Good catch!  What platform and compiler are you using exactly?  I'd
imagine that on most platforms, the size of that array is effectively
rounded up to 12 bytes due to alignment/padding considerations, which
would mask the mistake.  Yours must somehow be putting something
critical right after the array.

> 1) Is one of the postgresql developers willing to get this fix into
>     the next release? (We're patching our own 7.4.8 build.)

Yeah, we'll fix it.

			regards, tom lane

Re: Buffer overflow in psql

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

22 ноября 2006 г. в 14:53:05

Jack Orenstein  writes:
> Tom Lane wrote:
>> Good catch!  What platform and compiler are you using exactly?  I'd
>> imagine that on most platforms, the size of that array is effectively
>> rounded up to 12 bytes due to alignment/padding considerations, which
>> would mask the mistake.  Yours must somehow be putting something
>> critical right after the array.

> We're using gcc-4.0.2-8.fc4 on FC4 (intel). I believe that we didn't
> just get lucky with the overflow. One of our Linux experts says that
> our libc is doing memory bounds checking.

Ah so, that explains how come it noticed.  BTW, I see that somebody
already changed the array size to 16 bytes in HEAD --- so it's just
the back branches that need fixing.

			regards, tom lane

Buffer overflow in psql

От:

Jack Orenstein <jorenstein@archivas.com>

Дата:

22 ноября 2006 г. в 12:38:08

I'm using Postgrseql 7.4.8. In January, I reported a psql bug. The
problem was that an INSERT issued through psql would cause a
crash. There was no problem with other operations I tried, or with the
same INSERT submitted through JDBC. The discussion thread begins here:
http://archives.postgresql.org/pgsql-bugs/2006-01/msg00071.php

There was no resolution to this problem -- a bad psql build was
suggested and I couldn't disprove it.

The problem has occurred again, and I've found a buffer overflow in
psql that explains it. Here is code from src/bin/psql/common.c, from
the PrintQueryResults function:

		case PGRES_COMMAND_OK:
			{
				char		buf[10];

				success = true;
				sprintf(buf, "%u", (unsigned int) PQoidValue(results));

In 8.1.5, the sprintf is replaced by an snprintf, resulting in a less
serious form of the bug.

I believe that we end up in this code after an INSERT is processed
through psql. If PQoidValue returns 1000000000 (1 billion) or higher,
which requires 10 characters, then we overflow buf due to the terminal
zero.

Looking at my databases, I find that the problem occurs exactly in the
databases with OIDs above 1 billion for newly inserted rows. (Because
the psql crash occurs in processing results, the INSERT succeeds, and
I can examine the OIDs of the inserted rows.)

My January email indicates that we had been loading data for three
months, so OIDs could conceivably have gotten as high as 1 billion (if
I understand OIDs correctly). The problem occurred again at about the
same time -- three months into a test.

Changing the 10 to 11, INSERTs through psql no longer cause psql to
crash.

I have two questions:

1) Is one of the postgresql developers willing to get this fix into
    the next release? (We're patching our own 7.4.8 build.)

2) If no one else has hit this, then it suggests I might be in
    uncharted territory with OIDs getting this high.  Do I need to
    review my vacuuming strategy? (I can summarize my vacuuming
    strategy for anyone interested.)

Jack Orenstein

Re: Buffer overflow in psql

От:

Jack Orenstein <jorenstein@archivas.com>

Дата:

22 ноября 2006 г. в 14:46:43

Tom Lane wrote:
> Jack Orenstein  writes:
>> The problem has occurred again, and I've found a buffer overflow in
>> psql that explains it. Here is code from src/bin/psql/common.c, from
>> the PrintQueryResults function:
> 
>> 		case PGRES_COMMAND_OK:
>> 			{
>> 				char		buf[10];
> 
>> 				success = true;
>> 				sprintf(buf, "%u", (unsigned int) PQoidValue(results));
> 
> Good catch!  What platform and compiler are you using exactly?  I'd
> imagine that on most platforms, the size of that array is effectively
> rounded up to 12 bytes due to alignment/padding considerations, which
> would mask the mistake.  Yours must somehow be putting something
> critical right after the array.

We're using gcc-4.0.2-8.fc4 on FC4 (intel). I believe that we didn't
just get lucky with the overflow. One of our Linux experts says that
our libc is doing memory bounds checking. Note that the stack
goes through __sprintf_chk:

     /lib/libc.so.6(__chk_fail+0x41)[0xb7c0bbc5]
     /lib/libc.so.6(__vsprintf_chk+0x0)[0xb7c0b490]
     /lib/libc.so.6(_IO_default_xsputn+0x97)[0xb7b8e8d8]
     /lib/libc.so.6(_IO_vfprintf+0x1aeb)[0xb7b6a2f7]
     /lib/libc.so.6(__vsprintf_chk+0xa1)[0xb7c0b531]
     /lib/libc.so.6(__sprintf_chk+0x30)[0xb7c0b484]
     /usr/bin/psql[0x804ea63]
     /usr/bin/psql[0x805195b]
     /usr/bin/psql[0x804afdd]
     /usr/bin/psql[0x804cde9]
     /usr/bin/psql[0x804deb5]
     /usr/bin/psql[0x80510cf]
     /usr/bin/psql[0x805336d]
     /lib/libc.so.6(__libc_start_main+0xdf)[0xb7b42d7f]
     /usr/bin/psql[0x804a8e1]

Our Linux kernel is 2.6.17-1.2139

Jack Orenstein

Re: Buffer overflow in psql

От:

"John D. Burger" <john@mitre.org>

Дата:

27 ноября 2006 г. в 11:24:34

Tom Lane wrote:

>> Um, is that really considered a fix???  We all know that there's no
>> guarantee at all, even in ANSI C, that unsigned int isn't bigger than
>> 32 bits, right?
>
> OID is 32 bits.  Full stop.

I should know better than to argue about this, but:

In that case, casting it as in the OP's code sample seems problematic  
in the other direction:

   sprintf(buf, "%u", (unsigned int)PQoidValue(results));

since unsigned int could be as small as 16 bits, thus truncating the  
OID value.

Ok, I'll stop now, I promise.

- John D. Burger
   MITRE

Re: Buffer overflow in psql

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

22 ноября 2006 г. в 21:26:37

"John D. Burger"  writes:
> Tom Lane wrote:
>> Ah so, that explains how come it noticed.  BTW, I see that somebody
>> already changed the array size to 16 bytes in HEAD --- so it's just
>> the back branches that need fixing.

> Um, is that really considered a fix???  We all know that there's no  
> guarantee at all, even in ANSI C, that unsigned int isn't bigger than  
> 32 bits, right?

OID is 32 bits.  Full stop.

			regards, tom lane

Re: Buffer overflow in psql

От:

"John D. Burger" <john@mitre.org>

Дата:

22 ноября 2006 г. в 21:21:32

Tom Lane wrote:

> Ah so, that explains how come it noticed.  BTW, I see that somebody
> already changed the array size to 16 bytes in HEAD --- so it's just
> the back branches that need fixing.

Um, is that really considered a fix???  We all know that there's no  
guarantee at all, even in ANSI C, that unsigned int isn't bigger than  
32 bits, right?  There are still some weird architectures out there.

Whenever I need to print some integer x, I use code like this:

   char buf[1 + sizeof(x) * CHAR_BIT / 3]

I let the compiler figure out the length needed to print in octal,  
and use that as a (slight) over-estimate of the length for decimal.   
As a bonus, the type of x can be changed without having to track down  
this kind of crap.

Alternatively, the code in question could just cast to one of the  
newer fixed-length int types, like int32_t, although that has its own  
problems.

Sorry for the pedantry ...

- John D. Burger
   MITRE