DHCP decoder handling of header string fields

[Alan DeKok]

On Feb 4, 2016, at 11:21 AM, Chaigneau, Nicolas <nicolas.chaigneau at capgemini.com> wrote
> I've noticed that FreeRADIUS DHCP decoder is handling strings from the DHCP header fields a bit oddly.
> 
> Basically, it allocates a value pair buffer with the maximum field length (64 or 128 octets, for DHCP-Server-Host-Name and DHCP-Boot-Filename respectively) + 1, and memcpy the data then add a '\0'.

  That's not good.

> But RFC 2131 explicitly states that these DHCP fields are null terminated strings. So why not handle them as such ?

  It should.

> Current code from "fr_dhcp_decode" function:
> 
> 		case PW_TYPE_STRING:
> 			q = talloc_array(vp, char, dhcp_header_sizes[i] + 1);
> 			memcpy(q, p, dhcp_header_sizes[i]);
> 			q[dhcp_header_sizes[i]] = '\0';
> 			fr_pair_value_strsteal(vp, q);
> 
> 			if (vp->vp_length == 0) fr_pair_list_free(&vp);
> 			break;

 That's inefficient for a whole host of reasons.


> How about something like this instead ? (copy a string, but do not trust the DHCP field to be actually null terminated)
> 
> 		case PW_TYPE_STRING:
> 			if (*p != '\0') {
> 				uint8_t *end;
> 				int len;
> 				end = memchr(p, '\0', dhcp_header_sizes[i]);
> 				len = end ? end - p : dhcp_header_sizes[i];
> 				q = talloc_array(vp, char, len + 1);
> 				memcpy(q, p, len);
> 				q[len] = '\0';
> 				fr_pair_value_strsteal(vp, q);
> 			}
> 			if (vp->vp_length == 0) fr_pair_list_free(&vp);

  That should work.  Though the check for a vp_length == 0 should really be done before allocating any memory.

> This would avoid having things like:
> 
>        DHCP-Server-Host-Name = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"
>        DHCP-Boot-Filename = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000"

  That's good.

> (I can do a pull request for 3.1.x if you wish)

  That would be good.  It should also be applied to 3.0.

  Alan DeKok.