BUG #1254: CIDR check for no host-bit set has off-by-1 error in src/backend/utils/adt/network.c

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>