From owner-freebsd-current@freebsd.org Thu May 19 09:13:28 2016 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7F792B41B3C for ; Thu, 19 May 2016 09:13:28 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from mailman.ysv.freebsd.org (mailman.ysv.freebsd.org [IPv6:2001:1900:2254:206a::50:5]) by mx1.freebsd.org (Postfix) with ESMTP id 6FC511213 for ; Thu, 19 May 2016 09:13:28 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: by mailman.ysv.freebsd.org (Postfix) id 6F0D7B41B3B; Thu, 19 May 2016 09:13:28 +0000 (UTC) Delivered-To: current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6EA9DB41B3A for ; Thu, 19 May 2016 09:13:28 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from gw.catspoiler.org (unknown [IPv6:2602:304:b010:ef20::f2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "gw.catspoiler.org", Issuer "gw.catspoiler.org" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 338591212; Thu, 19 May 2016 09:13:28 +0000 (UTC) (envelope-from truckman@FreeBSD.org) Received: from FreeBSD.org (mousie.catspoiler.org [192.168.101.2]) by gw.catspoiler.org (8.15.2/8.15.2) with ESMTP id u4J9DKvV082394; Thu, 19 May 2016 02:13:24 -0700 (PDT) (envelope-from truckman@FreeBSD.org) Message-Id: <201605190913.u4J9DKvV082394@gw.catspoiler.org> Date: Thu, 19 May 2016 02:13:20 -0700 (PDT) From: Don Lewis Subject: Re: r299512 breaks dhclient on some networks To: cem@FreeBSD.org cc: current@freebsd.org In-Reply-To: MIME-Version: 1.0 Content-Type: TEXT/plain; charset=us-ascii X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 May 2016 09:13:28 -0000 On 18 May, Conrad Meyer wrote: > On Wed, May 18, 2016 at 5:19 PM, Don Lewis 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).