Thread: BUG #1254: CIDR check for no host-bit set has off-by-1 error in src/backend/utils/adt/network.c
BUG #1254: CIDR check for no host-bit set has off-by-1 error in src/backend/utils/adt/network.c
From
"PostgreSQL Bugs List"
Date:
The following bug has been logged online:
Bug reference: 1254
Logged by: kevin brintnall
Email address: kbrint@rufus.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@rufus.net>
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@rufus.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@rufus.net>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo@postgresql.org
>
--
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, Pennsylvania 19073
Index: src/backend/utils/adt/network.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/backend/utils/adt/network.c,v
retrieving revision 1.53
diff -c -c -r1.53 network.c
*** src/backend/utils/adt/network.c 29 Aug 2004 05:06:49 -0000 1.53
--- src/backend/utils/adt/network.c 8 Oct 2004 01:04:22 -0000
***************
*** 942,948 ****
if (bits == maxbits)
return true;
! byte = (bits + 7) / 8;
nbits = bits % 8;
mask = 0xff;
if (bits != 0)
--- 942,948 ----
if (bits == maxbits)
return true;
! byte = bits / 8;
nbits = bits % 8;
mask = 0xff;
if (bits != 0)