Bug 25219 - improve out-of-bounds checking with GCC 10 attribute access
Summary: improve out-of-bounds checking with GCC 10 attribute access
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: unspecified
: P2 enhancement
Target Milestone: 2.32
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-22 23:29 UTC by Martin Sebor
Modified: 2020-08-28 09:51 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Proof of concept patch. (2.23 KB, patch)
2019-12-18 19:57 UTC, Martin Sebor
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Sebor 2019-11-22 23:29:41 UTC
To help detect out-of-bounds accesses by third party functions, GCC 10 provides a new function attribute called access.  It associates pointer parameters with size parameters in function declarations so that actual arguments can be checked in calls to the functions to determine if they are in bounds.  Those that are not are diagnosed by GCC without changing the code (so unlike _FORTIFY_SOURCE it doesn't abort at runtime).

GCC has had the ability for some time to detect these invalid accesses by built-in functions and Glibc already has a mechanism to detect a subset of buffer overflows by functions it defines and that GCC doesn't recognize as built-ins.  But by relying on __builtin_object_size, the Glibc solution is limited to constant sizes and offsets.  The detection enabled by the new attribute doesn't have this limitation.

Making use of the new attribute in Glibc's APIs will significantly improve GCC's ability to detect buffer overflows and other bugs in ordinary functions (non-built-ins) defined by Glibc.  I have annotated a few of these functions with the attribute as a proof of concept to see how this might look in practice.  For instance, compiling the following example with GCC 10 and unmodified Glibc and -D_FORTIFY_SOURCE=2 only the buffer overflow in function f is diagnosed:

#include <string.h>
#include <unistd.h>

char a[32];

void f (int i)
{
  if (i < 27)
    return;

  strcpy (a + i, "123456");
}

int g (int fd, int i)
{
  if (i < 27)
    return -1;

  return read (fd, a + i, sizeof a);
}

In file included from ../include/bits/string_fortified.h:1,
                 from ../string/string.h:494,
                 from ../include/string.h:60,
                 from /build/tmp/t.c:1:
In function ‘strcpy’,
    inlined from ‘f’ at /build/tmp/t.c:11:3:
../string/bits/string_fortified.h:90:10: warning: ‘__builtin_memcpy’ forming offset [32, 33] is out of the bounds [0, 32] of object ‘a’ with type ‘char[32]’ [-Warray-bounds]
   90 |   return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/build/tmp/t.c: In function ‘f’:
/build/tmp/t.c:4:6: note: ‘a’ declared here
    4 | char a[32];
      |      ^


Applying the patch below lets the overflow in function g() also be diagnosed:

In function ‘read’,
    inlined from ‘g’ at /build/tmp/t.c:19:10:
../posix/bits/unistd.h:45:10: warning: ‘__read_alias’ writing 32 bytes into a region of size 5 overflows the destination [-Wstringop-overflow=]
   45 |   return __read_alias (__fd, __buf, __nbytes);
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../include/sys/cdefs.h:3,
                 from ../include/features.h:465,
                 from ../posix/sys/types.h:25,
                 from ../include/sys/types.h:1,
                 from ../include/string.h:6,
                 from /build/tmp/t.c:1:
../posix/bits/unistd.h: In function ‘g’:
../posix/bits/unistd.h:25:28: note: in a call to function ‘__read_alias’ declared with attribute ‘write_only (2, 3)’
   25 | extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf,
      |                            ^~~~~~~~~~~~
../misc/sys/cdefs.h:174:41: note: in definition of macro ‘__REDIRECT’
  174 | # define __REDIRECT(name, proto, alias) name proto __asm__ (__ASMNAME (#alias))
      |                                         ^~~~


diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index abcb0d5e3c..268d0ed1f2 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -522,4 +522,19 @@
 # define __HAVE_GENERIC_SELECTION 0
 #endif
 
+#if __GNUC_PREREQ (10, 0)
+/* Denotes a function pointer argument can be used to access an array
+   of N elements according to access mode.  */
+#  define __rdonly(...)  \
+  __attribute__ ((__access__ (__read_only__, __VA_ARGS__)))
+#  define __rdwr(...)  \
+  __attribute__ ((__access__ (__read_write__, __VA_ARGS__)))
+#  define __wronly(...) \
+  __attribute__ ((__access__ (__write_only__, __VA_ARGS__)))
+#else
+#  define __rdonly(...)
+#  define __rdwr(...)
+#  define __wronly(...)
+#endif
+
 #endif  /* sys/cdefs.h */
diff --git a/posix/bits/unistd.h b/posix/bits/unistd.h
index 103095b8e4..8e4b0f084e 100644
--- a/posix/bits/unistd.h
+++ b/posix/bits/unistd.h
@@ -21,9 +21,10 @@
 #endif
 
 extern ssize_t __read_chk (int __fd, void *__buf, size_t __nbytes,
-                          size_t __buflen) __wur;
+                          size_t __buflen) __wur __wronly (2, 3);
 extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf,
-                                         size_t __nbytes), read) __wur;
+                                         size_t __nbytes), read)
+  __wur __wronly (2, 3);
 extern ssize_t __REDIRECT (__read_chk_warn,
                           (int __fd, void *__buf, size_t __nbytes,
                            size_t __buflen), __read_chk)
Comment 1 Martin Sebor 2019-12-18 19:57:14 UTC
Created attachment 12133 [details]
Proof of concept patch.

