[v3] 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 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(-)

--
2.42.0
  

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
>
  

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

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?
  

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

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

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
  

* 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
  

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