Horton
Menu

Thread

  1. Tightening binary receive functions

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-08-31T08:12:13Z

    After the thread started by Andrew McNamara a while ago:

http://archives.postgresql.org/message-id/E419F08D-B908-446D-9B1E-F3520163CE9C@object-craft.com.au

the question was left hanging if other binary recv functions are missing
sanity checks that the corresponding text input functions have, and
whether they might pose a security issue. Tom Lane went through all the
recv functions after that, and found a few missing checks but nothing
that would present a security issue, but posted them to the
pgsql-security list for a double check. I just went through them again,
and found no security issues either.

We should nevertheless fix the recv functions so that they don't accept
any values that the corresponding text functions reject. While the
server itself handles them, such values will throw an error on
pg_dump/restore, for example. Attached patch adds the required sanity
checks. I'm thinking of applying this to CVS HEAD, but not back-patch,
just like Tom did with the original problem reported with time_recv()
and timetz_recv().

The most notable of these is the change to "char" datatype. The patch
tightens it so that it no longer accepts values >127 with a multi-byte
database encoding. Also, the handling of encoding in xml_recv() was bogus.

Here's the list of issues found:

Tom Lane wrote:
> array_recv:
>
> Allows zero-length dimensions (because ArrayGetNItems doesn't complain);
> whereas array_in does not.  Not sure if this is an issue.  We have had
> -hackers discussions about the behavior of zero-length arrays, so I
> think there may be other ways to get them into the system anyway.
>
> Doesn't check lower bounds at all, so it's possible that lowerbound plus
> length overflows an int.  Not sure if this is a security problem either,
> but it seems like something that should be rejected.

Yes, that seems dangerous.

I note that the handling of large bounds isn't very rigid in the text
input function either:

postgres=# SELECT '[9999999999999999:9999999999999] = {1}'::integer[];
          int4
-----------------------------
 [2147483647:2147483647]={1}
(1 row)

We use plain atoi() to convert the subscripts to integers, which returns
INT_MAX for an overflow. I didn't do anything about that now, but the
patch adds a check for the upper bound overflow in array_recv().

> charrecv
>
> Doesn't bother its pretty little head with maintaining encoding validity.
> Of course the entire datatype doesn't, so this is hardly the fault of
> the recv routine in particular.  It might be that the type is fine but we
> ought to constrain char_text() to fail on high-bit-set char values unless
> DB encoding is single-byte.

Constraining char_text() seems like a good idea. The current behavior of
char_text() with a high-bit-set byte is not useful, while using the full
range of char can be. However, we have to constrain charout() as well,
or we're back to square one with charout(c)::text. And if we constrain
charout(), then we should constrain charin() as well. Which brings us
back to forbidding such values altogether.

The patch constrains all the functions that you can use to get a "char"
into the system: charin(), charrecv(), i4tochar() and text_char(). They
now reject values with high bit set when using a multi-byte database
encoding

> date_recv
>
> Fails to do any range check, so the value might cause odd behavior later.
> Should probably limit to the values date_in would accept.

Yep.

> float4recv, float8recv, and geometric and other types depending on
> pq_getmsgfloatN
>
> These all accept any bit pattern whatever for a float.  Now I know of no
> machine where float doesn't consider all bitpatterns "valid", but
> nonetheless there are some issues here:
>
> * We don't currently allow any other way to inject an IEEE signaling NaN
> into the database.  As far as I can think, this could only lead to float
> traps in places where one might perhaps not have expected one, so I
> don't think this is a real problem.

Agreed. This is frankly the first time I even hear about signaling NaNs,
and after reading up on that a bit, I get the impression that even on
platforms that support them, you need a #define or a compiler flag to
enable them.

> * Some of these types probably aren't expecting NaNs or Infinities at all.
> Can anything really bad happen if they get one?

Geometric types seem to handle NaNs and Infs gracefully, although I
wonder what it means to have e.g a box with the X-coordinate of one
corner being NaN. I think it's ok from a security point of view.

timestamp_recv (with float timestamps) accepts NaN, while timestamp_in
does not. I'm not sure what happens if you pass a NaN timestamp to the
system, but we should forbid that anyway.

> interval_recv
>
> Fails to do any range check, but I think it's okay since we don't have any
> a-priori restrictions on the range of intervals.

Should disallow Infs and NaNs.

> numeric_recv
>
> Allows any weight or dscale value.  Can this cause problems?

It seems OK to me. make_result() normalizes and checks for overflow of
those.

> tintervalrecv
>
> Bogus --- fails to verify consistency of status with values or proper
> ordering of the two values.  Probably nothing very bad can happen, but
> should be fixed.

tintervalin doesn't check the ordering of the two values either. Status
should indeed be checked.

> xml_recv
>
> Encoding handling seems pretty questionable.  In particular I think it's
> going to treat a document without explicit encoding marking as being in
> the database encoding --- shouldn't it assume client encoding?
> Converting the encoding twice seems pretty icky as well.

Fixed this so that the input is treated as UTF-8 if no encoding is
specified in the XML header. client_encoding does not affect xml_recv()
at all. Per Peter's description.

Here's two extra ones:

oidvectorrecv
int2vectorrecv

Don't constrain the number of elements to FUNC_MAX_ARGS, like the text
input functions do. They also don't force lbound to 0, like the input
function. We assume in array_ref() and probably elsewhere too that
fixed-size array types are 0-based.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  2. Re: Tightening binary receive functions

    Greg Stark <gsstark@mit.edu> — 2009-08-31T10:41:24Z

    On Mon, Aug 31, 2009 at 9:12 AM, Heikki
Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:
> The most notable of these is the change to "char" datatype. The patch
> tightens it so that it no longer accepts values >127 with a multi-byte
> database encoding.

That doesn't sound right to me. We accept casts from integer to "char"
for all values in range (-128..127). The question should be what the
text representation should be since the raw bytes aren't valid mb
encodings.


-- 
greg
http://mit.edu/~gsstark/resume.pdf

  3. Re: Tightening binary receive functions

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-08-31T11:01:54Z

    Greg Stark wrote:
> On Mon, Aug 31, 2009 at 9:12 AM, Heikki
> Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:
>> The most notable of these is the change to "char" datatype. The patch
>> tightens it so that it no longer accepts values >127 with a multi-byte
>> database encoding.
> 
> That doesn't sound right to me. We accept casts from integer to "char"
> for all values in range (-128..127). 

The patch limits that range to 0..127, with multibyte encodings.

> The question should be what the
> text representation should be since the raw bytes aren't valid mb
> encodings.

Hmm, perhaps we should follow what we did to chr() and ascii(): map the
integer to unicode code points if the database encoding is UTF-8, and
restrict the range to 0..127 for other multi-byte encodings.

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  4. Re: Tightening binary receive functions

    Greg Stark <gsstark@mit.edu> — 2009-08-31T11:27:46Z

    On Mon, Aug 31, 2009 at 12:01 PM, Heikki
Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:
> Hmm, perhaps we should follow what we did to chr() and ascii(): map the
> integer to unicode code points if the database encoding is UTF-8, and
> restrict the range to 0..127 for other multi-byte encodings.

I don't think we even have to worry about the database's encoding.
Just make the textual representation of "char" be \xxx (or perhaps we
could switch to \xHH now) if the value isn't a printable ascii
character. As long as "char" reads that in properly it doesn't matter
if it's not a reasonable multibyte character.

That allows people to treat it as a 1-byte integer type which happens
to allow input or output as a single ascii character which is
convenient sometimes.

-- 
greg
http://mit.edu/~gsstark/resume.pdf

  5. Re: Tightening binary receive functions

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-09-04T11:29:02Z

    Greg Stark wrote:
> On Mon, Aug 31, 2009 at 12:01 PM, Heikki
> Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:
>> Hmm, perhaps we should follow what we did to chr() and ascii(): map the
>> integer to unicode code points if the database encoding is UTF-8, and
>> restrict the range to 0..127 for other multi-byte encodings.
> 
> I don't think we even have to worry about the database's encoding.
> Just make the textual representation of "char" be \xxx (or perhaps we
> could switch to \xHH now) if the value isn't a printable ascii
> character. As long as "char" reads that in properly it doesn't matter
> if it's not a reasonable multibyte character.
> 
> That allows people to treat it as a 1-byte integer type which happens
> to allow input or output as a single ascii character which is
> convenient sometimes.

Hmm, seems reasonable. However, it would mean that "\123" would be
interpreted differently in the new version than the old. In previous
versions the extra characters are truncated and the value becomes '\',
whereas the new interpretation would be 'S'.


(I committed the rest of the patch, without the "char" changes)

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  6. Re: Tightening binary receive functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2009-09-05T22:22:43Z

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Greg Stark wrote:
>> Just make the textual representation of "char" be \xxx (or perhaps we
>> could switch to \xHH now) if the value isn't a printable ascii
>> character. As long as "char" reads that in properly it doesn't matter
>> if it's not a reasonable multibyte character.

> Hmm, seems reasonable. However, it would mean that "\123" would be
> interpreted differently in the new version than the old. In previous
> versions the extra characters are truncated and the value becomes '\',
> whereas the new interpretation would be 'S'.

I think that Greg's proposal might be the sanest solution.  Aside from
avoiding the encoding problem, it would give us a more reasonable text
representation for byte value 0 (ie, '\000' not '').  We could probably
address the compatibility risk sufficiently by having charin throw error
whenever the input is more than one byte long and does NOT have the form
'\nnn'.  This would also respond to the discussion of bug #5028 about
how charin ought not silently accept multicharacter input.

			regards, tom lane

  7. Re: Tightening binary receive functions

    James William Pye <lists@jwp.name> — 2009-10-24T16:22:34Z

    On Aug 31, 2009, at 1:12 AM, Heikki Linnakangas wrote:
> ...


Is the new date_recv() constraint actually correct?

[looking at the "result < 0" part, at least]

src/backend/utils/adt/date.c
...
+       /* Limit to the same range that date_in() accepts. */
+       if (result < 0 || result > JULIAN_MAX)
+               ereport(ERROR,
+                               (errcode 
(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                                errmsg("date out of range")));
+
+       PG_RETURN_DATEADT(result);
  }


postgres=# SELECT date_send('500-01-01'::date);
  date_send
------------
  \xfff7a3e9
(1 row)

...

 >>> struct.unpack("!l", b'\xff\xf7\xa3\xe9')
(-547863,)


Perhaps 'result' needs to be adjusted by the postgres epoch for the  
comparison?

  8. Re: Tightening binary receive functions

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2009-10-24T20:26:58Z

    >>>>> "James" == James Pye <lists@jwp.name> writes:

 James> Is the new date_recv() constraint actually correct?

No, it's not:

regression=# create table x (a date);
CREATE TABLE
regression=# insert into x values ('1999-01-01');
INSERT 0 1
regression=# copy x to '/tmp/tst.dmp' binary;
COPY 1
regression=# copy x from '/tmp/tst.dmp' binary; 
ERROR:  date out of range

-- 
Andrew (irc:RhodiumToad)

  9. Re: Tightening binary receive functions

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2009-10-26T08:56:12Z

    Andrew Gierth wrote:
>>>>>> "James" == James Pye <lists@jwp.name> writes:
> 
>  James> Is the new date_recv() constraint actually correct?
> 
> No, it's not:

Oops, you're right. The check is indeed confusing julian day numbers,
with epoch at 23th of Nov 4714 BC, with postgres-reckoning day numbers,
with epoch at 1th of Jan 2000. Thanks, will fix.

BTW, I just noticed that to_date() doesn't respect those limits either:

postgres=# create table x (a date);
CREATE TABLE
postgres=# insert into x values (to_date('-4713 11 23', 'YYYY MM DD'));
INSERT 0 1
postgres=# select * from x;
       a
---------------
 4714-11-23 BC
(1 row)

postgres=# copy x to '/tmp/tst.dmp'; -- text mode
COPY 1
postgres=# copy x from '/tmp/tst.dmp';
ERROR:  date out of range: "4714-11-23 BC"
CONTEXT:  COPY x, line 1, column a: "4714-11-23 BC"

The date arithmetic operators + and - also allow you to create such
dates. I also note that they don't check for overflow.

I'm thinking that we should fix all that by adding range checks to all
those functions (or maybe just in date2j() and the operators).

-- 
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

  10. Re: Tightening binary receive functions

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2009-10-26T14:59:45Z

    >>>>> "Heikki" == Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

 Heikki> Oops, you're right. The check is indeed confusing julian day
 Heikki> numbers, with epoch at 23th of Nov 4714 BC, with
 Heikki> postgres-reckoning day numbers, with epoch at 1th of Jan
 Heikki> 2000. Thanks, will fix.

Which reminds me: why isn't there an extract(jday from ...) function
or similar?

-- 
Andrew.

  11. Re: Tightening binary receive functions

    Bruce Momjian <bruce@momjian.us> — 2009-11-10T16:54:44Z

    FYI, Heikki has fixed this bug and the fix will appear in Postgres 8.5.

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

Andrew Gierth wrote:
> >>>>> "Heikki" == Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> 
>  Heikki> Oops, you're right. The check is indeed confusing julian day
>  Heikki> numbers, with epoch at 23th of Nov 4714 BC, with
>  Heikki> postgres-reckoning day numbers, with epoch at 1th of Jan
>  Heikki> 2000. Thanks, will fix.
> 
> Which reminds me: why isn't there an extract(jday from ...) function
> or similar?
> 
> -- 
> Andrew.
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +

  12. Re: Tightening binary receive functions

    James William Pye <lists@jwp.name> — 2010-02-17T03:55:11Z

    On Nov 10, 2009, at 9:54 AM, Bruce Momjian wrote:

> FYI, Heikki has fixed this bug and the fix will appear in Postgres 8.5.

>> Heikki> Oops, you're right. The check is indeed confusing julian day
>> Heikki> numbers, with epoch at 23th of Nov 4714 BC, with
>> Heikki> postgres-reckoning day numbers, with epoch at 1th of Jan
>> Heikki> 2000. Thanks, will fix.



Need a special case for the infinities as well?


postgres=# create table foo (d date);
CREATE TABLE
postgres=# INSERT INTO foo VALUES ('infinity');
INSERT 0 1
postgres=# COPY foo TO '/Users/jwp/foo.copy' WITH BINARY;
COPY 1
postgres=# COPY foo FROM '/Users/jwp/foo.copy' WITH BINARY;
ERROR:  date out of range
CONTEXT:  COPY foo, line 1, column d
postgres=# DELETE FROM foo;
DELETE 1
postgres=# INSERT INTO foo VALUES ('-infinity');
INSERT 0 1
postgres=# COPY foo TO '/Users/jwp/foo.copy' WITH BINARY;
COPY 1
postgres=# COPY foo FROM '/Users/jwp/foo.copy' WITH BINARY;
ERROR:  date out of range
CONTEXT:  COPY foo, line 1, column d

postgres=# SELECT version();
                                                                      version                                                                     
---------------------------------------------------------------------------------------------------------------------------------------------------
 PostgreSQL 8.5devel on i386-apple-darwin10.2.0, compiled by GCC i686-apple-darwin10-gcc-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5646) (dot 1), 64-bit
(1 row)

  13. Re: Tightening binary receive functions

    Takahiro Itagaki <itagaki.takahiro@oss.ntt.co.jp> — 2010-02-18T03:27:59Z

    James William Pye <lists@jwp.name> wrote:

> Need a special case for the infinities as well?
> 
> postgres=# create table foo (d date);
> postgres=# INSERT INTO foo VALUES ('infinity');
> postgres=# COPY foo TO '/Users/jwp/foo.copy' WITH BINARY;
> postgres=# COPY foo FROM '/Users/jwp/foo.copy' WITH BINARY;
> ERROR:  date out of range

Exactly. Patch attached.

We have special treatments of infinity in timestamp_recv,
but don't have in date_recv.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center