Re: BUG #1254: CIDR check for no host-bit set has off-by-1 error

Nice catch. Patch attached and applied. Thanks.

We will have to mention in the release notes that this as a possible bad
data load problem from previous releases. I have already marked the
commit message accordingly.

I have also adjusted the regression tests to test for success and
failure of CIDR masks that are part of the final byte being tested.

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

PostgreSQL Bugs List wrote:
>
> The following bug has been logged online:
>
> Bug reference: 1254
> Logged by: kevin brintnall
>
> Email address: kbrint(at)rufus(dot)net
>
> PostgreSQL version: 7.4.5
>
> Operating system: FreeBSD 4.8-RELEASE and SunOS 5.8
>
> Description: CIDR check for no host-bit set has off-by-1 error in
> src/backend/utils/adt/network.c
>
> Details:
>
> The function addressOK() in src/backend/utils/adt/network.c
> allows some invalid values to be inserted into CIDR columns.
>
> I think this is because the author confused the Nth octet in
> an IP dotted-quad with the array offset a[N], which of course
> will produce an off-by-one error. Here is an example of some
> INSERTs that are permitted but SHOULD BE REJECTED because they
> contain 1's in the host bits:
>
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/30');
> INSERT 17160 1
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/26');
> INSERT 17161 1
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/25');
> INSERT 17162 1
>
> You can see that the INSERTs start to fail at the /24 byte
> boundary.
>
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/24');
> ERROR: invalid cidr value: "204.248.199.199/24"
> DETAIL: Value has bits set to right of mask.
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/23');
> ERROR: invalid cidr value: "204.248.199.199/23"
> DETAIL: Value has bits set to right of mask.
>
> You can see the same problem manifest at the byte boundary
> between /16 and /17. Note that NONE of these INSERTs should
> be accepted.
>
> test=> INSERT INTO c (ip) VALUES ('204.248.199.0/18');
> INSERT 17166 1
> test=> INSERT INTO c (ip) VALUES ('204.248.199.0/17');
> INSERT 17167 1
> test=> INSERT INTO c (ip) VALUES ('204.248.199.0/16');
> ERROR: invalid cidr value: "204.248.199.0/16"
> DETAIL: Value has bits set to right of mask.
> test=> INSERT INTO c (ip) VALUES ('204.248.199.0/15');
> ERROR: invalid cidr value: "204.248.199.0/15"
> DETAIL: Value has bits set to right of mask.
>
> The function uses this integer division to "round up" to
> the next byte. Here it is clear that the author was
> thinking of IP octets and not array offsets:
>
> byte = (bits + 7) / 8;
>
> Here is a table listing which byte we want to start
> comparing for various values of bits:
>
> bits=0..7 start with a[0]
> bits=8..15 start with a[1]
> bits=16..23 start with a[2]
> bits=24..31 start with a[3]
>
> Since byte is used as an array offset (a[byte]), it
> is clear that that line should "round down" instead of "round up".
>
> Here is a patch listing the fix:
>
> 920c920
> < byte = (bits + 7) / 8;
> ---
> > byte = bits / 8;
>
> After applying this patch, the CIDR data type has its expected behavior,
> as we can see with the following INSERT commands:
>
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/32');
> INSERT 17168 1
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/31');
> ERROR: invalid cidr value: "204.248.199.199/31"
> DETAIL: Value has bits set to right of mask.
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/30');
> ERROR: invalid cidr value: "204.248.199.199/30"
> DETAIL: Value has bits set to right of mask.
>
> test=> INSERT INTO c (ip) VALUES ('204.248.199.199/26');
> ERROR: invalid cidr value: "204.248.199.199/26"
> DETAIL: Value has bits set to right of mask.
>
> test=> INSERT INTO c (ip) VALUES ('204.248.199.0/24');
> INSERT 17169 1
> test=> INSERT INTO c (ip) VALUES ('204.248.199.0/23');
> ERROR: invalid cidr value: "204.248.199.0/23"
> DETAIL: Value has bits set to right of mask.
> test=> INSERT INTO c (ip) VALUES ('204.248.199.0/22');
> ERROR: invalid cidr value: "204.248.199.0/22"
> DETAIL: Value has bits set to right of mask.
>
> I believe the regression tests should also be modified to
> catch errors of this type. The use of bit-boundary
> netmasks in the regression test prevented this error from being
> discovered sooner.
>
> This error has existed since the "inet" data type was
> generalized from an IPV4-only 32-bit unsigned value to a generic
> character array in revision 1.42 (24/January/2003).
>
> Please let me know if/when you decide to integrate this fix.
>
> Thanks!! I really like your product.
>
> kevin brintnall
> <kbrint(at)rufus(dot)net>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073