dhclient in 6.0

[David W. Hankins]

On Thu, Feb 09, 2006 at 12:50:08PM -0500, Chuck Swiger wrote:
> 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.

If we get enough demand for zeroconf, or funding for development, or
contributed patches, we can entertain it.  Inarguably, such support
does belong in dhclient since you really only want to entertain
zeroconf when dhcp fails, consistently (and also doesn't supply the
relevant new options in DHCPNAK that tell clients not to entertain
zeroconf).

Doing address conflict detection just to probe the offered address in
DHCP is worthwhile, learning to do ARP also lets us pick up on Bernard
Aboba's DNAv4 draft, which (did I say this already?) I must admit I
have a fondness for, a desire to implement.  If a complete lack of
demand from users (so this remains a far, far future).


I think that exhausts your last DHCP related query that might reasonably
have been a part of this thread.  We should probably move the rest of
this off-list, if at all.

I certainly don't think I possess any more new information that I can
usefully impart to you.


> Depending on inspection to guarantee that the first arg to snprintf() won't
> overflow is risky,

As yet, depending upon gravity to remain constant within a certain
range of the Earth has not turned out to be a great risk.

Somehow my car still adheres to the parking lot outside our office
despite lack of inspection of the current gravitational field around
it.

If you stipulate that risk exists within a wholly deterministic system,
I think that act is to reject science.

"This code will either overflow buffer, or it won't" is identical to
the statement that "my car will either stay on the ground, or float
out into outer space."

I know all the variables in either circumstance, science, the two
different sciences in either case, demands there is only one possible
outcome.

So, there is no need to rope my car down to the parking lot.

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

In ISC's case, all changes to these software packages undergo peer
review before being applied and, as I mentioned, cases where these
functions are permitted to be used is limited to those situations
where review is doable within edit buffer (ideally, within diff -u
context).

If headers enter into it, then that clearly is an improper use of the
function, according to our policy, as that requires too much review.

Prior to ever reaching a point of review where the sizes of these
variables change unexpectedly, that use of these functions would not
have been allowed.


I suspect you may not realize this, so I'll point it out:  You are
arguing with ISC's policy, to which as an employee I must adhere,
not me.


> 	snprintf(buf, sizeof(buf), ... );
> 
> ...is always going to be safer.  Maybe I was overtrained about this matter, but

Unless someone changes 'buf' from an array to a pointer.  Depending on
architecture, this may not get detected by 'make test' until someone
in the field provides input that exceeds 4 or 8 bytes.  The result,
failing to check the return code of snprintf() in your example, would
almost certainly produce invalid (truncated) output.

In network protocols, BIND8 and BIND4 have taught us that this is
simply not acceptable behaviour.  It is better for the daemon to fail
than to continue along producing bad output.


Black and white thinking again I fear.  "You either always check
bounds or it's just as bad as if you never do" is a longer
summarization of the school of thought I have earlier summarized as
"the 'snprintf/strncpy always!' crowd".

I submit that this example use of sprintf():

	char buf[sizeof("255.255.255.255")];

	sprintf(buf, "%u.%u.%u.%u",
	        a & 0xff, b & 0xff, c & 0xff, d & 0xff);

Is identically secure to:

	char buf[sizeof("255.255.255.255")];

	INSIST(snprintf(buf, sizeof(buf), "%u.%u.%u.%u",
			a & 0xff, b & 0xff, c & 0xff, d & 0xff) < sizeof(buf));

The top strawman is acceptable use of sprintf() because it exhibits
ISC's policy behaviour: use of sprintf() is auditably secure within 4
lines.  Changes to anything that is to produce a change in security
in this invocation of sprintf() will be viewable by the reviewer
without having to visit external sources.

Note that if 'buf' were declared more than a page earlier than the
sprintf call, it would not be allowed.

The bottom strawman is not, per policy, unacceptable, but through
BIND9 ISC has adopted a unique perspective on the results of applying
overbearing levels of sanity checks, and a peer reviewing that use of
code would probably want to know how often it was expected to be
invoked (initialization, shutdown, on what kinds of events, etc).

