[PATCH] debug: State buffer overflow message more precisely

[Adhemerval Zanella Netto]

On 07/07/24 13:23, Ingo Blechschmidt wrote:
> This patch contributes to our source fortification endeavors by
> slightly rewording the error message on detecting a probable buffer
> overflow. The new error message adds a bit of nuance, helping to
> demarcate the efforts of source fortification from an unmitigated
> actual buffer overflow, thereby describing the situation more precisely,
> preventing misunderstandings and highlighting source fortification.
> 
> I ran into this issue when debugging a problem with Privoxy
> (https://github.com/NixOS/nixpkgs/issues/265654). Source fortification
> correctly identified a libc call which was potentially problematic and
> (in my view) misleading, but actually safe. A more precise error
> message, as proposed by this patch, would have sped up the diagnosis.

>From the bug report, the code does seems to be issuing UB and compiler itself
warns about. With a reduced testcase:

---
$ cat t.c
#define _GNU_SOURCE
#include <string.h>

#define BUFFER_SIZE 5000

static const char socks_userid[] = "anonymous";

struct socks_op {
   unsigned char vn;          /* socks version number */
   unsigned char cd;          /* command code */
   unsigned char dstport[2];  /* destination port */
   unsigned char dstip[4];    /* destination address */
   char userid;               /* first byte of userid */
   char padding[3];           /* make sure sizeof(struct socks_op) is endian-independent. */
   /* more bytes of the userid follow, terminated by a NULL */
};

void foo (void)
{
  char buf[BUFFER_SIZE];
  struct socks_op    *c = (struct socks_op    *)buf;
  strlcpy(&(c->userid), socks_userid, sizeof(buf) - sizeof(struct socks_op));
}
$ gcc -Wall t.c -c -O2 -D_FORTIFY_SOURCE=2
t.c: In function ‘foo’:
t.c:22:3: warning: ‘strlcpy’ writing 4988 bytes into a region of size 1 overflows the destination [-Wstringop-overflow=]
   22 |   strlcpy(&(c->userid), socks_userid, sizeof(buf) - sizeof(struct socks_op));
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
t.c:13:9: note: destination object ‘userid’ of size 1
   13 |    char userid;               /* first byte of userid */
      |         ^~~~~~
In file included from /usr/include/features.h:502,
                 from /usr/include/x86_64-linux-gnu/bits/libc-header-start.h:33,
                 from /usr/include/string.h:26,
                 from t.c:2:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:150:1: note: in a call to function ‘strlcpy’ declared with attribute ‘access (write_only, 1, 3)’
  150 | __NTH (strlcpy (char *__restrict __dest, const char *__restrict __src,
      | ^~~~~
---

And I don't think changing the error message improves anything here,
if __chk_fail is called the program will be terminated anyway.  If you
think this is a false-positive, we will need to check whether the compiler
expansion for the strlcpy is being correct (meaning that __glibc_objsize
is passing the expected values), not to paper over the log.

> ---
>  debug/chk_fail.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/debug/chk_fail.c b/debug/chk_fail.c
> index 77d54c6706..44f367deec 100644
> --- a/debug/chk_fail.c
> +++ b/debug/chk_fail.c
> @@ -25,6 +25,6 @@ void
>  __attribute__ ((noreturn))
>  __chk_fail (void)
>  {
> -  __fortify_fail ("buffer overflow detected");
> +  __fortify_fail ("probable buffer overflow detected");
>  }
>  libc_hidden_def (__chk_fail)