| Message ID | 20250314191240.1339994-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Accepted |
| Commit | ed6a68bac7cd056abda9008019c71b167f0362dc |
| Headers | show |
| Series | [v2] debug: Improve '%n' fortify detection (BZ 30932) | expand |
Hi Adhemerval, > The 7bb8045ec0 path made the '%n' fortify check ignore EMFILE errors > while trying to open /proc/self/maps, and this added a security > issue where EMFILE can be attacker-controlled thus making it > ineffective for some cases. > > The EMFILE failure is reinstated but with a different error > message. Also, to improve the false positive of the hardening for > the cases where no new files can be opened, the > _dl_readonly_area now uses _dl_find_object to check if the > memory area is within a writable ELF segment. The procfs method is > still used as fallback. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > -- > Changes from v1: > * Rename readonly-area-arch to readonly-area-fallback. > * Handle found RW areas on _dl_readonly_area. > * Use l_relro_addr/l_relro_size in check_relro. > * Change variable names in tst-sprintf-fortify-rdonly-mod.c. Thanks! There are three minor issues I've listed below, inline. TL;DR version: 1. The Makefile libsupport dependency can be dropped now. 2. The mach implementation function wasn't renamed with _fallback (but the file was). 3. The comment typo (segmetns). I sort of vaguely wrote "segments" in my earlier review but I meant the comment, rather than the error message. With these changes, the patch looks good to me: Reviewed-by: Arjun Shankar <arjun@redhat.com> Cheers, Arjun > --- > Makeconfig | 2 +- > debug/Makefile | 14 ++ > debug/readonly-area.c | 23 +-- > debug/tst-sprintf-fortify-rdonly-dlopen.c | 1 + > debug/tst-sprintf-fortify-rdonly-mod.c | 56 +++++++ > debug/tst-sprintf-fortify-rdonly.c | 158 ++++++++++++++++-- > elf/Makefile | 1 + > elf/dl-readonly-area.c | 86 ++++++++++ > elf/rtld.c | 1 + > include/stdlib.h | 15 ++ > stdio-common/vfprintf-internal.c | 9 +- > stdio-common/vfprintf-process-arg.c | 28 ++-- > sysdeps/generic/ldsodefs.h | 14 ++ > ...adonly-area.c => readonly-area-fallback.c} | 11 +- > ...adonly-area.c => readonly-area-fallback.c} | 21 +-- > 15 files changed, 375 insertions(+), 65 deletions(-) > create mode 100644 debug/tst-sprintf-fortify-rdonly-dlopen.c > create mode 100644 debug/tst-sprintf-fortify-rdonly-mod.c > create mode 100644 elf/dl-readonly-area.c > rename sysdeps/mach/{readonly-area.c => readonly-area-fallback.c} (90%) > rename sysdeps/unix/sysv/linux/{readonly-area.c => readonly-area-fallback.c} (84%) > > diff --git a/Makeconfig b/Makeconfig > index aa547a443f..bf50406656 100644 > --- a/Makeconfig > +++ b/Makeconfig > @@ -633,7 +633,7 @@ link-libc-printers-tests = $(link-libc-rpath) \ > $(link-libc-tests-after-rpath-link) > > # This is how to find at build-time things that will be installed there. > -rpath-dirs = math elf dlfcn nss nis rt resolv mathvec support misc > +rpath-dirs = math elf dlfcn nss nis rt resolv mathvec support misc debug > rpath-link = \ > $(common-objdir):$(subst $(empty) ,:,$(patsubst ../$(subdir),.,$(rpath-dirs:%=$(common-objpfx)%))) > else # build-static > diff --git a/debug/Makefile b/debug/Makefile > index 6a05205ce6..97d36438e8 100644 > --- a/debug/Makefile > +++ b/debug/Makefile > @@ -74,6 +74,7 @@ routines = \ > readlink_chk \ > readlinkat_chk \ > readonly-area \ > + readonly-area-fallback \ OK. Changed name. > realpath_chk \ > recv_chk \ > recvfrom_chk \ > @@ -179,9 +180,17 @@ CPPFLAGS-tst-longjmp_chk3.c += $(no-fortify-source) -D_FORTIFY_SOURCE=1 > CPPFLAGS-tst-realpath-chk.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 > CPPFLAGS-tst-chk-cancel.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 > CFLAGS-tst-sprintf-fortify-rdonly.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 > +CFLAGS-tst-sprintf-fortify-rdonly-mod.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 > +CFLAGS-tst-sprintf-fortify-rdonly-dlopen.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 > CFLAGS-tst-fortify-syslog.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 > CFLAGS-tst-fortify-wide.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 > > +$(objpfx)tst-sprintf-fortify-rdonly: \ > + $(objpfx)tst-sprintf-fortify-rdonly-mod.so \ > + $(objpfx)tst-sprintf-fortify-rdonly-dlopen.so > +LDLIBS-tst-sprintf-fortify-rdonly-mod.so = $(libsupport) > +LDLIBS-tst-sprintf-fortify-rdonly-dlopen.so = $(libsupport) > + The libsupport dependency can now be dropped. > # _FORTIFY_SOURCE tests. > # Auto-generate tests for _FORTIFY_SOURCE for different levels, compilers and > # preprocessor conditions based on tst-fortify.c. > @@ -302,6 +311,11 @@ tests-container += \ > tst-fortify-syslog \ > # tests-container > > +modules-names += \ > + tst-sprintf-fortify-rdonly-dlopen \ > + tst-sprintf-fortify-rdonly-mod \ > + # modules-names > + > ifeq ($(have-ssp),yes) > tests += tst-ssp-1 > endif > diff --git a/debug/readonly-area.c b/debug/readonly-area.c > index 04b437ed8d..a35ef9934f 100644 > --- a/debug/readonly-area.c > +++ b/debug/readonly-area.c > @@ -16,18 +16,19 @@ > <https://www.gnu.org/licenses/>. */ > > #include <stdlib.h> > +#include <ldsodefs.h> > > -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable. > - Return -1 if it is writable. */ > - > -int > +enum readonly_error_type > __readonly_area (const void *ptr, size_t size) > { > - /* We cannot determine in general whether memory is writable or not. > - This must be handled in a system-dependent manner. to not > - unconditionally break code we need to return here a positive > - answer. This disables this security measure but that is the > - price people have to pay for using systems without a real > - implementation of this interface. */ > - return 1; > + switch (GLRO(dl_readonly_area (ptr, size))) > + { > + case dl_readonly_area_rdonly: > + return readonly_noerror; > + case dl_readonly_area_writable: > + return readonly_area_writable; > + default: > + /* fallthrough */ > + } > + return __readonly_area_fallback (ptr, size); OK. dl_readonly_area now also returns an enum. We translate and return that. > } > diff --git a/debug/tst-sprintf-fortify-rdonly-dlopen.c b/debug/tst-sprintf-fortify-rdonly-dlopen.c > new file mode 100644 > index 0000000000..7da3f51f16 > --- /dev/null > +++ b/debug/tst-sprintf-fortify-rdonly-dlopen.c > @@ -0,0 +1 @@ > +#include "tst-sprintf-fortify-rdonly-mod.c" > diff --git a/debug/tst-sprintf-fortify-rdonly-mod.c b/debug/tst-sprintf-fortify-rdonly-mod.c > new file mode 100644 > index 0000000000..3655f27b32 > --- /dev/null > +++ b/debug/tst-sprintf-fortify-rdonly-mod.c > @@ -0,0 +1,56 @@ > +/* Testcase for BZ 30932. > + Copyright (C) 2025 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <stdio.h> > +#include <string.h> > +#include <stdlib.h> OK. Use stdlib.h instead of support.h so that we don't need libsupport. > + > +static const char *str2 = "F"; > +static char writeable_format[10] = "%s"; > +static char relro_format[10] __attribute__ ((section (".data.rel.ro"))) = > + "%s%n%s%n"; OK. Renamed variables. > + > +void > +init_writable (void) > +{ > + strcpy (writeable_format + 2, "%n%s%n"); > +} OK. Renamed function and variable use. > + > +int > +sprintf_writable (int *n1, int *n2) > +{ > + char buf[128]; > + return sprintf (buf, writeable_format, str2, n1, str2, n2); > +} OK. Same. > + > +int > +sprintf_relro (int *n1, int *n2) > +{ > + char buf[128]; > + return sprintf (buf, relro_format, str2, n1, str2, n2); > +} OK. Same. > + > +int > +sprintf_writable_malloc (int *n1, int *n2) > +{ > + char buf[128]; > + char *buf2_malloc = strdup (writeable_format); > + if (buf2_malloc == NULL) > + abort (); > + return sprintf (buf, buf2_malloc, str2, n1, str2, n2); > +} OK. same. > diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c > index ba5a791020..fafc8340ea 100644 > --- a/debug/tst-sprintf-fortify-rdonly.c > +++ b/debug/tst-sprintf-fortify-rdonly.c > @@ -27,16 +27,64 @@ > #include <support/check.h> > #include <support/support.h> > #include <support/temp_file.h> > +#include <support/xdlfcn.h> > > -jmp_buf chk_fail_buf; > -bool chk_fail_ok; > +static sigjmp_buf chk_fail_buf; OK. Changed type. > +static volatile int ret; > +static bool chk_fail_ok; > > -const char *str2 = "F"; > -char buf2[10] = "%s"; > +static void > +handler (int sig) > +{ > + if (chk_fail_ok) > + { > + chk_fail_ok = false; > + longjmp (chk_fail_buf, 1); > + } > + else > + _exit (127); > +} > + > +#define FORTIFY_FAIL \ > + do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0) > +#define CHK_FAIL_START \ > + chk_fail_ok = true; \ > + if (! sigsetjmp (chk_fail_buf, 1)) \ > + { > +#define CHK_FAIL_END \ > + chk_fail_ok = false; \ > + FORTIFY_FAIL; \ > + } > + > +static const char *str2 = "F"; > +static char writeable_format[10] = "%s"; > +static char relro_format[10] __attribute__ ((section (".data.rel.ro"))) = > + "%s%n%s%n"; > + > +extern void init_writable (void); > +extern int sprintf_writable (int *, int *); > +extern int sprintf_relro (int *, int *); > +extern int sprintf_writable_malloc (int *, int *); > + > +#define str(__x) # __x > +void (*init_writable_dlopen)(void); > +int (*sprintf_writable_dlopen)(int *, int *); > +int (*sprintf_rdonly_dlopen)(int *, int *); > +int (*sprintf_writable_malloc_dlopen)(int *, int *); > > static int > do_test (void) > { > + set_fortify_handler (handler); > + > + { > + void *h = xdlopen ("tst-sprintf-fortify-rdonly-dlopen.so", RTLD_NOW); > + init_writable_dlopen = xdlsym (h, str(init_writable)); > + sprintf_writable_dlopen = xdlsym (h, str(sprintf_writable)); > + sprintf_rdonly_dlopen = xdlsym (h, str(sprintf_relro)); > + sprintf_writable_malloc_dlopen = xdlsym (h, str(sprintf_writable_malloc)); > + } > + > struct rlimit rl; > int max_fd = 24; > > @@ -63,20 +111,94 @@ do_test (void) > } > TEST_VERIFY_EXIT (nfiles != 0); > > - /* When the format string is writable and contains %n, > - with -D_FORTIFY_SOURCE=2 it causes __chk_fail. However, if libc can not > - open procfs to check if the input format string in within a writable > - memory segment, the fortify version can not perform the check. */ > - char buf[128]; > - int n1; > - int n2; > - > - strcpy (buf2 + 2, "%n%s%n"); > - if (sprintf (buf, buf2, str2, &n1, str2, &n2) != 2 > - || n1 != 1 || n2 != 2) > - FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2); > - > - return 0; > + strcpy (writeable_format + 2, "%n%s%n"); > + init_writable (); > + init_writable_dlopen (); > + > + /* writeable_format is at a writable part of .bss segment, so libc should be > + able to check it without resorting to procfs. */ > + { > + char buf[128]; > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf (buf, writeable_format, str2, &n1, str2, &n2); > + CHK_FAIL_END > + } > + > + /* Same as before, but from an library. */ > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_writable (&n1, &n2); > + CHK_FAIL_END > + } > + > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_writable_dlopen (&n1, &n2); > + CHK_FAIL_END > + } > + > + /* relro_format is at a readonly part of .bss segment, so '%n' in format input > + should not trigger a fortify failure. */ > + { > + char buf[128]; > + int n1; > + int n2; > + if (sprintf (buf, relro_format, str2, &n1, str2, &n2) != 2 > + || n1 != 1 || n2 != 2) > + FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2); > + } > + > + /* Same as before, but from an library. */ > + { > + int n1; > + int n2; > + if (sprintf_relro (&n1, &n2) != 2 || n1 != 1 || n2 != 2) > + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); > + } > + > + { > + int n1; > + int n2; > + if (sprintf_rdonly_dlopen (&n1, &n2) != 2 || n1 != 1 || n2 != 2) > + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); > + } > + > + /* However if the format string is placed on a writable memory not covered > + by ELF segments, libc needs to resort to procfs. */ > + { > + char buf[128]; > + int n1; > + int n2; > + char *buf2_malloc = xstrdup (writeable_format); > + CHK_FAIL_START > + sprintf (buf, buf2_malloc, str2, &n1, str2, &n2); > + CHK_FAIL_END > + } > + > + /* Same as before, but from an library. */ > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_writable_malloc (&n1, &n2); > + CHK_FAIL_END > + } > + > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_writable_malloc_dlopen (&n1, &n2); > + CHK_FAIL_END > + } > + > + return ret; > } > > #include <support/test-driver.c> OK. Function calls and variables renamed to be more readable. > diff --git a/elf/Makefile b/elf/Makefile > index 2bce1ed486..8203048a88 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -72,6 +72,7 @@ dl-routines = \ > dl-open \ > dl-origin \ > dl-printf \ > + dl-readonly-area \ > dl-reloc \ > dl-runtime \ > dl-scope \ > diff --git a/elf/dl-readonly-area.c b/elf/dl-readonly-area.c > new file mode 100644 > index 0000000000..22769ec9d9 > --- /dev/null > +++ b/elf/dl-readonly-area.c > @@ -0,0 +1,86 @@ > +/* Check if range is within a read-only from a loaded ELF object. > + Copyright (C) 2025 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <ldsodefs.h> > + > +static bool > +check_relro (const struct link_map *l, uintptr_t start, uintptr_t end) > +{ > + if (l->l_relro_addr != 0) > + { > + uintptr_t relro_start = ALIGN_DOWN (l->l_addr + l->l_relro_addr, > + GLRO(dl_pagesize)); > + uintptr_t relro_end = ALIGN_DOWN (l->l_addr + l->l_relro_addr > + + l->l_relro_size, > + GLRO(dl_pagesize)); > + /* RELRO is caved out from a RW segment, so the next range is either > + RW or nonexistent. */ > + return relro_start <= start && end <= relro_end > + ? dl_readonly_area_rdonly : dl_readonly_area_writable; > + > + } > + return dl_readonly_area_writable; > +} OK. Use l_relro_addr and l_relro_size instead of looping. The comparisons look right. > + > +enum dl_readonly_area_error_type > +_dl_readonly_area (const void *ptr, size_t size) OK. New enum used instead of bool. > +{ > + struct dl_find_object dlfo; > + if (_dl_find_object ((void *)ptr, &dlfo) != 0) > + return dl_readonly_area_not_found; OK. Use enum. > + > + const struct link_map *l = dlfo.dlfo_link_map; > + uintptr_t ptr_start = (uintptr_t) ptr; > + uintptr_t ptr_end = ptr_start + size; > + > + for (const ElfW(Phdr) *ph = l->l_phdr; ph < &l->l_phdr[l->l_phnum]; ++ph) > + if (ph->p_type == PT_LOAD) > + { > + /* For segments with alignment larger than the page size, > + _dl_map_segment allocates additional space that is mark as > + PROT_NONE (so we can ignore). */ > + uintptr_t from = l->l_addr > + + ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize)); > + uintptr_t to = l->l_addr > + + ALIGN_UP (ph->p_vaddr + ph->p_filesz, GLRO(dl_pagesize)); > + > + /* Found an entry that at least partially covers the area. */ > + if (from < ptr_end && to > ptr_start) > + { > + if (ph->p_flags & PF_W) > + return check_relro (l, ptr_start, ptr_end); > + > + if ((ph->p_flags & PF_R) == 0) > + return dl_readonly_area_writable; OK. Use new enum. > + > + if (from <= ptr_start && to >= ptr_end) OK. Correct comparisons. > + return dl_readonly_area_rdonly; OK. New enum. > + else if (from <= ptr_start) OK. Correct comparison. > + size -= to - ptr_start; > + else if (to >= ptr_end) OK. Correct comparison. > + size -= ptr_end - from; > + else > + size -= to - from; > + > + if (size == 0) > + break; > + } > + } > + > + return size == 0 ? dl_readonly_area_rdonly : dl_readonly_area_not_found; OK. New enum. > +} > diff --git a/elf/rtld.c b/elf/rtld.c > index 00b25c1a73..099c447e80 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -371,6 +371,7 @@ struct rtld_global_ro _rtld_global_ro attribute_relro = > ._dl_error_free = _dl_error_free, > ._dl_tls_get_addr_soft = _dl_tls_get_addr_soft, > ._dl_libc_freeres = __rtld_libc_freeres, > + ._dl_readonly_area = _dl_readonly_area, > }; > /* If we would use strong_alias here the compiler would see a > non-hidden definition. This would undo the effect of the previous > diff --git a/include/stdlib.h b/include/stdlib.h > index 57f4ab8545..b7147ba590 100644 > --- a/include/stdlib.h > +++ b/include/stdlib.h > @@ -368,6 +368,21 @@ struct abort_msg_s > extern struct abort_msg_s *__abort_msg; > libc_hidden_proto (__abort_msg) > > +enum readonly_error_type > +{ > + readonly_noerror, > + readonly_area_writable, > + readonly_procfs_inaccessible, > + readonly_procfs_open_fail, > +}; > + > +extern enum readonly_error_type __readonly_area (const void *ptr, > + size_t size) > + attribute_hidden; > +extern enum readonly_error_type __readonly_area_fallback (const void *ptr, > + size_t size) > + attribute_hidden; > + > # if IS_IN (rtld) > extern __typeof (unsetenv) unsetenv attribute_hidden; > extern __typeof (__strtoul_internal) __strtoul_internal attribute_hidden; > diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c > index aa9708bff5..fa41e1b242 100644 > --- a/stdio-common/vfprintf-internal.c > +++ b/stdio-common/vfprintf-internal.c > @@ -576,7 +576,8 @@ static const uint8_t jump_table[] = > > /* Handle positional format specifiers. */ > static void printf_positional (struct Xprintf_buffer *buf, > - const CHAR_T *format, int readonly_format, > + const CHAR_T *format, > + enum readonly_error_type readonly_format, > va_list ap, va_list *ap_savep, > int nspecs_done, const UCHAR_T *lead_str_end, > CHAR_T *work_buffer, int save_errno, > @@ -626,9 +627,7 @@ Xprintf_buffer (struct Xprintf_buffer *buf, const CHAR_T *format, > /* For the %m format we may need the current `errno' value. */ > int save_errno = errno; > > - /* 1 if format is in read-only memory, -1 if it is in writable memory, > - 0 if unknown. */ > - int readonly_format = 0; > + enum readonly_error_type readonly_format = readonly_noerror; > > /* Initialize local variables. */ > grouping = (const char *) -1; > @@ -1045,7 +1044,7 @@ do_positional: > > static void > printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format, > - int readonly_format, > + enum readonly_error_type readonly_format, > va_list ap, va_list *ap_savep, int nspecs_done, > const UCHAR_T *lead_str_end, > CHAR_T *work_buffer, int save_errno, > diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c > index 8d20493766..ad1e0ea3ff 100644 > --- a/stdio-common/vfprintf-process-arg.c > +++ b/stdio-common/vfprintf-process-arg.c > @@ -324,16 +324,24 @@ LABEL (form_pointer): > LABEL (form_number): > if ((mode_flags & PRINTF_FORTIFY) != 0) > { > - if (! readonly_format) > - { > - extern int __readonly_area (const void *, size_t) > - attribute_hidden; > - readonly_format > - = __readonly_area (format, ((STR_LEN (format) + 1) > - * sizeof (CHAR_T))); > - } > - if (readonly_format < 0) > - __libc_fatal ("*** %n in writable segment detected ***\n"); > + if (readonly_format == readonly_noerror) > + readonly_format = __readonly_area (format, ((STR_LEN (format) + 1) > + * sizeof (CHAR_T))); > + switch (readonly_format) > + { > + case readonly_area_writable: > + __libc_fatal ("*** %n in writable segments detected ***\n"); > + /* The format is not within ELF segmetns and opening /proc/self/maps Sorry I wasn't clear about the typo here earlier. It was in the comment: "segmetns". The "segment" in the libc_fatal message was actually okay. > + failed because there are too many files. */ > + case readonly_procfs_open_fail: > + __libc_fatal ("*** procfs could not open ***\n"); > + /* The /proc/self/maps can not be opened either because it is not > + available or the process does not have the right permission. Since > + it should not be attacker-controlled we can avoid failure. */ > + case readonly_procfs_inaccessible: > + case readonly_noerror: > + break; > + } > } > /* Answer the count of characters written. */ > void *ptrptr = process_arg_pointer (); > diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h > index 19494b82ee..5b12a41a18 100644 > --- a/sysdeps/generic/ldsodefs.h > +++ b/sysdeps/generic/ldsodefs.h > @@ -276,6 +276,12 @@ struct audit_ifaces > struct audit_ifaces *next; > }; > > +enum dl_readonly_area_error_type > +{ > + dl_readonly_area_rdonly, > + dl_readonly_area_writable, > + dl_readonly_area_not_found, > +}; OK. New enum declared here. > > /* Test whether given NAME matches any of the names of the given object. */ > extern int _dl_name_match_p (const char *__name, const struct link_map *__map) > @@ -676,6 +682,10 @@ struct rtld_global_ro > dlopen. */ > int (*_dl_find_object) (void *, struct dl_find_object *); > > + /* Implementation of _dl_readonly_area, used in fortify routines to check > + if memory area is within a read-only ELF segment. */ > + enum dl_readonly_area_error_type (*_dl_readonly_area) (const void *, size_t); OK. Change declaration to use enum. > + > /* Dynamic linker operations used after static dlopen. */ > const struct dlfcn_hook *_dl_dlfcn_hook; > > @@ -1284,6 +1294,10 @@ extern void _dl_show_scope (struct link_map *new, int from) > extern struct link_map *_dl_find_dso_for_object (const ElfW(Addr) addr); > rtld_hidden_proto (_dl_find_dso_for_object) > > +extern enum dl_readonly_area_error_type _dl_readonly_area (const void *ptr, > + size_t size) > + attribute_hidden; OK. Change declaration to use enum. > + > /* Initialization which is normally done by the dynamic linker. */ > extern void _dl_non_dynamic_init (void) > attribute_hidden; > diff --git a/sysdeps/mach/readonly-area.c b/sysdeps/mach/readonly-area-fallback.c > similarity index 90% > rename from sysdeps/mach/readonly-area.c > rename to sysdeps/mach/readonly-area-fallback.c OK. File renamed. > index fb89413fe6..b1bb1cba39 100644 > --- a/sysdeps/mach/readonly-area.c > +++ b/sysdeps/mach/readonly-area-fallback.c > @@ -20,11 +20,8 @@ > #include <stdint.h> > #include <mach.h> > > -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable. > - Return -1 if it is writable. */ > - > -int > -__readonly_area (const char *ptr, size_t size) > +enum readonly_error_type > +__readonly_area_arch (const void *ptr, size_t size) The function needs to be renamed as well. > { > vm_address_t region_address = (uintptr_t) ptr; > vm_size_t region_length = size; > @@ -46,11 +43,11 @@ __readonly_area (const char *ptr, size_t size) > continue; > > if (protection & VM_PROT_WRITE) > - return -1; > + return readonly_area_writable; > > if (region_address - (uintptr_t) ptr >= size) > break; > } > > - return 1; > + return readonly_noerror; > } > diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area-fallback.c > similarity index 84% > rename from sysdeps/unix/sysv/linux/readonly-area.c > rename to sysdeps/unix/sysv/linux/readonly-area-fallback.c OK. File renamed. > index 62d2070c72..c93ad2a598 100644 > --- a/sysdeps/unix/sysv/linux/readonly-area.c > +++ b/sysdeps/unix/sysv/linux/readonly-area-fallback.c > @@ -23,11 +23,8 @@ > #include <string.h> > #include "libio/libioP.h" > > -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable. > - Return -1 if it is writable. */ > - > -int > -__readonly_area (const char *ptr, size_t size) > +enum readonly_error_type > +__readonly_area_fallback (const void *ptr, size_t size) OK. Function renamed. > { > const void *ptr_end = ptr + size; > > @@ -42,11 +39,11 @@ __readonly_area (const char *ptr, size_t size) > to the /proc filesystem if it is set[ug]id. There has > been no willingness to change this in the kernel so > far. */ > - || errno == EACCES > - /* Process has reached the maximum number of open files. */ > - || errno == EMFILE) > - return 1; > - return -1; > + || errno == EACCES) > + return readonly_procfs_inaccessible; > + /* Process has reached the maximum number of open files or another > + unusual error. */ > + return readonly_procfs_open_fail; > } > > /* We need no locking. */ > @@ -98,7 +95,5 @@ __readonly_area (const char *ptr, size_t size) > fclose (fp); > free (line); > > - /* If the whole area between ptr and ptr_end is covered by read-only > - VMAs, return 1. Otherwise return -1. */ > - return size == 0 ? 1 : -1; > + return size == 0 ? readonly_noerror : readonly_area_writable; > } > -- > 2.43.0 > -- Arjun Shankar he/him/his
* Adhemerval Zanella: > The 7bb8045ec0 path made the '%n' fortify check ignore EMFILE errors > while trying to open /proc/self/maps, and this added a security > issue where EMFILE can be attacker-controlled thus making it > ineffective for some cases. > > The EMFILE failure is reinstated but with a different error > message. Also, to improve the false positive of the hardening for > the cases where no new files can be opened, the > _dl_readonly_area now uses _dl_find_object to check if the > memory area is within a writable ELF segment. The procfs method is > still used as fallback. > > Checked on x86_64-linux-gnu and i686-linux-gnu. > -- > Changes from v1: > * Rename readonly-area-arch to readonly-area-fallback. > * Handle found RW areas on _dl_readonly_area. > * Use l_relro_addr/l_relro_size in check_relro. > * Change variable names in tst-sprintf-fortify-rdonly-mod.c. Adhemerval, are you looking for further comments on this patch? Thanks, Florian
On 21/03/25 12:41, Florian Weimer wrote: > * Adhemerval Zanella: > >> The 7bb8045ec0 path made the '%n' fortify check ignore EMFILE errors >> while trying to open /proc/self/maps, and this added a security >> issue where EMFILE can be attacker-controlled thus making it >> ineffective for some cases. >> >> The EMFILE failure is reinstated but with a different error >> message. Also, to improve the false positive of the hardening for >> the cases where no new files can be opened, the >> _dl_readonly_area now uses _dl_find_object to check if the >> memory area is within a writable ELF segment. The procfs method is >> still used as fallback. >> >> Checked on x86_64-linux-gnu and i686-linux-gnu. >> -- >> Changes from v1: >> * Rename readonly-area-arch to readonly-area-fallback. >> * Handle found RW areas on _dl_readonly_area. >> * Use l_relro_addr/l_relro_size in check_relro. >> * Change variable names in tst-sprintf-fortify-rdonly-mod.c. > > Adhemerval, are you looking for further comments on this patch? No, I was focusing on the tst-origin breakage before install this.
diff --git a/Makeconfig b/Makeconfig index aa547a443f..bf50406656 100644 --- a/Makeconfig +++ b/Makeconfig @@ -633,7 +633,7 @@ link-libc-printers-tests = $(link-libc-rpath) \ $(link-libc-tests-after-rpath-link) # This is how to find at build-time things that will be installed there. -rpath-dirs = math elf dlfcn nss nis rt resolv mathvec support misc +rpath-dirs = math elf dlfcn nss nis rt resolv mathvec support misc debug rpath-link = \ $(common-objdir):$(subst $(empty) ,:,$(patsubst ../$(subdir),.,$(rpath-dirs:%=$(common-objpfx)%))) else # build-static diff --git a/debug/Makefile b/debug/Makefile index 6a05205ce6..97d36438e8 100644 --- a/debug/Makefile +++ b/debug/Makefile @@ -74,6 +74,7 @@ routines = \ readlink_chk \ readlinkat_chk \ readonly-area \ + readonly-area-fallback \ realpath_chk \ recv_chk \ recvfrom_chk \ @@ -179,9 +180,17 @@ CPPFLAGS-tst-longjmp_chk3.c += $(no-fortify-source) -D_FORTIFY_SOURCE=1 CPPFLAGS-tst-realpath-chk.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 CPPFLAGS-tst-chk-cancel.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 CFLAGS-tst-sprintf-fortify-rdonly.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 +CFLAGS-tst-sprintf-fortify-rdonly-mod.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 +CFLAGS-tst-sprintf-fortify-rdonly-dlopen.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 CFLAGS-tst-fortify-syslog.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 CFLAGS-tst-fortify-wide.c += $(no-fortify-source) -D_FORTIFY_SOURCE=2 +$(objpfx)tst-sprintf-fortify-rdonly: \ + $(objpfx)tst-sprintf-fortify-rdonly-mod.so \ + $(objpfx)tst-sprintf-fortify-rdonly-dlopen.so +LDLIBS-tst-sprintf-fortify-rdonly-mod.so = $(libsupport) +LDLIBS-tst-sprintf-fortify-rdonly-dlopen.so = $(libsupport) + # _FORTIFY_SOURCE tests. # Auto-generate tests for _FORTIFY_SOURCE for different levels, compilers and # preprocessor conditions based on tst-fortify.c. @@ -302,6 +311,11 @@ tests-container += \ tst-fortify-syslog \ # tests-container +modules-names += \ + tst-sprintf-fortify-rdonly-dlopen \ + tst-sprintf-fortify-rdonly-mod \ + # modules-names + ifeq ($(have-ssp),yes) tests += tst-ssp-1 endif diff --git a/debug/readonly-area.c b/debug/readonly-area.c index 04b437ed8d..a35ef9934f 100644 --- a/debug/readonly-area.c +++ b/debug/readonly-area.c @@ -16,18 +16,19 @@ <https://www.gnu.org/licenses/>. */ #include <stdlib.h> +#include <ldsodefs.h> -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable. - Return -1 if it is writable. */ - -int +enum readonly_error_type __readonly_area (const void *ptr, size_t size) { - /* We cannot determine in general whether memory is writable or not. - This must be handled in a system-dependent manner. to not - unconditionally break code we need to return here a positive - answer. This disables this security measure but that is the - price people have to pay for using systems without a real - implementation of this interface. */ - return 1; + switch (GLRO(dl_readonly_area (ptr, size))) + { + case dl_readonly_area_rdonly: + return readonly_noerror; + case dl_readonly_area_writable: + return readonly_area_writable; + default: + /* fallthrough */ + } + return __readonly_area_fallback (ptr, size); } diff --git a/debug/tst-sprintf-fortify-rdonly-dlopen.c b/debug/tst-sprintf-fortify-rdonly-dlopen.c new file mode 100644 index 0000000000..7da3f51f16 --- /dev/null +++ b/debug/tst-sprintf-fortify-rdonly-dlopen.c @@ -0,0 +1 @@ +#include "tst-sprintf-fortify-rdonly-mod.c" diff --git a/debug/tst-sprintf-fortify-rdonly-mod.c b/debug/tst-sprintf-fortify-rdonly-mod.c new file mode 100644 index 0000000000..3655f27b32 --- /dev/null +++ b/debug/tst-sprintf-fortify-rdonly-mod.c @@ -0,0 +1,56 @@ +/* Testcase for BZ 30932. + Copyright (C) 2025 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <stdio.h> +#include <string.h> +#include <stdlib.h> + +static const char *str2 = "F"; +static char writeable_format[10] = "%s"; +static char relro_format[10] __attribute__ ((section (".data.rel.ro"))) = + "%s%n%s%n"; + +void +init_writable (void) +{ + strcpy (writeable_format + 2, "%n%s%n"); +} + +int +sprintf_writable (int *n1, int *n2) +{ + char buf[128]; + return sprintf (buf, writeable_format, str2, n1, str2, n2); +} + +int +sprintf_relro (int *n1, int *n2) +{ + char buf[128]; + return sprintf (buf, relro_format, str2, n1, str2, n2); +} + +int +sprintf_writable_malloc (int *n1, int *n2) +{ + char buf[128]; + char *buf2_malloc = strdup (writeable_format); + if (buf2_malloc == NULL) + abort (); + return sprintf (buf, buf2_malloc, str2, n1, str2, n2); +} diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c index ba5a791020..fafc8340ea 100644 --- a/debug/tst-sprintf-fortify-rdonly.c +++ b/debug/tst-sprintf-fortify-rdonly.c @@ -27,16 +27,64 @@ #include <support/check.h> #include <support/support.h> #include <support/temp_file.h> +#include <support/xdlfcn.h> -jmp_buf chk_fail_buf; -bool chk_fail_ok; +static sigjmp_buf chk_fail_buf; +static volatile int ret; +static bool chk_fail_ok; -const char *str2 = "F"; -char buf2[10] = "%s"; +static void +handler (int sig) +{ + if (chk_fail_ok) + { + chk_fail_ok = false; + longjmp (chk_fail_buf, 1); + } + else + _exit (127); +} + +#define FORTIFY_FAIL \ + do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0) +#define CHK_FAIL_START \ + chk_fail_ok = true; \ + if (! sigsetjmp (chk_fail_buf, 1)) \ + { +#define CHK_FAIL_END \ + chk_fail_ok = false; \ + FORTIFY_FAIL; \ + } + +static const char *str2 = "F"; +static char writeable_format[10] = "%s"; +static char relro_format[10] __attribute__ ((section (".data.rel.ro"))) = + "%s%n%s%n"; + +extern void init_writable (void); +extern int sprintf_writable (int *, int *); +extern int sprintf_relro (int *, int *); +extern int sprintf_writable_malloc (int *, int *); + +#define str(__x) # __x +void (*init_writable_dlopen)(void); +int (*sprintf_writable_dlopen)(int *, int *); +int (*sprintf_rdonly_dlopen)(int *, int *); +int (*sprintf_writable_malloc_dlopen)(int *, int *); static int do_test (void) { + set_fortify_handler (handler); + + { + void *h = xdlopen ("tst-sprintf-fortify-rdonly-dlopen.so", RTLD_NOW); + init_writable_dlopen = xdlsym (h, str(init_writable)); + sprintf_writable_dlopen = xdlsym (h, str(sprintf_writable)); + sprintf_rdonly_dlopen = xdlsym (h, str(sprintf_relro)); + sprintf_writable_malloc_dlopen = xdlsym (h, str(sprintf_writable_malloc)); + } + struct rlimit rl; int max_fd = 24; @@ -63,20 +111,94 @@ do_test (void) } TEST_VERIFY_EXIT (nfiles != 0); - /* When the format string is writable and contains %n, - with -D_FORTIFY_SOURCE=2 it causes __chk_fail. However, if libc can not - open procfs to check if the input format string in within a writable - memory segment, the fortify version can not perform the check. */ - char buf[128]; - int n1; - int n2; - - strcpy (buf2 + 2, "%n%s%n"); - if (sprintf (buf, buf2, str2, &n1, str2, &n2) != 2 - || n1 != 1 || n2 != 2) - FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2); - - return 0; + strcpy (writeable_format + 2, "%n%s%n"); + init_writable (); + init_writable_dlopen (); + + /* writeable_format is at a writable part of .bss segment, so libc should be + able to check it without resorting to procfs. */ + { + char buf[128]; + int n1; + int n2; + CHK_FAIL_START + sprintf (buf, writeable_format, str2, &n1, str2, &n2); + CHK_FAIL_END + } + + /* Same as before, but from an library. */ + { + int n1; + int n2; + CHK_FAIL_START + sprintf_writable (&n1, &n2); + CHK_FAIL_END + } + + { + int n1; + int n2; + CHK_FAIL_START + sprintf_writable_dlopen (&n1, &n2); + CHK_FAIL_END + } + + /* relro_format is at a readonly part of .bss segment, so '%n' in format input + should not trigger a fortify failure. */ + { + char buf[128]; + int n1; + int n2; + if (sprintf (buf, relro_format, str2, &n1, str2, &n2) != 2 + || n1 != 1 || n2 != 2) + FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2); + } + + /* Same as before, but from an library. */ + { + int n1; + int n2; + if (sprintf_relro (&n1, &n2) != 2 || n1 != 1 || n2 != 2) + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); + } + + { + int n1; + int n2; + if (sprintf_rdonly_dlopen (&n1, &n2) != 2 || n1 != 1 || n2 != 2) + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); + } + + /* However if the format string is placed on a writable memory not covered + by ELF segments, libc needs to resort to procfs. */ + { + char buf[128]; + int n1; + int n2; + char *buf2_malloc = xstrdup (writeable_format); + CHK_FAIL_START + sprintf (buf, buf2_malloc, str2, &n1, str2, &n2); + CHK_FAIL_END + } + + /* Same as before, but from an library. */ + { + int n1; + int n2; + CHK_FAIL_START + sprintf_writable_malloc (&n1, &n2); + CHK_FAIL_END + } + + { + int n1; + int n2; + CHK_FAIL_START + sprintf_writable_malloc_dlopen (&n1, &n2); + CHK_FAIL_END + } + + return ret; } #include <support/test-driver.c> diff --git a/elf/Makefile b/elf/Makefile index 2bce1ed486..8203048a88 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -72,6 +72,7 @@ dl-routines = \ dl-open \ dl-origin \ dl-printf \ + dl-readonly-area \ dl-reloc \ dl-runtime \ dl-scope \ diff --git a/elf/dl-readonly-area.c b/elf/dl-readonly-area.c new file mode 100644 index 0000000000..22769ec9d9 --- /dev/null +++ b/elf/dl-readonly-area.c @@ -0,0 +1,86 @@ +/* Check if range is within a read-only from a loaded ELF object. + Copyright (C) 2025 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <ldsodefs.h> + +static bool +check_relro (const struct link_map *l, uintptr_t start, uintptr_t end) +{ + if (l->l_relro_addr != 0) + { + uintptr_t relro_start = ALIGN_DOWN (l->l_addr + l->l_relro_addr, + GLRO(dl_pagesize)); + uintptr_t relro_end = ALIGN_DOWN (l->l_addr + l->l_relro_addr + + l->l_relro_size, + GLRO(dl_pagesize)); + /* RELRO is caved out from a RW segment, so the next range is either + RW or nonexistent. */ + return relro_start <= start && end <= relro_end + ? dl_readonly_area_rdonly : dl_readonly_area_writable; + + } + return dl_readonly_area_writable; +} + +enum dl_readonly_area_error_type +_dl_readonly_area (const void *ptr, size_t size) +{ + struct dl_find_object dlfo; + if (_dl_find_object ((void *)ptr, &dlfo) != 0) + return dl_readonly_area_not_found; + + const struct link_map *l = dlfo.dlfo_link_map; + uintptr_t ptr_start = (uintptr_t) ptr; + uintptr_t ptr_end = ptr_start + size; + + for (const ElfW(Phdr) *ph = l->l_phdr; ph < &l->l_phdr[l->l_phnum]; ++ph) + if (ph->p_type == PT_LOAD) + { + /* For segments with alignment larger than the page size, + _dl_map_segment allocates additional space that is mark as + PROT_NONE (so we can ignore). */ + uintptr_t from = l->l_addr + + ALIGN_DOWN (ph->p_vaddr, GLRO(dl_pagesize)); + uintptr_t to = l->l_addr + + ALIGN_UP (ph->p_vaddr + ph->p_filesz, GLRO(dl_pagesize)); + + /* Found an entry that at least partially covers the area. */ + if (from < ptr_end && to > ptr_start) + { + if (ph->p_flags & PF_W) + return check_relro (l, ptr_start, ptr_end); + + if ((ph->p_flags & PF_R) == 0) + return dl_readonly_area_writable; + + if (from <= ptr_start && to >= ptr_end) + return dl_readonly_area_rdonly; + else if (from <= ptr_start) + size -= to - ptr_start; + else if (to >= ptr_end) + size -= ptr_end - from; + else + size -= to - from; + + if (size == 0) + break; + } + } + + return size == 0 ? dl_readonly_area_rdonly : dl_readonly_area_not_found; +} diff --git a/elf/rtld.c b/elf/rtld.c index 00b25c1a73..099c447e80 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -371,6 +371,7 @@ struct rtld_global_ro _rtld_global_ro attribute_relro = ._dl_error_free = _dl_error_free, ._dl_tls_get_addr_soft = _dl_tls_get_addr_soft, ._dl_libc_freeres = __rtld_libc_freeres, + ._dl_readonly_area = _dl_readonly_area, }; /* If we would use strong_alias here the compiler would see a non-hidden definition. This would undo the effect of the previous diff --git a/include/stdlib.h b/include/stdlib.h index 57f4ab8545..b7147ba590 100644 --- a/include/stdlib.h +++ b/include/stdlib.h @@ -368,6 +368,21 @@ struct abort_msg_s extern struct abort_msg_s *__abort_msg; libc_hidden_proto (__abort_msg) +enum readonly_error_type +{ + readonly_noerror, + readonly_area_writable, + readonly_procfs_inaccessible, + readonly_procfs_open_fail, +}; + +extern enum readonly_error_type __readonly_area (const void *ptr, + size_t size) + attribute_hidden; +extern enum readonly_error_type __readonly_area_fallback (const void *ptr, + size_t size) + attribute_hidden; + # if IS_IN (rtld) extern __typeof (unsetenv) unsetenv attribute_hidden; extern __typeof (__strtoul_internal) __strtoul_internal attribute_hidden; diff --git a/stdio-common/vfprintf-internal.c b/stdio-common/vfprintf-internal.c index aa9708bff5..fa41e1b242 100644 --- a/stdio-common/vfprintf-internal.c +++ b/stdio-common/vfprintf-internal.c @@ -576,7 +576,8 @@ static const uint8_t jump_table[] = /* Handle positional format specifiers. */ static void printf_positional (struct Xprintf_buffer *buf, - const CHAR_T *format, int readonly_format, + const CHAR_T *format, + enum readonly_error_type readonly_format, va_list ap, va_list *ap_savep, int nspecs_done, const UCHAR_T *lead_str_end, CHAR_T *work_buffer, int save_errno, @@ -626,9 +627,7 @@ Xprintf_buffer (struct Xprintf_buffer *buf, const CHAR_T *format, /* For the %m format we may need the current `errno' value. */ int save_errno = errno; - /* 1 if format is in read-only memory, -1 if it is in writable memory, - 0 if unknown. */ - int readonly_format = 0; + enum readonly_error_type readonly_format = readonly_noerror; /* Initialize local variables. */ grouping = (const char *) -1; @@ -1045,7 +1044,7 @@ do_positional: static void printf_positional (struct Xprintf_buffer * buf, const CHAR_T *format, - int readonly_format, + enum readonly_error_type readonly_format, va_list ap, va_list *ap_savep, int nspecs_done, const UCHAR_T *lead_str_end, CHAR_T *work_buffer, int save_errno, diff --git a/stdio-common/vfprintf-process-arg.c b/stdio-common/vfprintf-process-arg.c index 8d20493766..ad1e0ea3ff 100644 --- a/stdio-common/vfprintf-process-arg.c +++ b/stdio-common/vfprintf-process-arg.c @@ -324,16 +324,24 @@ LABEL (form_pointer): LABEL (form_number): if ((mode_flags & PRINTF_FORTIFY) != 0) { - if (! readonly_format) - { - extern int __readonly_area (const void *, size_t) - attribute_hidden; - readonly_format - = __readonly_area (format, ((STR_LEN (format) + 1) - * sizeof (CHAR_T))); - } - if (readonly_format < 0) - __libc_fatal ("*** %n in writable segment detected ***\n"); + if (readonly_format == readonly_noerror) + readonly_format = __readonly_area (format, ((STR_LEN (format) + 1) + * sizeof (CHAR_T))); + switch (readonly_format) + { + case readonly_area_writable: + __libc_fatal ("*** %n in writable segments detected ***\n"); + /* The format is not within ELF segmetns and opening /proc/self/maps + failed because there are too many files. */ + case readonly_procfs_open_fail: + __libc_fatal ("*** procfs could not open ***\n"); + /* The /proc/self/maps can not be opened either because it is not + available or the process does not have the right permission. Since + it should not be attacker-controlled we can avoid failure. */ + case readonly_procfs_inaccessible: + case readonly_noerror: + break; + } } /* Answer the count of characters written. */ void *ptrptr = process_arg_pointer (); diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index 19494b82ee..5b12a41a18 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -276,6 +276,12 @@ struct audit_ifaces struct audit_ifaces *next; }; +enum dl_readonly_area_error_type +{ + dl_readonly_area_rdonly, + dl_readonly_area_writable, + dl_readonly_area_not_found, +}; /* Test whether given NAME matches any of the names of the given object. */ extern int _dl_name_match_p (const char *__name, const struct link_map *__map) @@ -676,6 +682,10 @@ struct rtld_global_ro dlopen. */ int (*_dl_find_object) (void *, struct dl_find_object *); + /* Implementation of _dl_readonly_area, used in fortify routines to check + if memory area is within a read-only ELF segment. */ + enum dl_readonly_area_error_type (*_dl_readonly_area) (const void *, size_t); + /* Dynamic linker operations used after static dlopen. */ const struct dlfcn_hook *_dl_dlfcn_hook; @@ -1284,6 +1294,10 @@ extern void _dl_show_scope (struct link_map *new, int from) extern struct link_map *_dl_find_dso_for_object (const ElfW(Addr) addr); rtld_hidden_proto (_dl_find_dso_for_object) +extern enum dl_readonly_area_error_type _dl_readonly_area (const void *ptr, + size_t size) + attribute_hidden; + /* Initialization which is normally done by the dynamic linker. */ extern void _dl_non_dynamic_init (void) attribute_hidden; diff --git a/sysdeps/mach/readonly-area.c b/sysdeps/mach/readonly-area-fallback.c similarity index 90% rename from sysdeps/mach/readonly-area.c rename to sysdeps/mach/readonly-area-fallback.c index fb89413fe6..b1bb1cba39 100644 --- a/sysdeps/mach/readonly-area.c +++ b/sysdeps/mach/readonly-area-fallback.c @@ -20,11 +20,8 @@ #include <stdint.h> #include <mach.h> -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable. - Return -1 if it is writable. */ - -int -__readonly_area (const char *ptr, size_t size) +enum readonly_error_type +__readonly_area_arch (const void *ptr, size_t size) { vm_address_t region_address = (uintptr_t) ptr; vm_size_t region_length = size; @@ -46,11 +43,11 @@ __readonly_area (const char *ptr, size_t size) continue; if (protection & VM_PROT_WRITE) - return -1; + return readonly_area_writable; if (region_address - (uintptr_t) ptr >= size) break; } - return 1; + return readonly_noerror; } diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area-fallback.c similarity index 84% rename from sysdeps/unix/sysv/linux/readonly-area.c rename to sysdeps/unix/sysv/linux/readonly-area-fallback.c index 62d2070c72..c93ad2a598 100644 --- a/sysdeps/unix/sysv/linux/readonly-area.c +++ b/sysdeps/unix/sysv/linux/readonly-area-fallback.c @@ -23,11 +23,8 @@ #include <string.h> #include "libio/libioP.h" -/* Return 1 if the whole area PTR .. PTR+SIZE is not writable. - Return -1 if it is writable. */ - -int -__readonly_area (const char *ptr, size_t size) +enum readonly_error_type +__readonly_area_fallback (const void *ptr, size_t size) { const void *ptr_end = ptr + size; @@ -42,11 +39,11 @@ __readonly_area (const char *ptr, size_t size) to the /proc filesystem if it is set[ug]id. There has been no willingness to change this in the kernel so far. */ - || errno == EACCES - /* Process has reached the maximum number of open files. */ - || errno == EMFILE) - return 1; - return -1; + || errno == EACCES) + return readonly_procfs_inaccessible; + /* Process has reached the maximum number of open files or another + unusual error. */ + return readonly_procfs_open_fail; } /* We need no locking. */ @@ -98,7 +95,5 @@ __readonly_area (const char *ptr, size_t size) fclose (fp); free (line); - /* If the whole area between ptr and ptr_end is covered by read-only - VMAs, return 1. Otherwise return -1. */ - return size == 0 ? 1 : -1; + return size == 0 ? readonly_noerror : readonly_area_writable; }
The 7bb8045ec0 path made the '%n' fortify check ignore EMFILE errors while trying to open /proc/self/maps, and this added a security issue where EMFILE can be attacker-controlled thus making it ineffective for some cases. The EMFILE failure is reinstated but with a different error message. Also, to improve the false positive of the hardening for the cases where no new files can be opened, the _dl_readonly_area now uses _dl_find_object to check if the memory area is within a writable ELF segment. The procfs method is still used as fallback. Checked on x86_64-linux-gnu and i686-linux-gnu. -- Changes from v1: * Rename readonly-area-arch to readonly-area-fallback. * Handle found RW areas on _dl_readonly_area. * Use l_relro_addr/l_relro_size in check_relro. * Change variable names in tst-sprintf-fortify-rdonly-mod.c. --- Makeconfig | 2 +- debug/Makefile | 14 ++ debug/readonly-area.c | 23 +-- debug/tst-sprintf-fortify-rdonly-dlopen.c | 1 + debug/tst-sprintf-fortify-rdonly-mod.c | 56 +++++++ debug/tst-sprintf-fortify-rdonly.c | 158 ++++++++++++++++-- elf/Makefile | 1 + elf/dl-readonly-area.c | 86 ++++++++++ elf/rtld.c | 1 + include/stdlib.h | 15 ++ stdio-common/vfprintf-internal.c | 9 +- stdio-common/vfprintf-process-arg.c | 28 ++-- sysdeps/generic/ldsodefs.h | 14 ++ ...adonly-area.c => readonly-area-fallback.c} | 11 +- ...adonly-area.c => readonly-area-fallback.c} | 21 +-- 15 files changed, 375 insertions(+), 65 deletions(-) create mode 100644 debug/tst-sprintf-fortify-rdonly-dlopen.c create mode 100644 debug/tst-sprintf-fortify-rdonly-mod.c create mode 100644 elf/dl-readonly-area.c rename sysdeps/mach/{readonly-area.c => readonly-area-fallback.c} (90%) rename sysdeps/unix/sysv/linux/{readonly-area.c => readonly-area-fallback.c} (84%)