dhclient in 6.0

[Chuck Swiger]

David W. Hankins wrote:
> On Sat, Feb 04, 2006 at 01:37:33AM -0500, Chuck Swiger wrote:
[ ...sorry for the minor delay, realjob sometimes interferes... ]
>> OK, cool.  Are you doing anything with regard to Zeroconf/Rendezvous?
> 
> Not really, no, except when DHCP options appear to turn off IPv4LL, or
> that sort of thing.
> 
> What did you have in mind?

It might be nice for dhclient to leave an interface configured with a zeroconf
link-local address, if no lease can be obtained.  It's possible that the
multicast DNS resolution and service location could augment dhclient's attempts
to query information like which DNS servers are available.

If you haven't read this:

   http://files.stuartcheshire.org/draft-cheshire-ipv4-acd.txt

...it's worth considering the way it standardizes (or proposes to standardize)
how ARP traffic from unconfigured IPs should be done to avoid flooding large
networks, or polluting the ARP caches with traffic from unconfigured hosts.

>> would be nice if dhcpd would never get confused by it's own /var/db/dhcpd.leases
>> file.  I don't get useful diagnostics when this happens, but every once in a
>> blue moon dhcpd will no longer hand out leases until I delete the leases file
>> and restart it.
> 
> From this description it's hard to say what's happening.  From your
> second descriptoin, I think it's pretty obvious you have a rogue client
> that's eating up your free leases.

I know it's hard to debug from the lack of data; running of out leases was not
the problem, IIRC, the leases file had perhaps 30 or so, out of an address pool
of 150.

>> It would also be good if dhcpd would reassign the same IP to the same machine
>> (if the IP is not otherwise being used) if there was a prior lease matching the
>> client asking for a new lease, and not just when a client is trying to renew an
>> existing lease.
> 
> dhcpd tries hard - very hard - to find a lease to assign to the client
> it most recently was bound to.
[ ... ]
> 2) The client returns to an odd form of "INIT/BOUND" when it decides to
> renew its lease.  dhcpd sends an ICMP echo request when a client is sensed
> to be in the INIT state - since the client is bound, this echo request
> succeeds, and the lease is marked 'ABANDONED' to avoid duplicate
> allocations (the server has falsely discovered a conflict).  Workaround:
> disable ping-check.  We're also looking at better ways to get around
> this, changing ping-check behaviour in a future release.

Using ping-check appears to the reason, then.

I don't really want to disable ping-checking since I might manually configure a
machine in the DHCP dynamic pool range for whatever reasons...

[ ... ]
>> Please use snprintf() and not sprintf() whenever you're dealing with any data
>> which is not known to be static and compiled in; using sprintf() on data from
>> the network is simply unwise.  (Why yes, I happen to trust the security of the
>> OpenBSD code quite a bit more than the ISC code I've seen.  People writing code
>> which runs as root should take great pains to avoid code patterns which could
>> ever be exploitable.)
> 
> Already done.
> 
> Up until my tenure at ISC, in and around 3.0.1RC14 engineering, some
> includes/cf/* (system-local includes such as freebsd.h etc) used:
> 
>   #define vsnprintf(buf, size, fmt, list) vsprintf (buf, fmt, list)
> 
> And a stub snprintf() that calls vsnprintf() within the sources.
> 
> So astounding is this, when I encountered the problem in snprintf()
> overflowing the buffer it was given, my immediate reaction was "libc bug."
> 
> It was more believable to me that someone optimized-out the bounds check
> than that our code #defined it away.
> 
> Because really, who would do a thing like that?
> 
> What a rude awakening that was.

Oh, my goodness.  :-)  Well, some people write scary code, and it good to fix
it, thanks for being aware of the issue.

> As far as I am aware, from my recent audits of the software, there do not
> currently exist any unbounded buffer utilizations.  sprintf() is used,
> frequently, but it is gauged in each event as impossible to overflow
> (even if the contents of the arguments are maligned).  The company line
> on this is that it must be "auditable within (n) lines of where it is
> used."  Where (n) is derived from average employee edit buffer screen
> sizes.

Depending on inspection to guarantee that the first arg to snprintf() won't
overflow is risky, because someone down the line might change the size of the
buffers in a header somewhere and invalidate the assumptions made in the inline
code review.

	snprintf(buf, sizeof(buf), ... );

...is always going to be safer.  Maybe I was overtrained about this matter, but
using sprintf() in code which runs as root against any dynamic input is like
using malloc() without checking for NULL.

> It is also the case that where snprintf() is used today, it results in
> truncated log lines (a matter of some controversy within ISC as to wether
> this should be allowed or not), but if and when used outside of logging
> format strings, an error is always asserted, and either an all-stop in
> the case where strings are not network-supplied, or the client is at
> least not replied to.

The syslog RFC defines a maximum length of 1024 bytes in section 4.1:

  http://www.faqs.org/rfcs/rfc3164.html

> This latter bit - what to do when it truncation ocurrs - is often
> ignored by the "strncpy/snprintf always!" school of thinking.  Not
> overrunning buffer is good - not overrunning buffer and producing
> invalid protocol as a consequence is bad.

If your protocol has length limitations, they should be enforced.

You should detect and log something would otherwise overrun the buffer as a
possibly malicious attempt to hack into the program, just as SMTP programs now
object to RFC-822/2822 headers longer than 1024 bytes as "potential
MIME-decoding attacks".

>> I try to make sure my code goes under FreeBSD, NetBSD, MacOSX/Darwin, Solaris,
>> Red Hat, and SuSE (not necessarily in that order), although I don't particularly
>> view it as writing code for those platforms so much as making sure I don't write
>> platform-specific code except when I really have to.
> 
> This thinking is dangerous.  "The code is portable, your system is the
> problem" is the disease that sometimes precipitates.
> 
> Only sometimes, mind you.
> 
> Ironically, ISC DHCP was built from this mindset, from what I read.
> The result is not very portable, and people trying to make it so create
> static machinations of #ifdef'ery that ultimately collapse the entire
> package into an obscure singularity.

Platform-specific #ifdef's are obviously not portable code, by definition.

The code I was thinking of had one platform-specific section for NetBSD, which
largely is no longer needed with PR standards/25058, out of ~7300 lines of C.
(It's a bespoke IDS that implements most of an IPv4 TCP/IP stack, and even
understands the DHCP protocol well enough to obtain a lease itself, as well as
monitor others being granted and the ARP traffic going by.)

% cat *.[hc] | wc -l
    7282
[ ... ]
#include <sys/types.h>
#include <sys/param.h>
#include <sys/stat.h>

#if defined(__NetBSD__)
#include <net/ethertypes.h>
#else
#include <net/ethernet.h>
#endif

#include <netinet/in.h>

/* NetBSD doesn't seem to define these... */
#if !defined(TH_ECE)
#define      TH_ECE  0x40
#endif
#if !defined(TH_CWR)
#define      TH_CWR  0x80
#endif

