Fix FORTIFY_SOURCE false positive

  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.
---
 sysdeps/unix/sysv/linux/readonly-area.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

--
2.42.0
  

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 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.

> +          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?

Thanks,
Sid
  

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):

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>
  

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>
  

On 02.10.23 20:00, Siddhesh Poyarekar wrote:
>
> 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?

This is my first patch I sent to a mailing list, so forgive me if the
formatting is wrong:


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.

Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de>
---
  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
+          into this if clause here. */
+          || errno == EMFILE)
      return 1;
        return -1;
      }
--
2.42.0
>
> Thanks,
> Sid
  

On 2023-10-03 09:22, Volker Weißmann wrote:
> 
> On 02.10.23 20:00, Siddhesh Poyarekar wrote:
>>
>> 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?
> 
> This is my first patch I sent to a mailing list, so forgive me if the
> formatting is wrong:

No worries, please review the Contribution checklist[1], which goes into 
much detail about submitting a patch to glibc.

One issue is, if you reply to a patch with a patch like you've done now, 
patchwork[2] does not pick it up; it is what we use to manage patches. 
Please fix the below review comments and send a v3.  Basically in your 
git repo, commit your patch with the subject and body of this email as 
the git commit log (with the Signed-off-by) and then generate a patch 
like so:

git format-patch --subject-prefix="PATCH v3" -1

and then send that patch to the list using "git send-email".

> 
> 
> 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.
> 
> Signed-off-by: Volker Weißmann <volker.weissmann@gmx.de>
> ---
>   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
> +          into this if clause here. */

Please replace this code in the comment with a descriptive line as 
Adhemerval suggested, something like "Process has reached the maximum 
number of open files."

> +          || errno == EMFILE)
>       return 1;
>         return -1;
>       }
> -- 
> 2.42.0
>>
>> Thanks,
>> Sid
> 


Thanks,
Sid

[1] https://sourceware.org/glibc/wiki/Contribution%20checklist
[2] https://patchwork.sourceware.org/project/glibc/list/