| Message ID | 20250224133906.1723623-1-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | debug: Improve '%n' fortify detection (BZ 30932) | expand |
Hi Adhemerval, Took me a while to go through it since I had to learn about some of the context before I could meaningfully comment on the patch. I learned something while reviewing this. Thanks! The patch seems mostly okay to me, but I have some comments below. Cheers! Arjun > 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 afallback. > > Checked on x86_64-linux-gnu and i686-linux-gnu. Small typo: "afallback". > --- > Makeconfig | 2 +- > debug/Makefile | 14 ++ > debug/readonly-area.c | 17 +- > debug/tst-sprintf-fortify-rdonly-dlopen.c | 1 + > debug/tst-sprintf-fortify-rdonly-mod.c | 53 ++++++ > debug/tst-sprintf-fortify-rdonly.c | 155 ++++++++++++++++-- > elf/Makefile | 1 + > elf/dl-readonly-area.c | 84 ++++++++++ > 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 | 7 + > .../{readonly-area.c => readonly-area-arch.c} | 11 +- > .../{readonly-area.c => readonly-area-arch.c} | 21 +-- > 15 files changed, 355 insertions(+), 64 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-arch.c} (90%) > rename sysdeps/unix/sysv/linux/{readonly-area.c => readonly-area-arch.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 OK. Looks like this is needed for the test to be able to dlopen the test module. > 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..be432f7740 100644 > --- a/debug/Makefile > +++ b/debug/Makefile > @@ -74,6 +74,7 @@ routines = \ > readlink_chk \ > readlinkat_chk \ > readonly-area \ > + readonly-area-arch \ This is the fallback implementation for checking read-only status of an address range. The -arch suffix seems a bit confusing to me as a name. How about readonly-area-fallback.c? > 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 OK. Build test modules with fortification on so that the checks we test for are performed. > 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) > + Florian was mentioning that linking test modules against libsupport might not work properly. I don't remember the details though. > # _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 > + OK. New test modules. > ifeq ($(have-ssp),yes) > tests += tst-ssp-1 > endif > diff --git a/debug/readonly-area.c b/debug/readonly-area.c > index 04b437ed8d..e53bc00f9f 100644 > --- a/debug/readonly-area.c > +++ b/debug/readonly-area.c > @@ -16,18 +16,13 @@ > <https://www.gnu.org/licenses/>. */ > > #include <stdlib.h> > +#include <ldsodefs.h> OK. > > -/* 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; OK. This used to be a stub implementation but now it is the primary one and uses the new enum to differentiate failure modes. The Linux implementation was at sysdeps/unix/sysv/linux/readonly-area.c. > + if (GLRO(dl_readonly_area (ptr, size))) > + return readonly_noerror; OK. First try to verify that it is read-only by iterating the link map. If it is able to verify that the entire block (of size length) is read-only, it will return true. Otherwise, it will return false. > + > + return __readonly_area_arch (ptr, size); > } OK. Then we use the fallback (which will read from /proc on Linux). > 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" OK. Rebuild the other module separately, for dlopen'ing. > diff --git a/debug/tst-sprintf-fortify-rdonly-mod.c b/debug/tst-sprintf-fortify-rdonly-mod.c > new file mode 100644 > index 0000000000..def15f1ac4 > --- /dev/null > +++ b/debug/tst-sprintf-fortify-rdonly-mod.c OK. New module for test. > @@ -0,0 +1,53 @@ > +/* 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 <support/support.h> > + > +static const char *str2 = "F"; OK. We use this as a placeholder string to be written for every %s. > +static char buf2[10] = "%s"; > +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n"; Format strings. buf2 is writeable, buf3 is relro (and therefore not writeable). Maybe we could use more descriptive names. I see that these are inherited from the existing test. Perhaps writeable_format and relro_format? > + > +void > +init_str2 (void) > +{ > + strcpy (buf2 + 2, "%n%s%n"); > +} > + Should this be init_buf2? Or, if you rename the variable, it could become init_writeable_format e.g.. > +int > +sprintf_buf2 (int *n1, int *n2) > +{ > + char buf[128]; > + return sprintf (buf, buf2, str2, n1, str2, n2); > +} OK. > + > +int > +sprintf_buf3 (int *n1, int *n2) > +{ > + char buf[128]; > + return sprintf (buf, buf3, str2, n1, str2, n2); > +} OK. > + > +int > +sprintf_buf2_malloc (int *n1, int *n2) > +{ > + char buf[128]; > + char *buf2_malloc = xstrdup (buf2); > + return sprintf (buf, buf2_malloc, str2, n1, str2, n2); > +} OK. > diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c > index ba5a791020..432adba370 100644 > --- a/debug/tst-sprintf-fortify-rdonly.c > +++ b/debug/tst-sprintf-fortify-rdonly.c > @@ -27,16 +27,63 @@ > #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 jmp_buf chk_fail_buf; > +static volatile int ret; > +static bool chk_fail_ok; OK. > > -const char *str2 = "F"; > -char buf2[10] = "%s"; OK. These declarations simply moved further down. > +static void > +handler (int sig) > +{ > + if (chk_fail_ok) > + { > + chk_fail_ok = false; > + longjmp (chk_fail_buf, 1); > + } > + else > + _exit (127); > +} Fortify test signal handler. It will be called upon SIGABRT. If the check_fail_ok is true, we are inside a CHECK_FAIL_START / CHECK_FAIL_END scope, which is expected. We then reset the flag and return to the CHK_FAIL_START sigsetjmp. Shouldn't this be a siglongjmp with a sigjmp_buf, to correspond to the sigsetjmp? > + > +#define FORTIFY_FAIL \ > + do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0) OK. > +#define CHK_FAIL_START \ > + chk_fail_ok = true; \ > + if (! sigsetjmp (chk_fail_buf, 1)) \ > + { Also accordingly needs a sigjmp_buf, otherwise OK. Once the check inside this scope leads to a SIGABRT, we will return from this point as if sigsetjmp returned 1, thus skipping the rest of the scope and not executing the FORTIFY_FAIL below. > +#define CHK_FAIL_END \ > + chk_fail_ok = false; \ > + FORTIFY_FAIL; \ > + } OK. Whenever a test expected to SIGABRT does *not* sigabrt, we will execute the FORTIFY_FAIL here, thus logging an error that the fortification did not abort as expected. > + > +static const char *str2 = "F"; > +static char buf2[10] = "%s"; > +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n"; OK. Local buffers. > + > +extern void init_str2 (void); > +extern int sprintf_buf2 (int *, int *); > +extern int sprintf_buf3 (int *, int *); > +extern int sprintf_buf2_malloc (int *, int *); OK. These should get picked up from the module. > + > +#define str(__x) # __x > +void (*init_str2_dlopen)(void); > +int (*sprintf_buf2_dlopen)(int *, int *); > +int (*sprintf_buf3_dlopen)(int *, int *); > +int (*sprintf_buf2_malloc_dlopen)(int *, int *); OK. These should get populated from the dlopen'd module. > > static int > do_test (void) > { > + set_fortify_handler (handler); OK. Set up the handler that will facilitate the checks for expected fortification errors. > + > + { > + void *h = xdlopen ("tst-sprintf-fortify-rdonly-dlopen.so", RTLD_NOW); > + init_str2_dlopen = xdlsym (h, str(init_str2)); > + sprintf_buf2_dlopen = xdlsym (h, str(sprintf_buf2)); > + sprintf_buf3_dlopen = xdlsym (h, str(sprintf_buf3)); > + sprintf_buf2_malloc_dlopen = xdlsym (h, str(sprintf_buf2_malloc)); > + } > + OK. > struct rlimit rl; > int max_fd = 24; > > @@ -63,20 +110,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; > + init_str2 (); > + init_str2_dlopen (); > + > + /* buf2 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, buf2, str2, &n1, str2, &n2); > + CHK_FAIL_END > + } > + OK. This is a check that expects a fortification failure, hence the CHK_FAIL_* scope. > + /* Same as before, but from an library. */ > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_buf2 (&n1, &n2); > + CHK_FAIL_END > + } OK. Same. > + > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_buf2_dlopen (&n1, &n2); > + CHK_FAIL_END > + } OK. Same. > + > + /* buf3 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, buf3, str2, &n1, str2, &n2) != 2 > + || n1 != 1 || n2 != 2) > + FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2); > + } OK. > + > + /* Same as before, but from an library. */ > + { > + int n1; > + int n2; > + if (sprintf_buf3 (&n1, &n2) != 2 || n1 != 1 || n2 != 2) > + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); > + } OK. Same. > + > + { > + int n1; > + int n2; > + if (sprintf_buf3_dlopen (&n1, &n2) != 2 || n1 != 1 || n2 != 2) > + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); > + } OK. Same. > + > + /* 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 (buf2); > + CHK_FAIL_START > + sprintf (buf, buf2_malloc, str2, &n1, str2, &n2); > + CHK_FAIL_END > + } OK. > + > + /* Same as before, but from an library. */ > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_buf2_malloc (&n1, &n2); > + CHK_FAIL_END > + } OK. > + > + { > + int n1; > + int n2; > + CHK_FAIL_START > + sprintf_buf2_malloc_dlopen (&n1, &n2); > + CHK_FAIL_END > + } OK. > + > + return ret; OK. This ensures that any failures logged by CHK_FAIL_END lead to a test failure. All the positive test cases use EXIT_FAIL1 instead. > } > > #include <support/test-driver.c> > diff --git a/elf/Makefile b/elf/Makefile > index 1ea0e7037e..6675302a87 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -72,6 +72,7 @@ dl-routines = \ > dl-open \ > dl-origin \ > dl-printf \ > + dl-readonly-area \ OK. This is for _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..09a98319db > --- /dev/null > +++ b/elf/dl-readonly-area.c OK. New implementation. > @@ -0,0 +1,84 @@ > +/* 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) > +{ > + for (const ElfW(Phdr) *ph = l->l_phdr; ph < &l->l_phdr[l->l_phnum]; ++ph) > + if (ph->p_type == PT_GNU_RELRO) > + { > + uintptr_t relro_start = ALIGN_DOWN (l->l_addr + ph->p_vaddr, > + GLRO(dl_pagesize)); > + uintptr_t relro_end = ALIGN_DOWN (l->l_addr + ph->p_vaddr > + + ph->p_memsz, GLRO(dl_pagesize)); > + /* RELRO is caved out from a RW segment, so the next range is either > + RW or nonexistent. */ > + return relro_start < start && relro_end >= end; > + } > + return false; > +} The link map has an l_relro_addr and an l_relro_size that could be used directly instead of the loop here. > + > +bool > +_dl_readonly_area (const void *ptr, size_t size) > +{ > + struct dl_find_object dlfo; > + if (_dl_find_object ((void *)ptr, &dlfo) != 0) > + return false; > + > + 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) OK. > + { > + if (ph->p_flags & PF_W) > + return check_relro (l, ptr_start, ptr_end); OK. > + > + if ((ph->p_flags & PF_R) == 0) > + return readonly_area_writable; This should return a boolean false in line with the return type. > + > + if (from < ptr_start && to >= ptr_end) > + return true; I think this should be "from <= ptr_start" to allow for the possibility that the string starts at the beginning of the section. > + else if (from < ptr_start) > + size -= to - ptr_start; At this point we know we have a partial and only partial overlap, and that the section begins at-or-before the string's beginning and doesn't cover the end of the string. The size accounted for is from the beginning of the string to the end of the section. This looks OK except again, unless I'm missing some detail, the comparison should be adjusted to "from <= ptr_start" to account for the section and string starting at the same boundary. > + else if (to >= ptr_end) > + size -= ptr_end - from; OK. This is similar but for partial coverage towards the end of the string. The comparison is correct, using ">=" to allow for the string and section to end at the same boundary. > + else > + size -= to - from; OK. Finally this case corresponds to the section only covering some middle part of the string. i.e. the string starts before the section and ends after it. > + > + if (size == 0) > + break; OK. We accounted for the entire string's length. > + } > + } > + > + return size == 0; > +} OK. If we have accounted for the entire string's length being read-only, potentially split across multiple sections, we return true. Otherwise, we return false. > diff --git a/elf/rtld.c b/elf/rtld.c > index 115f1da37f..69b0c9e111 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -372,6 +372,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, > }; OK. Initialize the function pointer in rtld_global_ro. > /* 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..6b554ba56f 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, > +}; > + OK. Now we differentiate between multiple error modes when it comes to verifying read-only format specifiers. > +extern enum readonly_error_type __readonly_area (const void *ptr, > + size_t size) > + attribute_hidden; > +extern enum readonly_error_type __readonly_area_arch (const void *ptr, > + size_t size) > + attribute_hidden; > + OK. The main and system specific read-only area checker functions. > # 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, OK. Use enum to clearly describe the type of error. > 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; OK. Again, use the new enum. > > /* 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, OK. Same. > 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..98dc9844d4 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 segment 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; > + } Small typo here: "segments". Otherwise OK. The usage of readonly_format is adjusted to use the enum instead of int. __readonly_area will now use the new implementation that returns the enum. Then we handle all possible enum values. > } > /* Answer the count of characters written. */ > void *ptrptr = process_arg_pointer (); > diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h > index 8465cbaa9b..ac95992ac8 100644 > --- a/sysdeps/generic/ldsodefs.h > +++ b/sysdeps/generic/ldsodefs.h > @@ -676,6 +676,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. */ > + bool (*_dl_readonly_area) (const void *, size_t); > + OK. Entry in rtld_global_ro struct. > /* Dynamic linker operations used after static dlopen. */ > const struct dlfcn_hook *_dl_dlfcn_hook; > > @@ -1280,6 +1284,9 @@ 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 bool _dl_readonly_area (const void *ptr, size_t size) > + attribute_hidden; > + OK. > /* 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-arch.c > similarity index 90% > rename from sysdeps/mach/readonly-area.c > rename to sysdeps/mach/readonly-area-arch.c > index fb89413fe6..b1bb1cba39 100644 > --- a/sysdeps/mach/readonly-area.c > +++ b/sysdeps/mach/readonly-area-arch.c OK. The old Linux and mach implementations are now a back-up and only called when the new link map based implementation doesn't return a useful result. > @@ -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) OK. Adjust for new enum. > { > 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; OK. Same. > > if (region_address - (uintptr_t) ptr >= size) > break; > } > > - return 1; > + return readonly_noerror; OK. Same. > } > diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area-arch.c > similarity index 84% > rename from sysdeps/unix/sysv/linux/readonly-area.c > rename to sysdeps/unix/sysv/linux/readonly-area-arch.c > index 62d2070c72..3a33a9ceb4 100644 > --- a/sysdeps/unix/sysv/linux/readonly-area.c > +++ b/sysdeps/unix/sysv/linux/readonly-area-arch.c OK. This is the Linux implementation that's now a back-up to the new link-map one. > @@ -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_arch (const void *ptr, size_t size) OK. Adjust for new enum. > { > 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; OK. Previously we treated ENOENT, EACCESS and EMFILE errors in the same way that we treated finding a readonly-area, i.e. no errors. > - return -1; OK. This corresponds to encountering any other fopen error and assuming writable area. > + || errno == EACCES) > + return readonly_procfs_inaccessible; OK. Now, we treat ENOENT and EACCESS as procfs being inaccessible. > + /* Process has reached the maximum number of open files or another > + unusual error. */ > + return readonly_procfs_open_fail; OK. All other fopen errors (including EMFILE) are treated as a failure to open procfs. > } > > /* 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; OK. Adjust for new enum. > } > -- > 2.43.0 >
On 13/03/25 20:41, Arjun Shankar wrote: > Hi Adhemerval, > > Took me a while to go through it since I had to learn about some of > the context before I could meaningfully comment on the patch. I > learned something while reviewing this. Thanks! > > The patch seems mostly okay to me, but I have some comments below. Thanks for the review, I will send an updated version. > > Cheers! > Arjun > >> 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 afallback. >> >> Checked on x86_64-linux-gnu and i686-linux-gnu. > > Small typo: "afallback". > >> --- >> Makeconfig | 2 +- >> debug/Makefile | 14 ++ >> debug/readonly-area.c | 17 +- >> debug/tst-sprintf-fortify-rdonly-dlopen.c | 1 + >> debug/tst-sprintf-fortify-rdonly-mod.c | 53 ++++++ >> debug/tst-sprintf-fortify-rdonly.c | 155 ++++++++++++++++-- >> elf/Makefile | 1 + >> elf/dl-readonly-area.c | 84 ++++++++++ >> 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 | 7 + >> .../{readonly-area.c => readonly-area-arch.c} | 11 +- >> .../{readonly-area.c => readonly-area-arch.c} | 21 +-- >> 15 files changed, 355 insertions(+), 64 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-arch.c} (90%) >> rename sysdeps/unix/sysv/linux/{readonly-area.c => readonly-area-arch.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 > > OK. Looks like this is needed for the test to be able to dlopen the test module. > >> 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..be432f7740 100644 >> --- a/debug/Makefile >> +++ b/debug/Makefile >> @@ -74,6 +74,7 @@ routines = \ >> readlink_chk \ >> readlinkat_chk \ >> readonly-area \ >> + readonly-area-arch \ > > This is the fallback implementation for checking read-only status of > an address range. > The -arch suffix seems a bit confusing to me as a name. How about > readonly-area-fallback.c? Ok, I will change to that. > >> 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 > > OK. Build test modules with fortification on so that the checks we > test for are performed. > >> 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) >> + > > Florian was mentioning that linking test modules against libsupport > might not work properly. I don't remember the details though. This is only required because of xstrdup, I will replace with strdup + abort. > >> # _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 >> + > > OK. New test modules. > >> ifeq ($(have-ssp),yes) >> tests += tst-ssp-1 >> endif > >> diff --git a/debug/readonly-area.c b/debug/readonly-area.c >> index 04b437ed8d..e53bc00f9f 100644 >> --- a/debug/readonly-area.c >> +++ b/debug/readonly-area.c >> @@ -16,18 +16,13 @@ >> <https://www.gnu.org/licenses/>. */ >> >> #include <stdlib.h> >> +#include <ldsodefs.h> > > OK. > >> >> -/* 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; > > OK. This used to be a stub implementation but now it is the primary > one and uses the new enum to differentiate failure modes. The Linux > implementation was at sysdeps/unix/sysv/linux/readonly-area.c. > >> + if (GLRO(dl_readonly_area (ptr, size))) >> + return readonly_noerror; > > OK. First try to verify that it is read-only by iterating the link > map. If it is able to verify that the entire block (of size length) is > read-only, it will return true. Otherwise, it will return false. > >> + >> + return __readonly_area_arch (ptr, size); >> } > > OK. Then we use the fallback (which will read from /proc on Linux). > >> 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" > > OK. Rebuild the other module separately, for dlopen'ing. > >> diff --git a/debug/tst-sprintf-fortify-rdonly-mod.c b/debug/tst-sprintf-fortify-rdonly-mod.c >> new file mode 100644 >> index 0000000000..def15f1ac4 >> --- /dev/null >> +++ b/debug/tst-sprintf-fortify-rdonly-mod.c > > OK. New module for test. > >> @@ -0,0 +1,53 @@ >> +/* 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 <support/support.h> >> + >> +static const char *str2 = "F"; > > OK. We use this as a placeholder string to be written for every %s. > >> +static char buf2[10] = "%s"; >> +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n"; > > Format strings. buf2 is writeable, buf3 is relro (and therefore not writeable). > Maybe we could use more descriptive names. I see that these are > inherited from the existing test. Perhaps writeable_format and > relro_format? > >> + >> +void >> +init_str2 (void) >> +{ >> + strcpy (buf2 + 2, "%n%s%n"); >> +} >> + > > Should this be init_buf2? Or, if you rename the variable, it could > become init_writeable_format e.g.. Ack. > >> +int >> +sprintf_buf2 (int *n1, int *n2) >> +{ >> + char buf[128]; >> + return sprintf (buf, buf2, str2, n1, str2, n2); >> +} > > OK. > >> + >> +int >> +sprintf_buf3 (int *n1, int *n2) >> +{ >> + char buf[128]; >> + return sprintf (buf, buf3, str2, n1, str2, n2); >> +} > > OK. > >> + >> +int >> +sprintf_buf2_malloc (int *n1, int *n2) >> +{ >> + char buf[128]; >> + char *buf2_malloc = xstrdup (buf2); >> + return sprintf (buf, buf2_malloc, str2, n1, str2, n2); >> +} > > OK. > >> diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c >> index ba5a791020..432adba370 100644 >> --- a/debug/tst-sprintf-fortify-rdonly.c >> +++ b/debug/tst-sprintf-fortify-rdonly.c >> @@ -27,16 +27,63 @@ >> #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 jmp_buf chk_fail_buf; >> +static volatile int ret; >> +static bool chk_fail_ok; > > OK. > >> >> -const char *str2 = "F"; >> -char buf2[10] = "%s"; > > OK. These declarations simply moved further down. > >> +static void >> +handler (int sig) >> +{ >> + if (chk_fail_ok) >> + { >> + chk_fail_ok = false; >> + longjmp (chk_fail_buf, 1); >> + } >> + else >> + _exit (127); >> +} > > Fortify test signal handler. It will be called upon SIGABRT. If the > check_fail_ok is true, we are inside a CHECK_FAIL_START / > CHECK_FAIL_END scope, which is expected. We then reset the flag and > return to the CHK_FAIL_START sigsetjmp. > > Shouldn't this be a siglongjmp with a sigjmp_buf, to correspond to the > sigsetjmp? It would be more conformant, however the __longjmp_chk (which would be generated by using _FORTIFY_SOURCE) will be essentially the siglongjmp (debug/longjmp_chk.c). I will change it. > >> + >> +#define FORTIFY_FAIL \ >> + do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0) > > OK. > >> +#define CHK_FAIL_START \ >> + chk_fail_ok = true; \ >> + if (! sigsetjmp (chk_fail_buf, 1)) \ >> + { > > Also accordingly needs a sigjmp_buf, otherwise OK. > > Once the check inside this scope leads to a SIGABRT, we will return > from this point as if sigsetjmp returned 1, thus skipping the rest of > the scope and not executing the FORTIFY_FAIL below. > >> +#define CHK_FAIL_END \ >> + chk_fail_ok = false; \ >> + FORTIFY_FAIL; \ >> + } > > OK. Whenever a test expected to SIGABRT does *not* sigabrt, we will > execute the FORTIFY_FAIL here, thus logging an error that the > fortification did not abort as expected. > >> + >> +static const char *str2 = "F"; >> +static char buf2[10] = "%s"; >> +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n"; > > OK. Local buffers. > >> + >> +extern void init_str2 (void); >> +extern int sprintf_buf2 (int *, int *); >> +extern int sprintf_buf3 (int *, int *); >> +extern int sprintf_buf2_malloc (int *, int *); > > OK. These should get picked up from the module. > >> + >> +#define str(__x) # __x >> +void (*init_str2_dlopen)(void); >> +int (*sprintf_buf2_dlopen)(int *, int *); >> +int (*sprintf_buf3_dlopen)(int *, int *); >> +int (*sprintf_buf2_malloc_dlopen)(int *, int *); > > OK. These should get populated from the dlopen'd module. > >> >> static int >> do_test (void) >> { >> + set_fortify_handler (handler); > > OK. Set up the handler that will facilitate the checks for expected > fortification errors. > >> + >> + { >> + void *h = xdlopen ("tst-sprintf-fortify-rdonly-dlopen.so", RTLD_NOW); >> + init_str2_dlopen = xdlsym (h, str(init_str2)); >> + sprintf_buf2_dlopen = xdlsym (h, str(sprintf_buf2)); >> + sprintf_buf3_dlopen = xdlsym (h, str(sprintf_buf3)); >> + sprintf_buf2_malloc_dlopen = xdlsym (h, str(sprintf_buf2_malloc)); >> + } >> + > > OK. > >> struct rlimit rl; >> int max_fd = 24; >> >> @@ -63,20 +110,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; > >> + init_str2 (); >> + init_str2_dlopen (); >> + >> + /* buf2 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, buf2, str2, &n1, str2, &n2); >> + CHK_FAIL_END >> + } >> + > > OK. This is a check that expects a fortification failure, hence the > CHK_FAIL_* scope. > >> + /* Same as before, but from an library. */ >> + { >> + int n1; >> + int n2; >> + CHK_FAIL_START >> + sprintf_buf2 (&n1, &n2); >> + CHK_FAIL_END >> + } > > OK. Same. > >> + >> + { >> + int n1; >> + int n2; >> + CHK_FAIL_START >> + sprintf_buf2_dlopen (&n1, &n2); >> + CHK_FAIL_END >> + } > > OK. Same. > >> + >> + /* buf3 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, buf3, str2, &n1, str2, &n2) != 2 >> + || n1 != 1 || n2 != 2) >> + FAIL_EXIT1 ("sprintf failed: %s %d %d", buf, n1, n2); >> + } > > OK. > >> + >> + /* Same as before, but from an library. */ >> + { >> + int n1; >> + int n2; >> + if (sprintf_buf3 (&n1, &n2) != 2 || n1 != 1 || n2 != 2) >> + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); >> + } > > OK. Same. > >> + >> + { >> + int n1; >> + int n2; >> + if (sprintf_buf3_dlopen (&n1, &n2) != 2 || n1 != 1 || n2 != 2) >> + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); >> + } > > OK. Same. > >> + >> + /* 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 (buf2); >> + CHK_FAIL_START >> + sprintf (buf, buf2_malloc, str2, &n1, str2, &n2); >> + CHK_FAIL_END >> + } > > OK. > >> + >> + /* Same as before, but from an library. */ >> + { >> + int n1; >> + int n2; >> + CHK_FAIL_START >> + sprintf_buf2_malloc (&n1, &n2); >> + CHK_FAIL_END >> + } > > OK. > >> + >> + { >> + int n1; >> + int n2; >> + CHK_FAIL_START >> + sprintf_buf2_malloc_dlopen (&n1, &n2); >> + CHK_FAIL_END >> + } > > OK. > >> + >> + return ret; > > OK. This ensures that any failures logged by CHK_FAIL_END lead to a > test failure. All the positive test cases use EXIT_FAIL1 instead. > >> } >> >> #include <support/test-driver.c> > >> diff --git a/elf/Makefile b/elf/Makefile >> index 1ea0e7037e..6675302a87 100644 >> --- a/elf/Makefile >> +++ b/elf/Makefile >> @@ -72,6 +72,7 @@ dl-routines = \ >> dl-open \ >> dl-origin \ >> dl-printf \ >> + dl-readonly-area \ > > OK. This is for _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..09a98319db >> --- /dev/null >> +++ b/elf/dl-readonly-area.c > > OK. New implementation. > >> @@ -0,0 +1,84 @@ >> +/* 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) >> +{ >> + for (const ElfW(Phdr) *ph = l->l_phdr; ph < &l->l_phdr[l->l_phnum]; ++ph) >> + if (ph->p_type == PT_GNU_RELRO) >> + { >> + uintptr_t relro_start = ALIGN_DOWN (l->l_addr + ph->p_vaddr, >> + GLRO(dl_pagesize)); >> + uintptr_t relro_end = ALIGN_DOWN (l->l_addr + ph->p_vaddr >> + + ph->p_memsz, GLRO(dl_pagesize)); >> + /* RELRO is caved out from a RW segment, so the next range is either >> + RW or nonexistent. */ >> + return relro_start < start && relro_end >= end; >> + } >> + return false; >> +} > > The link map has an l_relro_addr and an l_relro_size that could be > used directly instead of the loop here. Right, I forgot about these. > >> + >> +bool >> +_dl_readonly_area (const void *ptr, size_t size) >> +{ >> + struct dl_find_object dlfo; >> + if (_dl_find_object ((void *)ptr, &dlfo) != 0) >> + return false; >> + >> + 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) > > OK. > >> + { >> + if (ph->p_flags & PF_W) >> + return check_relro (l, ptr_start, ptr_end); > > OK. > >> + >> + if ((ph->p_flags & PF_R) == 0) >> + return readonly_area_writable; > > This should return a boolean false in line with the return type. In fact I realized that _dl_readonly_area should return either if the input in the read-only segment, read-write segment, and not found. The fallback should only be called if range is not found. I will change it. > >> + >> + if (from < ptr_start && to >= ptr_end) >> + return true; > > I think this should be "from <= ptr_start" to allow for the > possibility that the string starts at the beginning of the section. It should be, and the origin __readonly_area also does it. I will fix it. > >> + else if (from < ptr_start) >> + size -= to - ptr_start; > > At this point we know we have a partial and only partial overlap, and > that the section begins at-or-before the string's beginning and > doesn't cover the end of the string. The size accounted for is from > the beginning of the string to the end of the section. This looks OK > except again, unless I'm missing some detail, the comparison should be > adjusted to "from <= ptr_start" to account for the section and string > starting at the same boundary. You are right, this is another mistake I did by adapting the sysdeps/unix/sysv/linux/readonly-area.c. > >> + else if (to >= ptr_end) >> + size -= ptr_end - from; > > OK. This is similar but for partial coverage towards the end of the > string. The comparison is correct, using ">=" to allow for the string > and section to end at the same boundary. > >> + else >> + size -= to - from; > > OK. Finally this case corresponds to the section only covering some > middle part of the string. i.e. the string starts before the section > and ends after it. > >> + >> + if (size == 0) >> + break; > > OK. We accounted for the entire string's length. > >> + } >> + } >> + >> + return size == 0; >> +} > > OK. If we have accounted for the entire string's length being > read-only, potentially split across multiple sections, we return true. > Otherwise, we return false. > >> diff --git a/elf/rtld.c b/elf/rtld.c >> index 115f1da37f..69b0c9e111 100644 >> --- a/elf/rtld.c >> +++ b/elf/rtld.c >> @@ -372,6 +372,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, >> }; > > OK. Initialize the function pointer in rtld_global_ro. > >> /* 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..6b554ba56f 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, >> +}; >> + > > OK. Now we differentiate between multiple error modes when it comes to > verifying read-only format specifiers. > >> +extern enum readonly_error_type __readonly_area (const void *ptr, >> + size_t size) >> + attribute_hidden; >> +extern enum readonly_error_type __readonly_area_arch (const void *ptr, >> + size_t size) >> + attribute_hidden; >> + > > OK. The main and system specific read-only area checker functions. > >> # 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, > > OK. Use enum to clearly describe the type of error. > >> 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; > > OK. Again, use the new enum. > >> >> /* 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, > > OK. Same. > >> 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..98dc9844d4 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 segment 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; >> + } > > Small typo here: "segments". > Otherwise OK. The usage of readonly_format is adjusted to use the enum > instead of int. __readonly_area will now use the new implementation > that returns the enum. Then we handle all possible enum values. > >> } >> /* Answer the count of characters written. */ >> void *ptrptr = process_arg_pointer (); >> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h >> index 8465cbaa9b..ac95992ac8 100644 >> --- a/sysdeps/generic/ldsodefs.h >> +++ b/sysdeps/generic/ldsodefs.h >> @@ -676,6 +676,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. */ >> + bool (*_dl_readonly_area) (const void *, size_t); >> + > > OK. Entry in rtld_global_ro struct. > >> /* Dynamic linker operations used after static dlopen. */ >> const struct dlfcn_hook *_dl_dlfcn_hook; >> >> @@ -1280,6 +1284,9 @@ 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 bool _dl_readonly_area (const void *ptr, size_t size) >> + attribute_hidden; >> + > > OK. > >> /* 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-arch.c >> similarity index 90% >> rename from sysdeps/mach/readonly-area.c >> rename to sysdeps/mach/readonly-area-arch.c >> index fb89413fe6..b1bb1cba39 100644 >> --- a/sysdeps/mach/readonly-area.c >> +++ b/sysdeps/mach/readonly-area-arch.c > > OK. The old Linux and mach implementations are now a back-up and only > called when the new link map based implementation doesn't return a > useful result. > >> @@ -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) > > OK. Adjust for new enum. > >> { >> 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; > > OK. Same. > >> >> if (region_address - (uintptr_t) ptr >= size) >> break; >> } >> >> - return 1; >> + return readonly_noerror; > > OK. Same. > >> } > >> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area-arch.c >> similarity index 84% >> rename from sysdeps/unix/sysv/linux/readonly-area.c >> rename to sysdeps/unix/sysv/linux/readonly-area-arch.c >> index 62d2070c72..3a33a9ceb4 100644 >> --- a/sysdeps/unix/sysv/linux/readonly-area.c >> +++ b/sysdeps/unix/sysv/linux/readonly-area-arch.c > > OK. This is the Linux implementation that's now a back-up to the new > link-map one. > >> @@ -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_arch (const void *ptr, size_t size) > > OK. Adjust for new enum. > >> { >> 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; > > OK. Previously we treated ENOENT, EACCESS and EMFILE errors in the > same way that we treated finding a readonly-area, i.e. no errors. > >> - return -1; > > OK. This corresponds to encountering any other fopen error and > assuming writable area. > >> + || errno == EACCES) >> + return readonly_procfs_inaccessible; > > OK. Now, we treat ENOENT and EACCESS as procfs being inaccessible. > >> + /* Process has reached the maximum number of open files or another >> + unusual error. */ >> + return readonly_procfs_open_fail; > > OK. All other fopen errors (including EMFILE) are treated as a failure > to open procfs. > >> } >> >> /* 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; > > OK. Adjust for new enum. > >> } >> -- >> 2.43.0 >> >
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..be432f7740 100644 --- a/debug/Makefile +++ b/debug/Makefile @@ -74,6 +74,7 @@ routines = \ readlink_chk \ readlinkat_chk \ readonly-area \ + readonly-area-arch \ 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..e53bc00f9f 100644 --- a/debug/readonly-area.c +++ b/debug/readonly-area.c @@ -16,18 +16,13 @@ <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; + if (GLRO(dl_readonly_area (ptr, size))) + return readonly_noerror; + + return __readonly_area_arch (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..def15f1ac4 --- /dev/null +++ b/debug/tst-sprintf-fortify-rdonly-mod.c @@ -0,0 +1,53 @@ +/* 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 <support/support.h> + +static const char *str2 = "F"; +static char buf2[10] = "%s"; +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n"; + +void +init_str2 (void) +{ + strcpy (buf2 + 2, "%n%s%n"); +} + +int +sprintf_buf2 (int *n1, int *n2) +{ + char buf[128]; + return sprintf (buf, buf2, str2, n1, str2, n2); +} + +int +sprintf_buf3 (int *n1, int *n2) +{ + char buf[128]; + return sprintf (buf, buf3, str2, n1, str2, n2); +} + +int +sprintf_buf2_malloc (int *n1, int *n2) +{ + char buf[128]; + char *buf2_malloc = xstrdup (buf2); + 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..432adba370 100644 --- a/debug/tst-sprintf-fortify-rdonly.c +++ b/debug/tst-sprintf-fortify-rdonly.c @@ -27,16 +27,63 @@ #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 jmp_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 buf2[10] = "%s"; +static char buf3[10] __attribute__ ((section (".data.rel.ro"))) = "%s%n%s%n"; + +extern void init_str2 (void); +extern int sprintf_buf2 (int *, int *); +extern int sprintf_buf3 (int *, int *); +extern int sprintf_buf2_malloc (int *, int *); + +#define str(__x) # __x +void (*init_str2_dlopen)(void); +int (*sprintf_buf2_dlopen)(int *, int *); +int (*sprintf_buf3_dlopen)(int *, int *); +int (*sprintf_buf2_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_str2_dlopen = xdlsym (h, str(init_str2)); + sprintf_buf2_dlopen = xdlsym (h, str(sprintf_buf2)); + sprintf_buf3_dlopen = xdlsym (h, str(sprintf_buf3)); + sprintf_buf2_malloc_dlopen = xdlsym (h, str(sprintf_buf2_malloc)); + } + struct rlimit rl; int max_fd = 24; @@ -63,20 +110,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; + init_str2 (); + init_str2_dlopen (); + + /* buf2 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, buf2, str2, &n1, str2, &n2); + CHK_FAIL_END + } + + /* Same as before, but from an library. */ + { + int n1; + int n2; + CHK_FAIL_START + sprintf_buf2 (&n1, &n2); + CHK_FAIL_END + } + + { + int n1; + int n2; + CHK_FAIL_START + sprintf_buf2_dlopen (&n1, &n2); + CHK_FAIL_END + } + + /* buf3 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, buf3, 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_buf3 (&n1, &n2) != 2 || n1 != 1 || n2 != 2) + FAIL_EXIT1 ("sprintf failed: %d %d", n1, n2); + } + + { + int n1; + int n2; + if (sprintf_buf3_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 (buf2); + 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_buf2_malloc (&n1, &n2); + CHK_FAIL_END + } + + { + int n1; + int n2; + CHK_FAIL_START + sprintf_buf2_malloc_dlopen (&n1, &n2); + CHK_FAIL_END + } + + return ret; } #include <support/test-driver.c> diff --git a/elf/Makefile b/elf/Makefile index 1ea0e7037e..6675302a87 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..09a98319db --- /dev/null +++ b/elf/dl-readonly-area.c @@ -0,0 +1,84 @@ +/* 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) +{ + for (const ElfW(Phdr) *ph = l->l_phdr; ph < &l->l_phdr[l->l_phnum]; ++ph) + if (ph->p_type == PT_GNU_RELRO) + { + uintptr_t relro_start = ALIGN_DOWN (l->l_addr + ph->p_vaddr, + GLRO(dl_pagesize)); + uintptr_t relro_end = ALIGN_DOWN (l->l_addr + ph->p_vaddr + + ph->p_memsz, GLRO(dl_pagesize)); + /* RELRO is caved out from a RW segment, so the next range is either + RW or nonexistent. */ + return relro_start < start && relro_end >= end; + } + return false; +} + +bool +_dl_readonly_area (const void *ptr, size_t size) +{ + struct dl_find_object dlfo; + if (_dl_find_object ((void *)ptr, &dlfo) != 0) + return false; + + 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 readonly_area_writable; + + if (from < ptr_start && to >= ptr_end) + return true; + 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; +} diff --git a/elf/rtld.c b/elf/rtld.c index 115f1da37f..69b0c9e111 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -372,6 +372,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..6b554ba56f 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_arch (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..98dc9844d4 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 segment 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 8465cbaa9b..ac95992ac8 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -676,6 +676,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. */ + bool (*_dl_readonly_area) (const void *, size_t); + /* Dynamic linker operations used after static dlopen. */ const struct dlfcn_hook *_dl_dlfcn_hook; @@ -1280,6 +1284,9 @@ 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 bool _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-arch.c similarity index 90% rename from sysdeps/mach/readonly-area.c rename to sysdeps/mach/readonly-area-arch.c index fb89413fe6..b1bb1cba39 100644 --- a/sysdeps/mach/readonly-area.c +++ b/sysdeps/mach/readonly-area-arch.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-arch.c similarity index 84% rename from sysdeps/unix/sysv/linux/readonly-area.c rename to sysdeps/unix/sysv/linux/readonly-area-arch.c index 62d2070c72..3a33a9ceb4 100644 --- a/sysdeps/unix/sysv/linux/readonly-area.c +++ b/sysdeps/unix/sysv/linux/readonly-area-arch.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_arch (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 afallback. Checked on x86_64-linux-gnu and i686-linux-gnu. --- Makeconfig | 2 +- debug/Makefile | 14 ++ debug/readonly-area.c | 17 +- debug/tst-sprintf-fortify-rdonly-dlopen.c | 1 + debug/tst-sprintf-fortify-rdonly-mod.c | 53 ++++++ debug/tst-sprintf-fortify-rdonly.c | 155 ++++++++++++++++-- elf/Makefile | 1 + elf/dl-readonly-area.c | 84 ++++++++++ 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 | 7 + .../{readonly-area.c => readonly-area-arch.c} | 11 +- .../{readonly-area.c => readonly-area-arch.c} | 21 +-- 15 files changed, 355 insertions(+), 64 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-arch.c} (90%) rename sysdeps/unix/sysv/linux/{readonly-area.c => readonly-area-arch.c} (84%)