In short, the use of snprintf() is actually a greater burden upon
code reviewers than the use of sprintf() within policy restrictions.


So, in point of fact, snprintf() is not "always safer than sprintf()."
There exist conditions where the level of safety is identical, and all
you're doing is giving the machine extra work in bounds checks that
are impossible to exceed, or error handlers that will never be
utilized.


I really doubt you're going to reach agreement with ISC's policy.

I merely was attempting to illustrate the policy to which I am
implementing.  A policy designed to address the concerns contained
within your query exists, and that we abide by it.

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

Right.  I'm clearly not being clear enough as you are sourcing
information that is irrelevant.  Sorry, I'll start over.

Let us imagine someone from the "always use snprintf/strncpy" camp
produced a patch that simply did "s/sprintf/snprintf/" (we get maybe
one of these every year along with a report to CERT or similar that
ultimately gets closed for having no findings).  That is, the return
value of snprintf() was never examined to detect the case where the
formatted data was truncated to throw an error, the function was
merely replaced and the bounds was added as a calling parameter.  Eg
every invocation of this type of full line of code:

	sprintf(s, "%u", val);

Was turned into to this full line of code:

	snprintf(s, sizeof(s), "%u", val);

This behaviour is improper because truncation, if possible, produces
invalid output, wether it be in-protocol or not.  According to the ISC
policy on the subject this is not allowed.  That is:  If by switching
from sprintf->snprintf you have avoided any possible case where you
might have a security problem, then you have also now created a case
where you produce invalid output...unless you check snprintf's return
value as well, and appropriately handle the error.  The latter part of
that might make a simple function substantially more complex.


The point I was trying to make was that specifically in the case of
syslog() inputs, we have fuzzied this line, so as to make such
truncation permissible by policy.

Substantively, this implies that syslog() output is intended to be
human readable (where human brains are capable of detecting and
realizing the source problem where truncation occurs), not machine
readable (where a machine is liable to produce invalid output if
the input is invalid).


I was also trying to point out that this is a subject often never even
broached by people from that camp ("what to do when truncation
happens").  Evidenced by the 's/sprintf/snprintf/' patches they send
us, and their noted lack of error handling.

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

That is not merely to what I am referring.  I hope I re-explained
this adequately above.

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

Yet, I stipulate, they are the direct result of such thinking when applied
to open source projects.

You gave us an example where you produced a reasonable single use of
#ifdef.  It starts there, with one exception in a list of 7 operating
systems (ISC DHCP actually has a list of 15).  Then you find an eighth
platform, patches donated from a user who needs the software on another
platform, and it brings another oddity.  Then someone whose boss has
mandated the use of their favorite platform (SCO anyone?) requires an
admin to make it work on his platform...so they pay us to do that.
Then you get another oddity when that work is released publically.

I have witnessed this approach collapse modules into singularities,
unable to be observed by anyone other than the initial author lest
their eyes be ripped from their sockets and thrown into a pocket
dimension.

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

"Screwdrivers encourage eye removal."

Nothing new here.

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

This line of interrogation is no longer useful, as the lines of
communication I sought have been formed off-list.

But ask yourslf:  Why would I have entertained this given the
relationship with Martin I was already enjoying?

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

I choose to examine my conclusions when I find myself to be comitting
black-and-white arguments.  More often than not, they are fallacies.

A tool of Sophists.

There was a time in my past that I did not possess that sort of self
discipline, and participated enthusiastically in Sophist exchanges,
some on mailing lists, even Usenet.

It was, if nothing else, great fun.

But we never got any closer to the truth.

-- 
David W. Hankins		"If you don't do it right the first time,
Software Engineer			you'll just have to do it again."
Internet Systems Consortium, Inc.		-- Jack T. Hankins
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-stable/attachments/20060228/44880b38/attachment.bin