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@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)