Thread: No sanity checking performed on binary TIME parameters.

When submitting a query via the V3 binary protocol (PQexecParams,  
paramFormats[n]=1), it appears the PostgreSQL server performs no range  
checking on the passed values. Passing values greater than 24 hours  
results in unpredictable results (dumps that cannot be restored,  
strange output when printing the column in psql, etc). Tested with  
version 8.1 and 8.2 (integer_datetimes is false).

Using my python ocpgdb module (http://code.google.com/p/ocpgdb/):
>>> db.execute('select %s::time::text', DateTimeDelta(0,23,59,59))
[('23:59:59',)]>>> db.execute('select %s::time::text', DateTimeDelta(0,28,0,0))
[('K|\x1f',)]

ocpgdb has a lower-level API which is a thin layer on top of libpq -  
exercising this directly to rule out any problems with the  
mx.DateTime.DateTimeDelta class yields the same results:
>>> import struct>>> import ocpgdb, oclibpq>>> db=oclibpq.PgConnection('')>>> list(db.execute('select $1::time::text',
[(ocpgdb.pgoid.time, 
 
struct.pack('!d', 23*60*60))]))
[(<PyPgCell name 'text', type 25, modifier -1, value '23:00:00' at  
0x42a4a0>,)]>>> list(db.execute('select $1::time::text', [(ocpgdb.pgoid.time,  
struct.pack('!d', 48*60*60))]))
[(<PyPgCell name 'text', type 25, modifier -1, value 'K|\x1f' at  
0x42a500>,)]

Apologies if this bug has already been addressed - I didn't find any  
references to it while googling.

Andrew McNamara <andrewm@object-craft.com.au> writes:
> When submitting a query via the V3 binary protocol (PQexecParams,  
> paramFormats[n]=1), it appears the PostgreSQL server performs no range  
> checking on the passed values.

A quick look at time_recv() shows this is true, and timetz_recv()
checks neither the time nor the zone component.

> Passing values greater than 24 hours  
> results in unpredictable results (dumps that cannot be restored,  
> strange output when printing the column in psql, etc).

I'm not entirely sure why we put a range limit on time values at all,
but given that we do, it'd probably be a good idea to check the range
in the recv functions.  I'm inclined to fix this for 8.4, but not
back-patch because of compatibility considerations.  Any objections
out there?
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I'm not entirely sure why we put a range limit on time values at all,
> but given that we do, it'd probably be a good idea to check the range
> in the recv functions.  I'm inclined to fix this for 8.4, but not
> back-patch because of compatibility considerations.  Any objections
> out there?

Are we confident it can't be abused to impact other clients connecting
or break the back-end in some way?  More specifically, could it be a
security issue?  Havn't looked at it yet, but getting what sounded like
corrupted data back out could be bad..
Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I'm not entirely sure why we put a range limit on time values at all,
>> but given that we do, it'd probably be a good idea to check the range
>> in the recv functions.  I'm inclined to fix this for 8.4, but not
>> back-patch because of compatibility considerations.  Any objections
>> out there?

> Are we confident it can't be abused to impact other clients connecting
> or break the back-end in some way?  More specifically, could it be a
> security issue?  Havn't looked at it yet, but getting what sounded like
> corrupted data back out could be bad..

The only place I can find where an oversize time value behaves in a
seriously bogus fashion is in time_out, or more specifically
EncodeTimeOnly(): it fails to initialize its output string at all.
So you could easily get garbage text output, though in my quick tests
you seem to usually get an empty string instead.  The odds of an actual
crash seem pretty small, but not quite zero (if somehow there was no
zero byte up to the end of the stack).

My feeling is that the error check in EncodeTimeOnly is just stupid and
should be removed.  That code will work fine with oversize times (and
no, it won't overrun the output buffers either).  The callers aren't
bothering to check for error returns anyway...

On the whole the argument that it could be a security problem seems
pretty thin.
        regards, tom lane

On 26/05/2009, at 5:41 AM, Tom Lane wrote:
>
> The only place I can find where an oversize time value behaves in a
> seriously bogus fashion is in time_out, or more specifically
> EncodeTimeOnly(): it fails to initialize its output string at all.
> So you could easily get garbage text output, though in my quick tests
> you seem to usually get an empty string instead.  The odds of an  
> actual
> crash seem pretty small, but not quite zero (if somehow there was no
> zero byte up to the end of the stack).

I'm seeing all sorts of odd stuff - typically the last column value  
output, but occasionally other snippets of random data that don't seem  
related to the query.

> My feeling is that the error check in EncodeTimeOnly is just stupid  
> and
> should be removed.  That code will work fine with oversize times (and
> no, it won't overrun the output buffers either).  The callers aren't
> bothering to check for error returns anyway...


I'm not sure it's postgresql's job to police things like this, but  
returning values greater than 24 hours may violate assumptions in user  
code, and I would be worried about potentially causing silent  
failures. Of course, it should no longer be possible to get an illegal  
value into the database, so the risk is low - either a database that  
predates the fix, or database corruption.

Are there any other cases where the binary receive functions are  
missing sanity checks?

Andrew McNamara <andrewm@object-craft.com.au> writes:
> Are there any other cases where the binary receive functions are  
> missing sanity checks?

Possibly --- you want to go looking?
        regards, tom lane

On 26/05/2009, at 10:25 AM, Tom Lane wrote:

> Andrew McNamara <andrewm@object-craft.com.au> writes:
>> Are there any other cases where the binary receive functions are
>> missing sanity checks?
>
> Possibly --- you want to go looking?

Uh. I'd be lying if I said I "wanted to" - I got enough of a taste of  
those functions when trying to work out what they expect when doing my  
ocpgdb module. At the moment, however, I have a good excuse for  
wimping out - every waking hour is being expended on swine flu related  
work (although I'd almost welcome the distraction of a good *virtual*  
bug hunt).