debug: State buffer overflow message more precisely

Message ID ZorA48gZxSHgPttS@quasitopos
State Rejected
Headers
Series debug: State buffer overflow message more precisely |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Test passed

Commit Message

Ingo Blechschmidt July 7, 2024, 4:23 p.m. UTC
  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.
---
 debug/chk_fail.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Adhemerval Zanella Netto July 11, 2024, 2:34 p.m. UTC | #1
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)
  
Ingo Blechschmidt July 11, 2024, 8:16 p.m. UTC | #2
Dear Adhemerval,

thank you for your detailed review of my proposed patch! Much
appreciated.

On Thu Jul 11 11:34:45 2024, Adhemerval Zanella Netto wrote:
> 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.

I'm not sure I follow.

In my view, source fortification correctly identified a potentially
problematic call in Privoxy. But closer inspection shows that the call
is actually safe, hence strictly speaking it was a false positive.

But I wouldn't blame source fortification: It sure looks like a true
positive! &(c->userid) is indeed a pointer to a region of just size 1.
How should source fortification know that, at runtime, the memory
directly proceeding c->userid is allocated as well?

Privoxy could help source fortification by using a proper variable
length array instead of faking it. But still, I believe the Privoxy code
to be correct and hence the abort to be erroneous. As it's safer to
erroneously abort instead of erroneously continuing, I am all in favor
of the currect practice of aborting the program. I just thought that the
error message could be made more precise.

Please correct me if I'm mistaken: I got the impression that the purpose
of source fortification is to abort not only in cases where a buffer
overflow would undoubtedly occur, but also in cases where it's very
reasonable to believe that a buffer overflow is about to occur.

Cheers,
Ingo
  
Adhemerval Zanella Netto July 11, 2024, 8:44 p.m. UTC | #3
On 11/07/24 17:16, Ingo Blechschmidt wrote:
> Dear Adhemerval,
> 
> thank you for your detailed review of my proposed patch! Much
> appreciated.
> 
> On Thu Jul 11 11:34:45 2024, Adhemerval Zanella Netto wrote:
>> 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.
> 
> I'm not sure I follow.
> 
> In my view, source fortification correctly identified a potentially
> problematic call in Privoxy. But closer inspection shows that the call
> is actually safe, hence strictly speaking it was a false positive.
> 
> But I wouldn't blame source fortification: It sure looks like a true
> positive! &(c->userid) is indeed a pointer to a region of just size 1.
> How should source fortification know that, at runtime, the memory
> directly proceeding c->userid is allocated as well?
> 
> Privoxy could help source fortification by using a proper variable
> length array instead of faking it. But still, I believe the Privoxy code
> to be correct and hence the abort to be erroneous. As it's safer to
> erroneously abort instead of erroneously continuing, I am all in favor
> of the currect practice of aborting the program. I just thought that the
> error message could be made more precise.> 
> Please correct me if I'm mistaken: I got the impression that the purpose
> of source fortification is to abort not only in cases where a buffer
> overflow would undoubtedly occur, but also in cases where it's very
> reasonable to believe that a buffer overflow is about to occur.

The problem is once you add UB in the mix, there is no much the compiler
can do to help the fortification.  The object size passed along on the
fortify wrappers through __builtin_object_size/__builtin_dynamic_object_size
required well-behave code, if you start to using alias violation to make
clever hacks there no much compiler or runtime can do.

So sorry, but this patch does not make any sense.
  
Cristian Rodríguez July 12, 2024, 2:34 a.m. UTC | #4
On Sun, Jul 7, 2024 at 12:23 PM Ingo Blechschmidt <iblech@speicherleck.de>
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.
>

Ingo, all these error messages you are trying to correct are very
unfriendly and vague, adding a word will not do much to address that.
__chk_fail is generic for "Better error messages were never written".


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


The code does indeed UB and then all bets are off..
  

Patch

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)