Re: r299512 breaks dhclient on some networks

On 18 May, Conrad Meyer wrote:
> On Wed, May 18, 2016 at 5:19 PM, Don Lewis <truckman_at_freebsd.org> wrote:
>>
>> It looks to me like r299512 is changing the format of the client
>> identifier by inserting the struct hardware hlen field into it.
> 
> Yes.  The problem with r299512 is that it assumed the client_id was
> actually a valid struct hardware, as the array's size suggested, when
> in fact it has nothing to do with that struct.
> 
>>  That's
>> not valid if htype is non-zero the way I interpret RFC 2132.  On the
>> other hand, I would think that the server would interpret the client ID
>> as an opaque cookie, so I wouldn't think it would make a difference
>> (other than thinking this is a new client) unless your server is
>> configured to look for specific client IDs.
> 
> That seems like a pretty reasonable use of the client identifier.  Or
> less reasonably but still expected, only allowing client identifiers
> of exactly 6 bytes.

See section 9.14 in RFC 2132.  It's not a hard requirement because the
RFC uses MAY and SHOULD, but if the first byte of the ID is 1, then the
remainder of the ID should be a 6 byte ethernet MAC address.  If the
first byte is 0, then the ID is free form.

>> I think that r299512 is mostly incorrect and should be reverted.  The
>> problem reported by CID 1008682 is an overrun of hw_address.haddr.
>> struct hardware looks like this:
>>
>> struct hardware {
>>         u_int8_t htype;
>>         u_int8_t hlen;
>>         u_int8_t haddr[16];
>> };
>>
>> I think the correct fix is just this:
>>
>>                 size_t hwlen = MIN(ip->hw_address.hlen,
>>                     sizeof(ip->hw_address.haddr));
>>
>> The old code was wrong because sizeof(client_ident)-1 is the
>> same as sizeof(struct hardware)-1, when it should be -2 to exclude both
>> htype and hlen from the calculation.
> 
> Yep.  Or equivalently, I've defined the length of client_ident in
> terms of sizeof(haddr) + 1.  The result is the same.
> 
>> The fix for CID 1305550 looks correct.
> 
> Maybe; though I reverted it too.  Really I think hlen > sizeof(haddr)
> is invalid, but I'm not familiar enough with dhclient.c to insert that
> check in the right place.  I think throwing in MIN() in an ad-hoc
> fashion maybe isn't the best approach.

If the MIN() is omitted, then Coverity might be able to figure out that
hlen is never greater than sizeof(haddr) and the code is clean.  Just
adding the MIN() might make Coverity think that hlen is not well known
and that it needs to evaluate the code with hlen values that either pass
or fail the comparison.

hlen appears to be set by the code in the AF_LINK section of
discover_interfaces().  Note that Coverity isn't flagging the memcpy()
there, even though it has know way of knowing the relationship between
the length passed to memcpy() there and sizeof(hlen).  Even with a
sanity check there, I think that is too far removed from the code in
make_discover() for it to draw any conclusions about whether hlen still
meets the constraint.

I think the best thing to do is to remove both instances of MIN() and
optionally add an assert() before
	if (!options[DHO_DHCP_CLIENT_IDENTIFIER]) {
where it will protect both memcpy() calls.

BTW, the location of
	char client_ident[sizeof(struct hardware)];
violates style(9).