Re: Implement C11 annex K?

[Rich Felker <dalias@libc.org>]

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 <eggert@cs.ucla.edu> 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