[Bug string/31332] New: Improve detection of buffer overflow at compile-time with FORTIFY_SOURCE

[Bug string/31332] New: Improve detection of buffer overflow at compile-time with FORTIFY_SOURCE

[Vojislav.Tomasevic at Syrmia dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31332

            Bug ID: 31332
           Summary: Improve detection of buffer overflow at compile-time
                    with FORTIFY_SOURCE
           Product: glibc
           Version: unspecified
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: string
          Assignee: unassigned at sourceware dot org
          Reporter: Vojislav.Tomasevic at Syrmia dot com
  Target Milestone: ---

Created attachment 15350
  --> https://sourceware.org/bugzilla/attachment.cgi?id=15350&action=edit
Test case with buffer overflow in memcpy call

FORTIFY_SOURCE currently reports run-time errors when detecting buffer
overflows, both with clang and gcc. However, it would be more beneficial to
catch the issues earlier (at compile-time), when possible.

There is room for improvement in fortified implementations of functions
memcpy/memmove/memset/strncpy/bcopy/bzero as buffer overflows can be detected
at compile-time and reported as compile-time errors.

Consider the example memcpy.c in the attached test case which contains buffer
overflow:


bash-4.4$ clang -O2 -D_FORTIFY_SOURCE=2 memcpy.c   // no compile-time warning
bash-4.4$ ./a.out
*** buffer overflow detected ***: terminated
Aborted (core dumped)


Note that the overflow is caught at run-time only. However, in this case, we
should be able to detect it at compile-time as both the length and size of the
destination pointer is known at compile-time, when compiled with optimizations.

With changes to memcpy definition as below, the issue can be caught at
compile-time itself. Similar changes could be done to
memmove/memset/strncpy/bcopy/bzero functions as well. Both clang and gcc
compilers support error/warning attribute, builtin_object_size and
builtin_constant_p functions.


@@ -26,6 +26,13 @@ __fortify_function void *
 __NTH (memcpy (void *__restrict __dest, const void *__restrict __src,
               size_t __len))
 {
+  if (__bos (__dest) != (size_t) -1
+      && __builtin_constant_p (__len)
+      && __len > __bos (__dest))
+    {
+      void __fortify_error (void) __attribute__((error("dest is too small")));
+      __fortify_error ();
+    }
   return __builtin___memcpy_chk (__dest, __src, __len,
                                 __glibc_objsize0 (__dest));
 }


The above patch could be improved by using _errordecl macro to declare the
prototype of the __fortify_error function, which is already used in glibc for
similar purposes.

If the attached test case is considered now (after applying this patch), there
is a compile-time error like the following one:


bash-4.4$ clang -O2 -D_FORTIFY_SOURCE=2 memcpy.c
In file included from memcpy.c:1:
In file included from string.h:535:
glibc/install_dir/include/bits/string_fortified.h:34:7: error: call to
'__fortify_error' declared with 'error' attribute: dest is too small
   34 |       __fortify_error ();
      |       ^
1 error generated.


If this is agreeable, I would be interested to work on a patch which improves
buffer overflow detection at compile-time.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/31332] Improve detection of buffer overflow at compile-time with FORTIFY_SOURCE

[schwab@linux-m68k.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31332

--- Comment #1 from Andreas Schwab <schwab@linux-m68k.org> ---
It needs to be careful to not introduce false positives or flag uses that are
never executed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/31332] Improve detection of buffer overflow at compile-time with FORTIFY_SOURCE

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=31332

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/31332] Improve detection of buffer overflow at compile-time with FORTIFY_SOURCE

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31332

--- Comment #2 from Florian Weimer <fweimer at redhat dot com> ---
Comment on attachment 15350
  --> https://sourceware.org/bugzilla/attachment.cgi?id=15350
Test case with buffer overflow in memcpy call

Current GCC already warns about this:

#include <string.h>

__attribute__ ((weak))
void use (void *)
{
}

int main() {
  char buffer[5];
  char *src = "Hi guys";

  memcpy(buffer, src, strlen(src));
  use(buffer);

  return 0;
}

memcpy.c: In function ‘main’:
memcpy.c:12:3: warning: ‘memcpy’ forming offset [5, 6] is out of the bounds [0,
5] of object ‘buffer’ with type ‘char[5]’ [-Warray-bounds=]
   12 |   memcpy(buffer, src, strlen(src));
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
memcpy.c:9:8: note: ‘buffer’ declared here
    9 |   char buffer[5];
      |        ^~~~~~

This can be turned into an error with -Werror=array-bounds. The advantage is
that GCC can provide some helpful context about buffer sizes and offsets, which
we can do from a header with an inline wrapper function.

The issue is that with your original test case is that the memcpy call is
already gone at the point when such warnings are generated.

(What's missing is a GCC compilation mode where operations on a pointer that
cannot be bounds-checked fail to compile, but to be useful, that would have to
cover pointer arithmetic as well, so a header-only solution doesn't help with
that, either.)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/31332] Improve detection of buffer overflow at compile-time with FORTIFY_SOURCE

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=31332

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.