Date: Tue, 28 Feb 2006 16:16:26 -0800 From: "David W. Hankins" <David_Hankins@isc.org> To: Chuck Swiger <cswiger@mac.com> Cc: freebsd-stable@freebsd.org Subject: Re: dhclient in 6.0 Message-ID: <20060301001626.GM21419@isc.org> In-Reply-To: <43EB80D0.2090303@mac.com> References: <d8a4930a0602030429q50f6aa39o@mail.gmail.com> <54176.24.90.33.115.1138971301.squirrel@mail.el.net> <20060203130241.GJ44469@pegasus.dyndns.info> <c7aff4ef0602030524y3a2632d3w@mail.gmail.com> <60155.24.90.33.115.1138981486.squirrel@mail.el.net> <61418.24.90.33.115.1139004207.squirrel@mail.el.net> <20060204005501.GD7613@isc.org> <43E44BAD.50601@mac.com> <20060206225310.GA16149@isc.org> <43EB80D0.2090303@mac.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--fCcDWlUEdh43YKr8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 09, 2006 at 12:50:08PM -0500, Chuck Swiger wrote: > If you haven't read this: >=20 > http://files.stuartcheshire.org/draft-cheshire-ipv4-acd.txt >=20 > ...it's worth considering the way it standardizes (or proposes to standar= dize) > how ARP traffic from unconfigured IPs should be done to avoid flooding la= rge > networks, or polluting the ARP caches with traffic from unconfigured host= s. 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), ... ); >=20 > ...is always going to be safer. Maybe I was overtrained about this matte= r, 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 weth= er > > 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. >=20 > 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 =66rom 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. >=20 > 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 C= 99 > 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. > >=20 > > 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. >=20 > 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. >=20 > Yes, I use first-order logic a fair amount, and I find it more useful tha= n 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. --=20 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 --fCcDWlUEdh43YKr8 Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (GNU/Linux) iD8DBQFEBOfZcXeLeWu2vmoRAh5vAKC2Nlk2+JMgwuLaJGY9LBH8o39AuACgnuVT f0SXKqMXiHnCdbI3S0lM72Q= =PaNQ -----END PGP SIGNATURE----- --fCcDWlUEdh43YKr8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20060301001626.GM21419>