Re: [-Wstringop-overflow=] strncat(3)

[Alejandro Colomar]

Hi Martin,

On 12/15/22 21:50, Martin Sebor wrote:
> On 12/14/22 16:14, Alejandro Colomar via Libc-alpha wrote:
[...]

> > > >            int
> > > >            main(void)
> > > >            {
> > > >                char    buf[BUFSIZ];
> > > >                size_t  len;
> > > >
> > > >                buf[0] = '\0';  // There’s no ’cpy’ function to this ’cat’.
> > > >                strncat(buf, "Hello ", 6);
>
> There's nothing wrong with this but the two lines above would be
> more simply coded as one:
>
>    strcpy(buf, "Hello ");

I used a string literal to avoid having to think hard of an example using 
utmpx(5), which would be the actual code for which I want users to use this 
function.

With time, I expect to develop such example program.

> The original code suggests a misunderstanding of strncpy's purpose:

By the "original code" what are you exactly referring to?  The code quoted 
above, or another?

In any case, the code is using strncat(3) not strncpy(3).

> that it writes exactly 6 bytes into the destination.

That's the purpose of strncpy(3).  I'll quote the versions of the pages that I'm 
proposing:


strncpy(3):
       These functions copy the string pointed to by src  into  a  null‐padded
       character sequence at the fixed‐width buffer pointed to by dst.  If the
       destination buffer, limited by its size, isn’t large enough to hold the
       copy,  the resulting character sequence is truncated.  They only differ
       in the return value.


The description above says that the destination buffer is limited by its size, 
so as you say, this function copies exactly 6 bytes (the character sequence + 
null padding).


strncat(3)
       This  function  catenates  the  input character sequence contained in a
       null‐padded fixed‐width buffer, into a string at the buffer pointed  to
       by  dst.   The  programmer is responsible for allocating a buffer large
       enough, that is, strlen(dst) + strnlen(src, sz) + 1.


But strncat(3) writes whatever is the length of the character sequence passed as 
input (ignoring the padding null bytes), plus a terminating null byte.


>   That's what
> the warning points out.

The warning would be fair for strncpy(3), but not for strncat(3).

> > > >                strncat(buf, "world", 42);  // Padding null bytes ignored.
> > > >                strncat(buf, "!", 1);
> > > >                len = strlen(buf);
> > > >                printf("[len = %zu]: <%s>\n", len, buf);
> > > >
> > > >                exit(EXIT_SUCCESS);
> > > >            }
> > > >
> > > >     SEE ALSO
> > > >            string(3), string_copy(3)
> > > >
> > > >     Linux man‐pages (unreleased)      (date) strncat(3)
> > > >
> > > >
> > > > And when you compile that, you get:
> > > >
> > > > $ cc -Wall -Wextra ./strncat.c
> > > > ./strncat.c: In function ‘main’:
> > > > ./strncat.c:12:12: warning: ‘strncat’ specified bound 6 equals source length
> > > > [-Wstringop-overflow=]
> > > >      12 |            strncat(buf, "Hello ", 6);
> > > >         |            ^~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ./strncat.c:14:12: warning: ‘strncat’ specified bound 1 equals source length
> > > > [-Wstringop-overflow=]
> > > >      14 |            strncat(buf, "!", 1);
> > > >         |            ^~~~~~~~~~~~~~~~~~~~
> > > >
> > > >
> > > > So, what?  Where's the problem?  This function does exactly that: "take an
> > > > unterminated character sequence and catenate it to an existing string".
>
> Strncat has historically had two distinct use cases.  One of them
> -- to constrain the amount of data to copy to the space remaining
> in the destination -- gained popularity with the push to reduce
> buffer overflow weaknesses in code.  Mistakes in these uses gave
> rise to a whole other class of security bugs, to the extent that
> CERT felt it necessary to publish the strncpy and strncat best
> practice.  The GCC warning in turn was added to support the CERT
> guideline.  I touch on some of this in a blog post I wrote a few
> years ago:
>
> https://developers.redhat.com/blog/2018/05/24/detecting-string-truncation-with-gcc-8

While this is true, that's because there was nothing more appropriate, such as 
strlcat(3).

My point is that strncat(3) is far from being a good tool for that, and should 
never be used for that purpose in good code; especially now that we have 
strlcat(3) widely available in most if not all POSIX systems (in Linux through 
libbsd).

I'll quote some part of the link above:

|    strncat (dest, src, dest_size - strlen (dest) - 1);
|
| Calls that have this form are not diagnosed.

That code is a source of bugs.  Such code should _always_ be replaced by 
strlcat(3) as:

    strlcat(dst, src, sizeof(dst);


| Other calls, such as those where
| the size is derived in some way from the size or length of the source string,
| are diagnosed by -Wstringop-overflow. That includes unsafe calls like

The only sane use of strncat(3) is that.  It's a use where no other function 
serves better.  To read a possibly non-terminated character sequence from a 
fixed-width buffer; and the size is the only way strncat(3) can know when to 
stop reading.

|
|    strncat (dest, src, strlen (src));   // incorrect - warning

This call doesn't make any sense; I agree.  It should _always_ be replaced by 
strcat(3) as:

    strcat(dst, src);

I acknowledge that this is the case I show in the manual page, but only because 
there is no form to create an unterminated string literal (I take this 
opportunity to re-suggest "foo"u as an unterminated string literal).

I'll try to come up with an example program that avoids this.

|
| and
|
|    strncat (dest, src, sizeof src);   // incorrect - warning

This is the _perfect_ use case for strncat(3).  No other function is better.

Let's say you have the following:

char src[5] = {'h', 'e', 'l', 'l', 'o'};

How do you copy that into a string?  strncat(3).  utmpx(5) is one of the 
examples where this happens.

I'll modify the example program to use this, to avoid being misleading.  I'll 
replace string literals by character sequences.


> The specific uses of the function above are contrived (there's
> no point in calling strncat to append the full string -- strcat
> will do that more clearly and efficiently) but the general use
> case -- limiting the amount of copied data to an initial
> substring of the source sequence -- although valid and originally
> intended (it's one of the two uses of the function in UNIX v7),
> is not one that either the guideline or the warning consider.
> They can only consider one use cases, and they chose the one
> that was observed behind security bugs. 

It's sad that strncat(3) has been used so much for limiting the dest copy.  It's 
pretty unsafe for that, we agree.  How about allowing the safe use of it (i.e., 
size derived from source buffer), and warn about uses where the size is derived 
from the dst string?  I'm going to rewrite the manual page in that direction.

> That choice unavoidably
> leads to some false positives.  The expected way to deal with
> them is to suppress the warning by one of the usual mechanisms
> (command line option or #pragma GCC diagnostic).

The only use case that GCC is allowing is better served by strlcpy(3) and 
strlcat(3).  I suggest changing the warning to protect the only natural use case 
for strncat(3), and warn about uses of it supplanting strlcat(3) (and maybe 
suggest using strlcat(3)).

> Martin

Thanks!

Alex

--
<http://www.alejandro-colomar.es/>

OpenPGP_signature
Description: OpenPGP digital signature