The attached patch is a proof of concept implementation of the request for a subset of Glibc APIs, with the attribute hidden behind a non-variadic macro as Florian advised in the discussion below:

https://sourceware.org/ml/libc-alpha/2019-11/msg00779.html

I used __attr_access for the name to avoid conflicts with the function __access.

I only tested it by building Glibc and the testsuite with GCC 10 but I didn't look at the test results.  The proof of concept includes no tests or even a  ChangeLog entry so it's not quite ready but I include it here in case someone (e.g., Florian) has the interest and the cycles to finish before the upcoming Glibc release.  I'm away until Jan. 6 so I won't be able to work on it until sometime after that.
Comment 2 Martin Sebor 2020-04-30 22:13:42 UTC
An updated patch:
https://sourceware.org/pipermail/libc-alpha/2020-April/113503.html
Comment 3 Matheus Castanho 2020-05-20 13:39:01 UTC
This seems to have already been merged as 06febd8c6705c816b2f32ee7aa1f4c0184b05248 (though not reflected here).

I believe this is causing an issue when building binutils (I was not able to come up with a shorter reproducer yet, sorry):

/home/mscastanho/AT/next/tmp/build/at14.0-0-alpha.debian-10_ppc64le_ppc64le/sources/binutils/gas/read.c: In function ‘read_symbol_name’:
/home/mscastanho/AT/next/tmp/build/at14.0-0-alpha.debian-10_ppc64le_ppc64le/sources/binutils/gas/read.c:1687:11: error: argument 1 is null but the corresponding size argument 3 range is [128, 9223372036854775807] [-Werror=nonnull]
 1687 |       if (mbstowcs (NULL, name, len) == (size_t) -1)
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/mscastanho/AT/next/tmp/build/at14.0-0-alpha.debian-10_ppc64le_ppc64le/sources/binutils/gas/as.h:58,
                 from /home/mscastanho/AT/next/tmp/build/at14.0-0-alpha.debian-10_ppc64le_ppc64le/sources/binutils/gas/read.c:33:
/home/mscastanho/AT/next/opt/at-next-14.0-0-alpha/include/stdlib.h:933:15: note: in a call to function ‘mbstowcs’ declared with attribute ‘write_only (1, 3)’
  933 | extern size_t mbstowcs (wchar_t *__restrict  __pwcs,
      |               ^~~~~~~~
cc1: all warnings being treated as errors


So GCC is complaining because arg 1 of mbstowcs is NULL but arg 3 is a positive value. But from the function's manpage: "If dest [arg1] is NULL, n is ignored, and the conversion proceeds..."

So is there a chance this is a valid usage of the function that was not contemplated by this patch?
Comment 4 Martin Sebor 2020-05-20 16:02:08 UTC
The attribute has been added.

I added the attribute to most of the APIs based on the requirements in the standards (C11 and POSIX).  I also checked a few  for extensions described in the Glibc documentation and at least one in the Linux documentation project, but I don't think I looked in the latter for mbstowcs.

C11 requires the first argument to mbstowcs to be an array (i.e., non-null).  I don't see an extension documented in the Glibc manual to allow null, but the Linux man page (http://man7.org/linux/man-pages/man3/mbstowcs.3.html) does.

I'll leave it to the Glibc maintainers to decide what they want to do here.
Comment 5 Florian Weimer 2020-05-20 16:07:09 UTC
(In reply to Martin Sebor from comment #4)
> The attribute has been added.
> 
> I added the attribute to most of the APIs based on the requirements in the
> standards (C11 and POSIX).  I also checked a few  for extensions described
> in the Glibc documentation and at least one in the Linux documentation
> project, but I don't think I looked in the latter for mbstowcs.
> 
> C11 requires the first argument to mbstowcs to be an array (i.e., non-null).
> I don't see an extension documented in the Glibc manual to allow null, but
> the Linux man page (http://man7.org/linux/man-pages/man3/mbstowcs.3.html)
> does.

POSIX has an XSI extension which makes the NULL argument valid:

“
[XSI] [Option Start]  If pwcs is a null pointer, mbstowcs() shall return the length required to convert the entire array regardless of the value of n, but no values are stored. [Option End]
”

So this needs to be fixed.
Comment 6 Martin Sebor 2020-05-20 19:41:12 UTC
Thanks, I clearly missed the XSI extension.

The most straightforward fix to avoid the warning is to remove the attribute from mbstowcs.

Longer term though, unless it's declared with attribute nonnull, perhaps the attribute should allow the pointer to be null regardless of the size.  That's what it does now when the size argument isn't specified.  When the size is specified, the attribute it's modeled on functions like memcpy, but that may not be the most flexible model.
Comment 7 Florian Weimer 2020-05-20 19:43:55 UTC
(In reply to Martin Sebor from comment #6)
> Thanks, I clearly missed the XSI extension.
> 
> The most straightforward fix to avoid the warning is to remove the attribute
> from mbstowcs.
> 
> Longer term though, unless it's declared with attribute nonnull, perhaps the
> attribute should allow the pointer to be null regardless of the size. 
> That's what it does now when the size argument isn't specified.  When the
> size is specified, the attribute it's modeled on functions like memcpy, but
> that may not be the most flexible model.

It may already be possible today to implement the desired NULL behavior with an inline wrapper that calls different aliases as needed.
Comment 8 Jakub Jelinek 2020-08-28 09:33:24 UTC
getcwd has the same problem, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96832