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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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/>

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Kernel Documentation]     [Netdev]     [Linux Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux