[PATCH] debug: State buffer overflow message more precisely

[PATCH] debug: State buffer overflow message more precisely

[Ingo Blechschmidt]

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(-)

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)
-- 
2.45.1

Re: [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)

Re: [PATCH] debug: State buffer overflow message more precisely

[Ingo Blechschmidt]

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

Re: [PATCH] debug: State buffer overflow message more precisely

[Adhemerval Zanella Netto]

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.

Re: [PATCH] debug: State buffer overflow message more precisely

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]

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

Re: [PATCH] debug: State buffer overflow message more precisely

[Richard]

On 01/08/2024 14:54, Adhemerval Zanella Netto wrote:
> What I meant by UB is the expectation from the original program to call strlcpy
> over a 1-byte buffer and expects that strlcpy buffer-overrun the output with
> the input.
> 
> The fortify implementation was created*exactly*  to trigger on such usages,
> since they are a constant source of API misuse.  The strlcpy in that case
> could be replaced by a better call, like memcpy.

I do agree, it is API misuse and pretty bad coding and it is good to 
actually throw a hard error message with fortification there. So 
wouldn't it be best to print "buffer overflow or libc API misuse 
detected" then?

I do actually like that more than "probable buffer overflow". If desired 
I can prepare/send a patch for this change.

-- Richard

Re: [PATCH] debug: State buffer overflow message more precisely

[Adhemerval Zanella Netto]

On 31/07/24 19:58, Richard wrote:
>>
>> No, I am saying that once your code rely in UB there is no much compiler/libc
>> can help the caller.  I have described the usage the triggered this issue [1]
>> and clearly what caller passed to strlcpy is a buffer of 1-byte size and then
>> libc correctly reported this a buffer-overrun due the source argument.
> 
> I've thought a bit about this and I honestly don't think what you call UB here is undefined behaviour.
> 
> I've read the C standard a few years ago and as far as I remember the language does not have a concept of tracking buffer sizes. C has pointers, but there is no knowing/tracking of buffer sizes at the language level. So saying "calling strlcpy() with a 1 byte buffer (and all the other context of the original problem) is undefined behaviour" is no well-defined statement. Because UB is a C standard term but you are talking about compiler extensions/features here. Those are not in the standard.
> 
> Of course I might be wrong and I've read the C99 standard back then. If there is something in C17 or C23 I've missed, could you point me to the page in the standard that introduces the concept you're referencing when you say UB?
> 

What I meant by UB is the expectation from the original program to call strlcpy
over a 1-byte buffer and expects that strlcpy buffer-overrun the output with
the input. 

The fortify implementation was created *exactly* to trigger on such usages,
since they are a constant source of API misuse.  The strlcpy in that case
could be replaced by a better call, like memcpy. 

So I still think the libc implementation is correct to trigger a buffer 
overflow error at time of the callsize since it is all the information the 
API and compiler has provided indicates the call was done a buffer smaller 
that the required size.

Re: [PATCH] debug: State buffer overflow message more precisely

[Richard]

> 
> No, I am saying that once your code rely in UB there is no much compiler/libc
> can help the caller.  I have described the usage the triggered this issue [1]
> and clearly what caller passed to strlcpy is a buffer of 1-byte size and then
> libc correctly reported this a buffer-overrun due the source argument.

I've thought a bit about this and I honestly don't think what you call 
UB here is undefined behaviour.

I've read the C standard a few years ago and as far as I remember the 
language does not have a concept of tracking buffer sizes. C has 
pointers, but there is no knowing/tracking of buffer sizes at the 
language level. So saying "calling strlcpy() with a 1 byte buffer (and 
all the other context of the original problem) is undefined behaviour" 
is no well-defined statement. Because UB is a C standard term but you 
are talking about compiler extensions/features here. Those are not in 
the standard.

Of course I might be wrong and I've read the C99 standard back then. If 
there is something in C17 or C23 I've missed, could you point me to the 
page in the standard that introduces the concept you're referencing when 
you say UB?

Thanks,
-- Richard

Re: [PATCH] debug: State buffer overflow message more precisely

[Adhemerval Zanella Netto]

On 22/07/24 10:04, Richard wrote:
> Hi,
> 
>> 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.
> 
> After following this thread I must admit I don't understand this. Not the technical aspects, I'm an embedded kernel developer, that's fine, but the pragmatics/argument I don't get.
> 
> You're saying here, (paraphrased) "yes false positives are technically unavoidable therefore a log messages that informs users of probable false positives makes no sense"
> 
> And I really don't get that conclusion, could you explain that inference for me?
> 
> Thanks,
> -- Richard

No, I am saying that once your code rely in UB there is no much compiler/libc
can help the caller.  I have described the usage the triggered this issue [1]
and clearly what caller passed to strlcpy is a buffer of 1-byte size and then
libc correctly reported this a buffer-overrun due the source argument.

[1] https://sourceware.org/pipermail/libc-alpha/2024-July/158280.html

[PATCH] debug: State buffer overflow message more precisely

[Richard]

Hi,

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

After following this thread I must admit I don't understand this. Not 
the technical aspects, I'm an embedded kernel developer, that's fine, 
but the pragmatics/argument I don't get.

You're saying here, (paraphrased) "yes false positives are technically 
unavoidable therefore a log messages that informs users of probable 
false positives makes no sense"

And I really don't get that conclusion, could you explain that inference 
for me?

Thanks,
-- Richard

Re: [PATCH] debug: State buffer overflow message more precisely

[Adhemerval Zanella Netto]

On 22/07/24 09:41, Richard wrote:
> Hi all,
> 
> I've had a similar false positive a few weeks ago and just saw the post on the nixos github and this patch, would have saved me lots of debugging time. +1
> 
> Thanks,
> -- Richard

Which false positive to be more specific? Because this one was doing exactly
what is intended, since the called does issues strlcpy with a 1-byte size
buffer and relied on UB to get the buffer filled with data.

In any case, this change does not make much sense with this rationale.

[PATCH] debug: State buffer overflow message more precisely

[Richard]

Hi all,

I've had a similar false positive a few weeks ago and just saw the post 
on the nixos github and this patch, would have saved me lots of 
debugging time. +1

Thanks,
-- Richard