Re: Implement C11 annex K?

[Paul Eggert <eggert at cs dot ucla dot edu>]

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.