Date: Wed, 18 May 2016 17:28:17 -0700 From: Conrad Meyer <cem@FreeBSD.org> To: Don Lewis <truckman@freebsd.org> Cc: Ian FREISLICH <ian.freislich@capeaugusta.com>, current <current@freebsd.org> Subject: Re: r299512 breaks dhclient on some networks Message-ID: <CAG6CVpWMeHSbHZpQaObuB-S_73Tw4-PCnLfbOSpchbpVfdk_Vw@mail.gmail.com> In-Reply-To: <201605190019.u4J0JFXN080729@gw.catspoiler.org> References: <15379a54-3e6e-2c2c-b41c-8def3af65bad@capeaugusta.com> <201605190019.u4J0JFXN080729@gw.catspoiler.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, May 18, 2016 at 5:19 PM, Don Lewis <truckman@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG6CVpWMeHSbHZpQaObuB-S_73Tw4-PCnLfbOSpchbpVfdk_Vw>