[PATCH v3] Fix FORTIFY_SOURCE false positive

[PATCH v3] Fix FORTIFY_SOURCE false positive

[Volker Weißmann]

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 frpm the file "/proc/self/maps". If opening this
file fails due to too many open files (EMFILE), glibc
will now ignore this error.

Fixes [BZ #30932]

Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de>
---
 sysdeps/unix/sysv/linux/readonly-area.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c
index edc68873f6..ba32372ebb 100644
--- a/sysdeps/unix/sysv/linux/readonly-area.c
+++ b/sysdeps/unix/sysv/linux/readonly-area.c
@@ -42,7 +42,9 @@ __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
+	  /* Process has reached the maximum number of open files.  */
+	  || errno == EMFILE)
 	return 1;
       return -1;
     }
--
2.42.0

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Siddhesh Poyarekar]

On 2023-10-03 13:18, 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 frpm the file "/proc/self/maps". If opening this
> file fails due to too many open files (EMFILE), glibc
> will now ignore this error.
> 
> Fixes [BZ #30932]
> 
> Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de>
> ---

Thanks!  LGTM.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

Adhemerval, could you please add this with your test case patch and send 
it as a series?  I can then review that too.

Thanks,
Sid

>   sysdeps/unix/sysv/linux/readonly-area.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c
> index edc68873f6..ba32372ebb 100644
> --- a/sysdeps/unix/sysv/linux/readonly-area.c
> +++ b/sysdeps/unix/sysv/linux/readonly-area.c
> @@ -42,7 +42,9 @@ __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
> +	  /* Process has reached the maximum number of open files.  */
> +	  || errno == EMFILE)
>   	return 1;
>         return -1;
>       }
> --
> 2.42.0
> 

[PATCH] debug: Add regression tests for BZ 30932

[Adhemerval Zanella]

Checked on x86_64-linux-gnu.
---
 debug/Makefile                     |  5 ++
 debug/tst-sprintf-fortify-rdonly.c | 82 ++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+)
 create mode 100644 debug/tst-sprintf-fortify-rdonly.c

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..7d5c447598
--- /dev/null
+++ b/debug/tst-sprintf-fortify-rdonly.c
@@ -0,0 +1,82 @@
+/* Testcase for BZ 30932.
+   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>
-- 
2.34.1

Re: [PATCH] debug: Add regression tests for BZ 30932

[Siddhesh Poyarekar]

On 2023-10-03 14:13, Adhemerval Zanella wrote:
> Checked on x86_64-linux-gnu.
> ---

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

>   debug/Makefile                     |  5 ++
>   debug/tst-sprintf-fortify-rdonly.c | 82 ++++++++++++++++++++++++++++++
>   2 files changed, 87 insertions(+)
>   create mode 100644 debug/tst-sprintf-fortify-rdonly.c
> 
> 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..7d5c447598
> --- /dev/null
> +++ b/debug/tst-sprintf-fortify-rdonly.c
> @@ -0,0 +1,82 @@
> +/* Testcase for BZ 30932.
> +   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>

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Volker Weißmann]

I thought about my patch again...

If an attacker can make the victim-program leak file descriptors, this
can be used to defeat string fortification.

Since leaking file-descriptors is normally not that bad (normally, it
cannot lead to anything worse than a DOS), programmers/security auditors
might be less careful in ensuring that no fd leaks.

It doesn't even have to be a true leak, image if e.g. the attacker
controls python code that runs inside an interpreter that does some
sandboxing. Then the attacker could do something like:

with open("/dev/zero") as file1:
     with open("/dev/zero") as file2:
     ...
         with open("/dev/zero") as file1023:
             trigger_formatstring_bug_in_the_python_interpreter()

to break out of the sandbox.

I know I'm being a bit paranoid, but glibc is used *everywhere*.

I think instead of "return 1;" we should do

__libc_fatal ("*** too many open file descriptors ***\n");

instead.


On 03.10.23 19:25, Siddhesh Poyarekar wrote:
> On 2023-10-03 13:18, 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 frpm the file "/proc/self/maps". If opening this
>> file fails due to too many open files (EMFILE), glibc
>> will now ignore this error.
>>
>> Fixes [BZ #30932]
>>
>> Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de>
>> ---
>
> Thanks!  LGTM.
>
> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>
> Adhemerval, could you please add this with your test case patch and
> send it as a series?  I can then review that too.
>
> Thanks,
> Sid
>
>>   sysdeps/unix/sysv/linux/readonly-area.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c
>> b/sysdeps/unix/sysv/linux/readonly-area.c
>> index edc68873f6..ba32372ebb 100644
>> --- a/sysdeps/unix/sysv/linux/readonly-area.c
>> +++ b/sysdeps/unix/sysv/linux/readonly-area.c
>> @@ -42,7 +42,9 @@ __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
>> +      /* Process has reached the maximum number of open files. */
>> +      || errno == EMFILE)
>>       return 1;
>>         return -1;
>>       }
>> --
>> 2.42.0
>>

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Andreas Schwab]

