Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

["Volker Weißmann" <volker.weissmann@gmx.de>]

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