[Bug sanitizer/94849] New: Improper parameter validation in libsanitizer for fopen64

[Bug sanitizer/94849] New: Improper parameter validation in libsanitizer for fopen64

[rachel at rachelmant dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

            Bug ID: 94849
           Summary: Improper parameter validation in libsanitizer for
                    fopen64
           Product: gcc
           Version: 9.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rachel at rachelmant dot com
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

Created attachment 48408
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48408&action=edit
A simple fix for the bug

fopen() (actually fopen64 because of macro remapping) as implemented by Glibc
and other C run-times allows the first "path" or "filename" parameter to be
nullptr and causes the function to simply do nothing and return nullptr itself
when this happens.

Because of this, I have a test case in a project that is specifically verifying
this bad, but allowed, use of fopen64() as a way to guarantee it returns
nullptr for some further tests and for code coverage reasons.

The simplest form of failing program is:

```
#include <cstdio>

int main()
{
    return !fopen(nullptr, "w") ? 0 : 1;
}
```

which when compiled with `g++ -D_FILE_OFFSET_BITS=64 -o test test.cxx` runs and
returns 0, but when compiled with `g++ -D_FILE_OFFSET_BITS=64 -o test
-fsanitize=address test.cxx` or the thread sanitizer, instead crashes:

```
$ ./test; echo $?
AddressSanitizer:DEADLYSIGNAL
=================================================================
==2187382==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc
0x7f8867d791e5 bp 0x7ffcc8f95db0 sp 0x7ffcc8f95528 T0)
==2187382==The signal is caused by a READ memory access.
==2187382==Hint: address points to the zero page.
    #0 0x7f8867d791e4 in __strlen_avx2 (/usr/lib/libc.so.6+0x1611e4)
    #1 0x7f886817cc35 in __interceptor_fopen64
/build/gcc/src/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:5757
    #2 0x56367982918d in main (/tmp/test+0x118d)
    #3 0x7f8867c3f022 in __libc_start_main (/usr/lib/libc.so.6+0x27022)
    #4 0x5636798290ad in _start (/tmp/test+0x10ad)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/usr/lib/libc.so.6+0x1611e4) in __strlen_avx2
==2187382==ABORTING
1

```

This is on GCC 9.3.0, but I have reproduced this on everything from GCC 5
through to 9. Please note: This is not a problem when the non-64-bit fopen() is
used. This is specifically a problem with fopen64().

Unfortunately, libsanitizer does not properly validate the path parameter and
ends up calling strlen() on a nullptr - which is UB that on x86 crashes with
SEGV as seen above.

After triggering the bug and doing some research, it appears that since the
introduction of libsanitizer, this has been a thing:
https://github.com/gcc-mirror/gcc/blob/master/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc#L5882

Attached is a patch that would fix this bugged behaviour.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |marxin at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-04-29

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Rachel Mant from comment #0)
> Created attachment 48408 [details]
> A simple fix for the bug
> 
> fopen() (actually fopen64 because of macro remapping) as implemented by
> Glibc and other C run-times allows the first "path" or "filename" parameter
> to be nullptr and causes the function to simply do nothing and return
> nullptr itself when this happens.
> 

Thank you for the report. Can you please find where is the behavior documented?
I can't find anything about NULL pointer argument at:
man fopen.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think it is undefined behavior and just doesn't crash because the pathname is
passed to a syscall which will fail then.
So IMHO nothing we should support.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #2)
> I think it is undefined behavior and just doesn't crash because the pathname
> is passed to a syscall which will fail then.
> So IMHO nothing we should support.

Agree with that. One possible improvement can be decoration of the filename
with non-null argument (in Glibc).

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[rachel at rachelmant dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

--- Comment #4 from Rachel Mant <rachel at rachelmant dot com> ---
Glibc, MSVCRT and other CRTs all check for this condition in userspace and NOP
it by short-circuiting the call with a return of nullptr. MSVCRT even documents
this
(https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=vs-2019)

I agree that the fopen() man page leaves it as unspecified behaviour however.
That said.. this is not actually fopen() itself, but its 64-bit cousin
fopen64(). Again, Glibc and other CRTs agree to handle a nullptr file argument
by NOP'ing the call and returning nullptr.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So it might be well defined on Windows, but unless glibc documents it as an
extension, it is not valid on Linux.
C clearly says: "The fopen function opens the file whose name is the string
pointed to by filename" and NULL is not a pointer to a string.  Same POSIX,
e.g. in 1003.1-2017.  So, just don't do this, you invoke undefined behavior,
and one such behavior is that it crashes with the sanitizers.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[rachel at rachelmant dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

--- Comment #6 from Rachel Mant <rachel at rachelmant dot com> ---
Ok, fair enough - though I'd like to know your thoughts then on the rest of the
f*open() family and the fact the sanitizers do check for nullptr
paths/filenames even though the wording is the same. The fopen64() sanitizer
interceptor is the only one of the lot that does not.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[rachel at rachelmant dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

--- Comment #7 from Rachel Mant <rachel at rachelmant dot com> ---
Continuing to think on this a bit, and.. if it is undefined behaviour as you
say, then granted this is not a bug on ASAN/TSAN.. but it is still a bug as
UBSAN does and says nothing when faced with this even though it clearly should
terminate the program for invoking UB, with a suitable diagnostic.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #8 from Martin Sebor <msebor at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #3)
> One possible improvement can be decoration of the filename
> with non-null argument (in Glibc).

Agreed (unless Glibc wants to support nulls here, in which case it should
document it as an extension).  Another improvement is to declare fopen and
other functions that expect string arguments with attribute access read_only
(so for fopen both the file and mode arguments).  That would let GCC diagnose
calls that pass in valid pointers that don't point to strings (e.g., just
past-the-end pointers).  This would be a useful follow-up on
https://sourceware.org/bugzilla/show_bug.cgi?id=25219.

[Bug sanitizer/94849] Improper parameter validation in libsanitizer for fopen64

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94849

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |MOVED
             Status|ASSIGNED                    |RESOLVED

--- Comment #9 from Martin Liška <marxin at gcc dot gnu.org> ---
Let's close it as moved.