| Message ID | ZorA48gZxSHgPttS@quasitopos |
|---|---|
| State | New |
| Headers | show |
| Series | debug: State buffer overflow message more precisely | expand |
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)
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
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.
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..
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)