Re: [PATCH v3] Fix FORTIFY_SOURCE false positive

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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