Re: [PATCH] Fix FORTIFY_SOURCE false positive

[Siddhesh Poyarekar <siddhesh@gotplt.org>]

On 2023-10-02 14:12, Adhemerval Zanella Netto wrote:
> 
> 
> On 02/10/23 15:00, Siddhesh Poyarekar wrote:
>> On 2023-10-02 11:53, Volker Weißmann wrote:
>>> When -D_FORTIFY_SOURCE=2 was given during compilation,
>>> sprintf and similar functions will check if their
>>> first argument is in read-only memory and exit with
>>> *** %n in writable segment detected ***
>>> otherwise. To check if the memory is read-only, glibc
>>> reads form the file "/proc/self/maps". If opening this
>>> file fails due to too many open files (EMFILE), glibc
>>> will now ignore this error.
>>> ---
>>
>> Ugh, that looks like an easy way to defeat format string fortification :/
> 
> The sprintf '%n' fortify is already brittle to rely on procfs access to check
> if the format string is on a rw segment.
> 
>>
>> The fix is fine I think, just a little nit below.
>>
>>>    sysdeps/unix/sysv/linux/readonly-area.c | 10 +++++++++-
>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c
>>> index edc68873f6..629163461a 100644
>>> --- a/sysdeps/unix/sysv/linux/readonly-area.c
>>> +++ b/sysdeps/unix/sysv/linux/readonly-area.c
>>> @@ -42,7 +42,15 @@ __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)
>>> +      || errno == EACCES
>>> +          /* Example code to trigger EMFILE:
>>> +          while(1) {
>>> +            FILE *file = fopen("/dev/zero", "r");
>>> +            assert(file != NULL);
>>> +          }
>>> +          If your libc was compiled with -D_FORTIFY_SOURCE=2, we run
>>
>> Shouldn't this be "If the program was compiled with..." and not libc? Also, maybe the example code is unnecessary and you could just mention that the if the open file threshold is reached, this could become a spurious failure.
>>
> 
> I will just add a more simpler comment:
> 
> /* Process has reached the maximum number of open files.  */
> 
>>> +          into this if clause here. */
>>> +          || errno == EMFILE)
>>>        return 1;
>>>          return -1;
>>>        }
>>> -- 
>>> 2.42.0
>>>
>>
>> Also, if you don't have a copyright assignment on file with the FSF, could you add a Signed-off-by to certify your contribution?
> 
> I think we should add a regression test for this, with EMFILE is easy to create
> one than the one that check for procfs (which would require a test-container
> that does not mount procfs):

Sounds good.  Once Volker sends a v2 with their S-o-b, could you include 
that and post a patchset with your test?  That way both get credit for 
their work and pre-commit CI also won't complain.

Thanks,
Sid

> 
> diff --git a/debug/Makefile b/debug/Makefile
> index 434e52f780..d7e021a1c8 100644
> --- a/debug/Makefile
> +++ b/debug/Makefile
> @@ -178,6 +178,7 @@ CFLAGS-tst-longjmp_chk3.c += -fexceptions -fasynchronous-unwind-tables
>   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
>   
>   # _FORTIFY_SOURCE tests.
>   # Auto-generate tests for _FORTIFY_SOURCE for different levels, compilers and
> @@ -284,6 +285,7 @@ tests = \
>     tst-longjmp_chk \
>     tst-longjmp_chk2 \
>     tst-realpath-chk \
> +  tst-sprintf-fortify-rdonly \
>     tst-sprintf-fortify-unchecked \
>     # tests
>   
> @@ -291,6 +293,9 @@ tests-time64 += \
>     $(tests-all-time64-chk) \
>     # tests-time64
>   
> +tests-container += \
> +  # tests-container
> +
>   ifeq ($(have-ssp),yes)
>   tests += tst-ssp-1
>   endif
> diff --git a/debug/tst-sprintf-fortify-rdonly.c b/debug/tst-sprintf-fortify-rdonly.c
> new file mode 100644
> index 0000000000..f07e4c7e4a
> --- /dev/null
> +++ b/debug/tst-sprintf-fortify-rdonly.c
> @@ -0,0 +1,81 @@
> +/* Copyright (C) 2023 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 <errno.h>
> +#include <setjmp.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/resource.h>
> +#include <unistd.h>
> +
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/temp_file.h>
> +
> +jmp_buf chk_fail_buf;
> +bool chk_fail_ok;
> +
> +const char *str2 = "F";
> +char buf2[10] = "%s";
> +
> +static int
> +do_test (void)
> +{
> +  struct rlimit rl;
> +  int max_fd = 24;
> +
> +  if (getrlimit (RLIMIT_NOFILE, &rl) == -1)
> +    FAIL_EXIT1 ("getrlimit (RLIMIT_NOFILE): %m");
> +
> +  max_fd = (rl.rlim_cur < max_fd ? rl.rlim_cur : max_fd);
> +  rl.rlim_cur = max_fd;
> +
> +  if (setrlimit (RLIMIT_NOFILE, &rl) == 1)
> +    FAIL_EXIT1 ("setrlimit (RLIMIT_NOFILE): %m");
> +
> +  /* Exhaust the file descriptor limit with temporary files.  */
> +  int nfiles = 0;
> +  for (; nfiles < max_fd; nfiles++)
> +    {
> +      int fd = create_temp_file ("tst-spawn3.", NULL);
> +      if (fd == -1)
> +	{
> +	  if (errno != EMFILE)
> +	    FAIL_EXIT1 ("create_temp_file: %m");
> +	  break;
> +	}
> +    }
> +  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;
> +}
> +
> +#include <support/test-driver.c>