From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18126 invoked by alias); 18 Aug 2014 08:03:43 -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 18112 invoked by uid 89); 18 Aug 2014 08:03:42 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: smtp.cs.ucla.edu Message-ID: <53F1B352.3010207@cs.ucla.edu> Date: Mon, 18 Aug 2014 08:03:00 -0000 From: Paul Eggert User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: dwheeler@dwheeler.com, libc-alpha Subject: Re: Implement C11 annex K? References: In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2014-08/txt/msg00277.txt.bz2 David A. Wheeler wrote: > If strlcpy/strlcat were truly security disasters, or unhelpful, > you'd expect their use in OpenSSH to have disappeared by now I wouldn't expect that at all. 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. I looked at the five examples you discussed, and the results are striking. Details at the end of this email. Here's a summary: * auth.c:486's use of strlcpy can lead to undefined behavior. * authfd.c:107's use of strlcpy is silently truncating data in a context where the caller expects an error return. * In authfile.c:1179-1180, snprintf is clearly preferable. * In auth-pam.c:742, strcpy would be simpler. * In addrmatch.c:321, strlen + strcpy would be clear, snprintf would also be OK, and strlcpy doesn't fix any bugs or make the code significantly clearer. So in your five examples, the use of strlcpy (A) has not fixed any bugs, (B) has not made the code significantly clearer, (C) is involved with one bug, and (D) has possibly caused another bug due to silent truncation. This is a worse result than from my quick perusal of OpenSSH a dozen years ago. If this is the best evidence-based argument *for* strlcpy, imagine what the argument *against* it would look like! Here are details for the above analysis. > 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; 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); Regardless of the form one prefers, the use of strlcpy here does not fix any bugs or make the code significantly clearer, compared to using standard functions. > auth.c:486: > strlcpy(buf, cp, sizeof(buf)); > ... So.. do you really believe that MAXPATHLEN really is the max length? It's not a matter of belief. It's obvious from the code that sets 'cp', four lines earlier. Worse, this use of strlcpy has undefined behavior when cp points into buf. A fix would be: memmove(buf, cp, strlen(cp) + 1); > authfd.c:107: > strlcpy(sunaddr.sun_path, authsocket, sizeof(sunaddr.sun_path)); > ... Truncation isn't checked... but it's not clear what else you > could do when truncation occurs. No, it's quite clear. You could return -1, which is what strlcpy's caller is supposed to do on failure. Here, strlcpy might be contributing to a bug, and it certainly isn't helping: a programmer who had used strlen + strcpy would likely have done better here and returned -1 on overlong inputs. > authfile.c:1179-1180: > if ((strlcpy(file, filename, sizeof file) < sizeof(file)) && > (strlcat(file, ".pub", sizeof file) < sizeof(file)) && > (key_try_load_public(pub, file, commentp) == 1)) > return pub; > ... > I would use snprintf in this case Agreed. > auth-pam.c:742: > mlen = strlen(msg); > ... > len = plen + mlen + 1; > **prompts = xrealloc(**prompts, 1, len); > strlcpy(**prompts + plen, msg, len - plen); > plen += mlen; > .... > Advantage strlcpy, due to a philosophical preference I'm afraid that veers too closely to "I like strlcpy because I like strlcpy". strlcpy does not fix any bugs here compared to strcpy, and this was the point I originally made. And strcpy would be simpler here.