From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4480 invoked by alias); 22 Aug 2014 00:37:18 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 4458 invoked by uid 89); 22 Aug 2014 00:37:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.7 required=5.0 tests=AWL,BAYES_50,KAM_INFOUSME,RDNS_DYNAMIC,TVD_RCVD_IP autolearn=no version=3.3.2 X-HELO: brightrain.aerifal.cx Date: Fri, 22 Aug 2014 00:37:00 -0000 From: Rich Felker To: "David A. Wheeler" Cc: libc-alpha Subject: Re: Implement C11 annex K? Message-ID: <20140822003706.GM12888@brightrain.aerifal.cx> References: <53F1B352.3010207@cs.ucla.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2014-08/txt/msg00357.txt.bz2 On Thu, Aug 21, 2014 at 06:45:29PM -0400, David A. Wheeler wrote: > On Mon, 18 Aug 2014 01:03:30 -0700, Paul Eggert wrote: > > OpenSSH's authors have strongly > > advocated strlcpy and have much invested in it over the years. Even if > > they conceded that strlcpy is not that helpful now (admittedly > > unlikely), inertia would probably induce them to keep it. > > There are now *hundreds* of packages that use strlcpy/strlcat. > It is NOT just the original OpenSSH authors that use them. > Every package has to keep re-implementing them, typically in less-efficient > portable ways, because glibc fails to include them. This is exactly my reason for wanting them in glibc: I've repeatedly seen buggy re-implementations that probably introduce new security bugs. Even if these functions are bad (and I think they are), they're sufficiently famous that lots of people cargo-cult the API (but not the implementations) into their software, thereby introducing new security bugs. > Here's a list put together by deraadt, there are probably others: > http://marc.info/?l=openbsd-tech&m=138733933417096&w=2 Thanks for digging this up! > > > addrmatch.c:321: > > > ... The one-line snprintf version is this horror: > > > > That's because you wrote it in a horrible way. This is better: > > > > if (snprintf(addrbuf, sizeof addrbuf, "%s", p) >= sizeof addrbuf) > > return -1; > > The spec says snprintf can return <0, which this code fails to handle. > It's still not better, anyway; it sure isn't obvious that this is a string copy. > But to continue... This is simply wrong as I already addressed. The resulting type of the sizeof operator is size_t, and on all systems of any practical interest, size_t has integer conversion rank higher than (signed) int. Thus any negative value, after conversion, is >=INT_MAX, and in practice, >= the size of any struct. > > Though I wouldn't use snprintf here, as the following distinguishes the > > check from the action more clearly: > > > > if (strlen(p) >= sizeof addrbuf) > > return -1; > > strcpy(addrbuf, p); > > No. That wastes a lot of *people* time and is dangerous during maintenance. > In many projects, every strcpy() will (correctly) set off warning bells > requiring multiple people to *prove* that it can't exceed the buffer overflows. > And it's not just during writing the initial code. It's also easy to turn this kind > of code into a vulnerability when the code is maintained > (which is one reason this kind of code wastes a lot of people-time), > so every time the function is changed, people will have to re-check it, and > over time this can get painful. > > So every strcpy(), even if it's safe, increases *development* and *maintenance* time. > Developer and reviewer time is often MORE important than execution time > (even in C code, only some paths usually matter). > It's not TOO hard in this case to show that the strcpy cannot be > exceeded, sure, but that's not the end. For the most part I agree with this argument. I've used the same argument to reject use of sprintf as an "optimization" over snprintf even where it's easy to prove that sprintf would be safe. > > Worse, this use of strlcpy has undefined behavior > > when cp points into buf. > > I don't think so. strlcpy is required to copy the source left-to-right, since it stops at the \0, > and it's copying to the beginning of "buf", so I don't see an undefined behavior. Are you sure? For standard functions this is false; aside from memcpy, they are all documented with: "If copying takes place between objects that overlap, the behavior is undefined." Since this is the general pattern, I would treat use of strlcpy/strlcat with overlapping buffers as UB unless they're explicitly documented to support such usage. Note that, if they are documented to support overlapping buffers, this is another thing that re-implementors could easily get wrong. Rich