On Okt 03 2023, Volker Weißmann wrote:

> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c
> index edc68873f6..ba32372ebb 100644
> --- a/sysdeps/unix/sysv/linux/readonly-area.c
> +++ b/sysdeps/unix/sysv/linux/readonly-area.c
> @@ -42,7 +42,9 @@ __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
> +	  /* Process has reached the maximum number of open files.  */
> +	  || errno == EMFILE)

Should this also handle ENFILE?

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Volker Weißmann]

Yes, you are right. I will write a patch once we decided whether we want
to "return 1" or __libc_fatal ("*** too many open file descriptors ***\n")

On 04.10.23 16:51, Andreas Schwab wrote:
> On Okt 03 2023, Volker Weißmann wrote:
>
>> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c
>> index edc68873f6..ba32372ebb 100644
>> --- a/sysdeps/unix/sysv/linux/readonly-area.c
>> +++ b/sysdeps/unix/sysv/linux/readonly-area.c
>> @@ -42,7 +42,9 @@ __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
>> +	  /* Process has reached the maximum number of open files.  */
>> +	  || errno == EMFILE)
> Should this also handle ENFILE?
>

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Adhemerval Zanella Netto]

On 04/10/23 11:43, Volker Weißmann wrote:
> I thought about my patch again...
> 
> If an attacker can make the victim-program leak file descriptors, this
> can be used to defeat string fortification.
> 
> Since leaking file-descriptors is normally not that bad (normally, it
> cannot lead to anything worse than a DOS), programmers/security auditors
> might be less careful in ensuring that no fd leaks.
> 
> It doesn't even have to be a true leak, image if e.g. the attacker
> controls python code that runs inside an interpreter that does some
> sandboxing. Then the attacker could do something like:
> 
> with open("/dev/zero") as file1:
>     with open("/dev/zero") as file2:
>     ...
>         with open("/dev/zero") as file1023:
>             trigger_formatstring_bug_in_the_python_interpreter()
> 
> to break out of the sandbox.
> 
> I know I'm being a bit paranoid, but glibc is used *everywhere*.
> 
> I think instead of "return 1;" we should do
> 
> __libc_fatal ("*** too many open file descriptors ***\n");
> 
> instead.

The same if also check for procfs support, meaning that if it is not accessible
the process will start to abort execution.  Not sure about what kind of breakage
it would incur, but it should reasonable to abort on both cases since this is
done iff fortify is enabled.


> 
> 
> On 03.10.23 19:25, Siddhesh Poyarekar wrote:
>> On 2023-10-03 13:18, 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 frpm the file "/proc/self/maps". If opening this
>>> file fails due to too many open files (EMFILE), glibc
>>> will now ignore this error.
>>>
>>> Fixes [BZ #30932]
>>>
>>> Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de>
>>> ---
>>
>> Thanks!  LGTM.
>>
>> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
>>
>> Adhemerval, could you please add this with your test case patch and
>> send it as a series?  I can then review that too.
>>
>> Thanks,
>> Sid
>>
>>>   sysdeps/unix/sysv/linux/readonly-area.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c
>>> b/sysdeps/unix/sysv/linux/readonly-area.c
>>> index edc68873f6..ba32372ebb 100644
>>> --- a/sysdeps/unix/sysv/linux/readonly-area.c
>>> +++ b/sysdeps/unix/sysv/linux/readonly-area.c
>>> @@ -42,7 +42,9 @@ __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
>>> +      /* Process has reached the maximum number of open files. */
>>> +      || errno == EMFILE)
>>>       return 1;
>>>         return -1;
>>>       }
>>> -- 
>>> 2.42.0
>>>

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Siddhesh Poyarekar]

