Thread: Problem with CIDR data type restrictions
Yesterday I applied a patch that fixed incorrect restriction checking on
CIDR data. Our old code didn't check the last byte for non-zero values
so '1.1.1.1/25'::cidr would be accepted but '1.1.1.1/24'::cidr would
not. Both should be rejected as cidr types, though they are OK for
inet.
The new problem Andrew Dunstan discovered is that because cidr and inet
are marked as implicitly casted in pg_cast, you can bypass the cidr
restriction by casting from inet to cidr then inserting into a cidr
column:test=# SELECT '1.1.1.1/3'::inet::cidr; cidr------------ 1.1.1.1/3
Basically the string comes in as inet, checks OK, then gets cast to cidr
with no additional checks. You can insert invalid values into a table
using this method:
test=> CREATE TABLE test (x cidr);CREATE TABLEtest=> INSERT INTO test VALUES ('1.1.1.1/3');ERROR: invalid cidr value:
"1.1.1.1/3"DETAIL: Value has bits set to right of mask.test=> INSERT INTO test VALUES ('1.1.1.1/3'::inet);INSERT 17239
1test=#SELECT * FROM test; x----------- 1.1.1.1/3(1 row)
So now we have a cidr value in a column that is invalid:test=# SELECT '1.1.1.1/3'::cidr;ERROR: invalid cidr value:
"1.1.1.1/3"DETAIL: Value has bits set to right of mask.
Not sure how we can fix this without modifying the system tables. Not
sure how serious this is since we have gotten few complaints about it
but clearly it should be fixed.
-- 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,
Pennsylvania19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Not sure how we can fix this without modifying the system tables.
We can't: the only possible fix is to make inet-to-cidr not be a binary
compatible conversion but instead have an actual conversion function.
IMO the other direction should probably not be straight binary
compatible either; instead it ought to flip the bit that says "I'm a
CIDR value".
So fixing this requires a couple of new functions and some pg_cast
changes. Since we already forced initdb for beta4, there is a window of
opportunity to do that without any extra pain for beta testers, but the
fix would have to happen *now*.
> Not sure how serious this is since we have gotten few complaints about
> it but clearly it should be fixed.
Personally I'm inclined to leave it for 8.1. The inet/cidr code is
really designed around the assumption that these datatypes are
interchangeable, and I suspect that enforcing a stronger distinction
will actually take much more wide-ranging changes than just this.
Do all of the functions on inet/cidr take care to deliver a value that
is both correctly marked and declared as the correct type? I doubt it.
It needs some thought not just a band-aid ...
regards, tom lane
Tom Lane wrote: >Bruce Momjian <pgman@candle.pha.pa.us> writes: > > >>Not sure how serious this is since we have gotten few complaints about >>it but clearly it should be fixed. >> >> > >Personally I'm inclined to leave it for 8.1. The inet/cidr code is >really designed around the assumption that these datatypes are >interchangeable, and I suspect that enforcing a stronger distinction >will actually take much more wide-ranging changes than just this. >Do all of the functions on inet/cidr take care to deliver a value that >is both correctly marked and declared as the correct type? I doubt it. >It needs some thought not just a band-aid ... > > > > Yeah. I am not sure I understand the intention, but I should have thought there was a good case for clearing the bits past the mask on conversion from either text or inet, rather than rejecting or invalidly copying. As you say, it needs some thought. cheers andrew
Added to TODO: * Prevent inet cast to cidr if the unmasked bits are not zero, or zero bits --------------------------------------------------------------------------- Andrew Dunstan wrote: > > > Tom Lane wrote: > > >Bruce Momjian <pgman@candle.pha.pa.us> writes: > > > > > >>Not sure how serious this is since we have gotten few complaints about > >>it but clearly it should be fixed. > >> > >> > > > >Personally I'm inclined to leave it for 8.1. The inet/cidr code is > >really designed around the assumption that these datatypes are > >interchangeable, and I suspect that enforcing a stronger distinction > >will actually take much more wide-ranging changes than just this. > >Do all of the functions on inet/cidr take care to deliver a value that > >is both correctly marked and declared as the correct type? I doubt it. > >It needs some thought not just a band-aid ... > > > > > > > > > > Yeah. > > I am not sure I understand the intention, but I should have thought > there was a good case for clearing the bits past the mask on conversion > from either text or inet, rather than rejecting or invalidly copying. > > As you say, it needs some thought. > > cheers > > andrew > > ---------------------------(end of broadcast)--------------------------- > TIP 7: don't forget to increase your free space map settings > -- 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, Pennsylvania19073