/* length of an ethernet datalink-layer header in octets */
#define DLH_EN (14)

#include <pcap.h>
#include <libnet.h>

...and that was it for the platform specific stuff.  Actually, let me correct
that, NetBSD 1.6 and the earlier 2.x betas didn't update PCAP, either:

/* This returns the length of the layer-2 data (ie, the offset to IP data)
 * based on the datalink type.
 */
static int
datalink_offset(int dltype) {
    switch (dltype) {
      case DLT_NULL:    return 4;
      case DLT_EN10MB:  return DLH_EN;
      case DLT_IEEE802: return 22;
      case DLT_ARCNET:  return 4; /* not sure */
      case DLT_SLIP:    return 16;
      case DLT_PPP:     return 24;
      case DLT_FDDI:    return 21;
      case DLT_ATM_RFC1483: return 8; /* not sure */
      case DLT_RAW:     return 0;
#if !defined(__NetBSD__)
      case DLT_LOOP:    return 4;
      case DLT_LINUX_SLL: return 16;
#endif
      default:
        logwarn("unknown/unsupported PCAP datalink type\n");
        return 0;
    }
}

I don't think anyone would be likely to use those last two cases any more than I
expect to test an ARCnet network, so this could probably go away too.

> The autoconf approach is the correct one, but this doesn't find these
> "niggling details" until they hit someone square in the jaw, after
> release...it just makes it easier to mop up the blood afterward.

Autoconf is a mixture of a portable configuration front end which is genuinely
useful, with too many assumptions that get built in at compile time (or ./config
time), rather than dynamicly when the program is run.  It also fosters an
endless series of tests like these:

checking size of char... 1
checking for short... yes
checking size of short... 2
checking for int... yes
checking size of int... 4
checking for long... yes
checking size of long... 4

...which encourage people to write non-portable code, rather than using C99
standard datatypes (int32_t, etc).  Autoconf asks far too many questions that
portable code should not need to have answered.

[ ... ]
>>> Up until this, to me, unusual news, I had been planning the future
>>> without FreeBSD in the picture at all.
>>>
>>> It's surprising to me to think that someone still uses our client
>>> software on that platform.
>> Surprises often happen when one chooses not to pay attention.
> 
> I see.
> 
> I should be reading freebsd-arch, freebsd-net (which I was on),
> freebsd-stable, and freebsd-current, just to make sure I catch
> everything ISC-DHCP related.

No, although your exaggeration is entertaining.  All you need to do is
coordinate with:

% cd /usr/ports/net/isc-dhcp3-server && make maintainer
Joerg.Pulz at frm2.tum.de

...assuming that you didn't want to maintain the software directly yourself.

Perhaps that should be put the other way around, as most maintainers of ports
are happy to address FreeBSD-specific issues themselves, and only pass things
upstream which are useful patches or problems they can't resolve locally themselves.

The happy relationship you said you had with Martin Blapp seems to be (or have
been) a fine example of this, although I guess that mblapp@ has gone offline or
AWOL.

[ ...a discussion involving Mohammed and mountains, snipped :-)... ]
>> Either you choose to subscribe to the BSD mailing lists, or review the bug
>> database for PR's every once in a while, or perhaps even act as the maintainer
>> of the ISC DHCP port, or you choose not to do so.
> 
> This is a case of black and white thinking.

Yes, I use first-order logic a fair amount, and I find it more useful than not,
even if some situations are more complicated than it can account for.

Even if the world has more shades of grey than black-and-white situations,
individual actions or choices tend to be distinctive and clear, at least when
people are honest with themselves....

Anyway, thanks for your response,
-- 
-Chuck