On 2023-10-04 12:57, Adhemerval Zanella Netto wrote:
> 
> 
> On 04/10/23 11:43, Volker Weißmann wrote:
>> I thought about my patch again...
>>
>> If an attacker can make the victim-program leak file descriptors, this
>> can be used to defeat string fortification.
>>
>> Since leaking file-descriptors is normally not that bad (normally, it
>> cannot lead to anything worse than a DOS), programmers/security auditors
>> might be less careful in ensuring that no fd leaks.
>>
>> It doesn't even have to be a true leak, image if e.g. the attacker
>> controls python code that runs inside an interpreter that does some
>> sandboxing. Then the attacker could do something like:
>>
>> with open("/dev/zero") as file1:
>>      with open("/dev/zero") as file2:
>>      ...
>>          with open("/dev/zero") as file1023:
>>              trigger_formatstring_bug_in_the_python_interpreter()
>>
>> to break out of the sandbox.
>>
>> I know I'm being a bit paranoid, but glibc is used *everywhere*.
>>
>> I think instead of "return 1;" we should do
>>
>> __libc_fatal ("*** too many open file descriptors ***\n");
>>
>> instead.
> 
> The same if also check for procfs support, meaning that if it is not accessible
> the process will start to abort execution.  Not sure about what kind of breakage
> it would incur, but it should reasonable to abort on both cases since this is
> done iff fortify is enabled.

I'm worried that this would result in spurious reports and may 
discourage usage of fortification, something that we do 
distribution-wide right now.

Sid

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Florian Weimer]

* Volker Weißmann:

> 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 frpm the file "/proc/self/maps". If opening this
> file fails due to too many open files (EMFILE), glibc
> will now ignore this error.
>
> Fixes [BZ #30932]
>
> Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de>
> ---
>  sysdeps/unix/sysv/linux/readonly-area.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/sysdeps/unix/sysv/linux/readonly-area.c b/sysdeps/unix/sysv/linux/readonly-area.c
> index edc68873f6..ba32372ebb 100644
> --- a/sysdeps/unix/sysv/linux/readonly-area.c
> +++ b/sysdeps/unix/sysv/linux/readonly-area.c
> @@ -42,7 +42,9 @@ __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
> +	  /* Process has reached the maximum number of open files.  */
> +	  || errno == EMFILE)
>  	return 1;
>        return -1;
>      }

This whole thing is rather questionable.

First of all, the compiler should detect the fact that a format argument
to printf is a string literal and record that in the flags argument
(which already exists for __printf_chk).  Then we wouldn't have to do
any %n security checks for most uses of %n.  (The flags argument cannot
be spoofed just by altering the string.)

Siddhesh, is that something you could be working on?

Even without that, if we are willing to trust the ld.so data structures,
we can do the permission check in userspace for strings that come from
.rodata.  We an find the ELF object that contains them and check if the
loadable segment has the right permissions (or we are in the RELRO
area).

After these changes, I think we can fail hard on /proc-related errors
because they are very unlikely to happen.

Thanks,
Florian

Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Siddhesh Poyarekar]

On 2023-10-04 13:36, Florian Weimer wrote:
> This whole thing is rather questionable.
> 
> First of all, the compiler should detect the fact that a format argument
> to printf is a string literal and record that in the flags argument
> (which already exists for __printf_chk).  Then we wouldn't have to do
> any %n security checks for most uses of %n.  (The flags argument cannot
> be spoofed just by altering the string.)
> 
> Siddhesh, is that something you could be working on?

Hmm, I thought the compiler already did this.  I can take a look.

> Even without that, if we are willing to trust the ld.so data structures,
> we can do the permission check in userspace for strings that come from
> .rodata.  We an find the ELF object that contains them and check if the
> loadable segment has the right permissions (or we are in the RELRO
> area).
> 
> After these changes, I think we can fail hard on /proc-related errors
> because they are very unlikely to happen.

We'd have to figure out a way for static binaries too.

Sid