4.0.x - DHCP v4 encoder loops forever if given a string option > 255 octets

[nchaigne]

Issue type

• Defect - Unexpected behaviour (obvious or verified by project member).

Defect

Calling DHCP v4 encoder function (fr_dhcpv4_packet_encode) with a value pair containing a value (string or octets) too long for a DHCP option (> 255) makes it loop forever.

Function encode_rfc_hdr calls encode_value, which returns -1 if there is not enough space.
Then it tries again. But there will never be enough space in 255 octets, so it loops forever.

Proposed fix:

Have function fr_dhcpv4_encode_option check the length of the value pair it has to encode to DHCP options. If it exceeds 255, return an error.

if (vp->vp_length > 255) {
        fr_strerror_printf("Attribute \"%s\" value is too large for a DHCP option", vp->da->name);
        return -1;
}

[alandekok]

RFC 2131 says:

The values to be passed in an 'option' tag may be too long to fit in
the 255 octets available to a single option (e.g., a list of routers
in a 'router' option [21]). Options may appear only once, unless
otherwise specified in the options document. The client concatenates
the values of multiple instances of the same option into a single
parameter list for configuration.

We should just chop up the "too long" inputs into multiple options.

The RADIUS encoder does the same thing for some attributes.

[nchaigne]

OK, thanks for the RFC pointer.
Not so trivial then... I'll let you figure out how to fix this. :)

No urgency whatsoever of course.

[alandekok]

I don't see it looping forever in a test case, so I'm not sure what's wrong there.

I'll push a fix to split the option automatically

[nchaigne]

You can reproduce it with dhcpclient, e.g. with a 256 octets-long option:

echo "DHCP-Client-Hardware-Address=\"01:02:03:04:05:06\", DHCP-Merit-Dump-File=\"1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456\"" | dhcpclient 1.2.3.4 discover

[alandekok]

The issue here I think is "256", and not "300". :( It loops forever if fr_dhcpv4_encode_option() returns 0. Other values are OK.

I've pushed a fix so it doesn't loop forever. The larger fix of splitting data across multiple options will take more time.

[alandekok]

The fixes above should work. I've added the example to the test cases

[nchaigne]

Thanks for the fixes.

I tried with an option 82 larger than 255 bytes. Encoding seems to work, but Wireshark is not happy ("no room left in option for suboption value"). But maybe it's just too dumb to decode such long options...

My DHCP server returns an option 82 with a length of 0.
(Likely, the server doesn't know how to handle these long options either.)

However, FreeRADIUS cannot decode the reply: fr_dhcpv4_packet_decode fails. Seems it doesn't like an option with a length of 0.
I think this should be allowed.

[nchaigne]

A sub-option (82.n) of 253 octets is encoded in two options: one of length 0, and the other of length 255 (containing the sub-option of 253 octets).
While technically not incorrect, this is a bit weird (and unnecessary).

[nchaigne]

Thanks for these last fixes, it seems ok now.

Wireshark is still unhappy when a long sub-option is split across two options, but RFC 3396 clearly states how long options should be handled and I believe FreeRADIUS is now compliant :) (unless there's another RFC that says something different about sub-options...)

[alandekok]

Thanks for the review and update.