fix for issue #332: The validation for an Ipv6PrefixAttribute incorre…

[techiefrancistor]

Issue Ipv6PrefixAttribute validation failure

Summary

The validation for an Ipv6PrefixAttribute incorrectly rejects valid values. It works properly only when prefix sizes are multiple of 8.

For example, 2001:0:0:1650:0:0:0:0/60 will not be validated, although it is correct (the bits after the 60 are zero). Instead, and exception is thrown with the following message:

java.lang.IllegalArgumentException: Prefix-Length is 60, actual address has prefix length 63, bits outside of the Prefix-Length must be zero

Analysis

The issue is due to the fact that the number of non-zero bits of the IPv6 prefix are calculated using a java.util.BitSet, which is initialized by passing the 16 bytes of the IPv6 address with BitSet.valueOf(). But the created bits are using a little-endian representation (https://docs.oracle.com/javase/8/docs/api/java/util/BitSet.html#valueOf-byte:A-), instead of a big-endian as required to do the comparation properly.

For example, the prefix above is represented as shown below by BitSet

# Bitset representation (first line) vs Correct representation (sedond line)
00000100-10000000-00000000-00000000-00000000-00000000-01101000-00001010-00000000-00000000-00000000-00000000-00000000-00000000-00000000-00000000
00100000-00000001-00000000-00000000-00000000-00000000-00010110-01010000-00000000-00000000-00000000-00000000-00000000-00000000-00000000-00000000

This has been produced using the following code snippet

InetAddress address = InetAddress.getByName("2001:0:0:1650::0");
byte[] addrBytes = address.getAddress();

final BitSet bitSet = BitSet.valueOf(addrBytes);
final int bitSetLength = bitSet.length();
System.out.println("Bitset representation");
for(int i = 0; i < 128; i++){
    if(bitSet.get(i)) System.out.print("1"); else System.out.print("0");
    // To separate the bytes for better reading
    if(i % 8 == 7) System.out.print("-");
}
System.out.println();
System.out.println("Correct IPv6 representation");
for(int i = 0; i < addrBytes.length; i++){
    System.out.print(String.format("%8s-", Integer.toBinaryString((int)addrBytes[i] & 0xff)).replace(" ","0"));
}
System.out.println();
System.out.println(bitSetLength);

Fix

The class Ipv6PrefixAttribute.java must be updated.

Since probably you don't want to introduce external dependencies to libraries such as https://seancfoley.github.io/IPAddress/ or https://mvnrepository.com/artifact/com.github.jgonian/commons-ip-math, the checking can be done using pure java. The code is not trivial to read but only has two lines

// Preparation with test data
InetAddress ipv6Address = InetAddress.getByName("ffff:ffff:ffff:fff0::0");
byte[] addrBytes = ipv6Address.getAddress();
int prefix = 60;

// Check the first byte that must have some zeroed bits is conformant. This one is special because the
// comparison requires masking some bits, instead of checking the full byte as zero
boolean passed = (addrBytes[p/8] & &0xff (0xff >> p - 8*(p/8))) == 0;
// Check the rest of the bytes
for(int i = p/8 + 1; i < 16; i++) passed = passed & (addrBytes[i] == 0);

// passed is true if the address bytes do not have bits to 1 after the prefix

Since BitSet is not used, the building of the attributes must be slightly changed. I'd like better just to have all the bytes in the attribute sent, even if zero, so using...

    return ByteBuffer.allocate(18)
            .put((byte) 0)
            .put((byte) prefixLength)
            .put(addressBytes)
            .array();

...but this breaks the BaseRadiusPacketTest.addAttribute() test, because it is checking a string output that includes the expected length, and I'd rather minimize the proposed changes in this pull request.

Testing

A new test is added for an IPv6 prefix with a prefix which is not a multipe of 8

@Test
void string60bitsOk() {
    final Ipv6PrefixAttribute attribute = (Ipv6PrefixAttribute)
            IPV6_PREFIX.create(dictionary, -1, 97, (byte) 0, "2001:0:0:1650:0:0:0:0/60"); // Framed-IPv6-Prefix
    assertEquals("2001:0:0:1650:0:0:0:0/60", attribute.getValueString());
}

This test will fail in the current implementation and will pass with the fix.

[horaceli]

@techiefrancistor thanks for the PR, I've merged it now

Re your point about external dependencies, you are correct - we want to keep this the core lib as lightweight as possible. However I am happy to use them in tests to to verify the behaviour matches - I do something similar for the authenticator verification.

Re the point about sending all the bytes as zeros, I would still want to trim it as per RFC 8044
https://datatracker.ietf.org/doc/html/rfc8044#autoid-23

The Prefix field SHOULD NOT contain more octets than necessary to encode the Prefix field.