Skip site navigation (1)Skip section navigation (2)
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>