Re: r299512 breaks dhclient on some networks

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.

> 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.

Best,
Conrad