[Bug string/29030] New: GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[Bug string/29030] New: GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[joanbrugueram at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

            Bug ID: 29030
           Summary: GLIBC 2.35 regression - Fortify crash on certain valid
                    uses of mbsrtowcs (*** buffer overflow detected ***:
                    terminated)
           Product: glibc
           Version: 2.35
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: string
          Assignee: unassigned at sourceware dot org
          Reporter: joanbrugueram at gmail dot com
  Target Milestone: ---

Since the recent update to GLIBC 2.35, I've seen or got reports on Arch Linux,
OpenSuse Tumbleweed and Fedora 36 (RPMFusion), of a crash on startup of the
popular "moc" (Music on Console) player with the following error message:

    "*** buffer overflow detected ***: terminated"
    Aborted (core dumped)

See e.g. Arch Linux bug report https://bugs.archlinux.org/task/74041

I've been able to reduce the crash down to the following test case:

    #include <wchar.h>

    int main (void)
    {
        const char *hw = "HelloWorld";
        mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
        return 0;
    }

As far as I can tell, this is a valid use of `mbsrtowcs`, since `$2=src` is a
valid pointer, `$1=dst` and `$4=ps` are allowed to be NULL, and `$3=len` is
ignored when `dst==NULL`.

When built with `gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test`,
however, it crashes with the error message above, plus the following gcc
warning:

    In file included from /usr/include/features.h:490,
                     from /usr/include/bits/libc-header-start.h:33,
                     from /usr/include/wchar.h:27,
                     from test.c:1:
    In function 'mbsrtowcs',
        inlined from 'main' at test.c:6:9:
    /usr/include/bits/wchar2.h:428:10: warning: call to '__mbsrtowcs_chk_warn'
declared with attribute warning: mbsrtowcs called with dst buffer smaller than
len * sizeof (wchar_t) [-Wattribute-warning]
      428 |   return __glibc_fortify_n (mbsrtowcs, __len, sizeof (wchar_t),
          |          ^~~~~~~~~~~~~~~~~

The crash only reproduces with "large enough" values of `$3=len` - note `len`
is supposed to be ignored when `dst=NULL` according to the manual. Namely on my
x86_64 system it crashes when `len > 0x3FFFFFFFFFFFFFFFULL`.

I've been reviewing recent changes and I believe this regression was introduced
by commit a643f60c53876b, which refactors some fortify-related code. I believe
the false positive may only trigger on rare edge cases - it needs to be a
function where `__glibc_fortify(_n)?` is called with `__s > 1`, and a call with
a large buffer size parameter is valid - which as far as I can tell only
includes `mbsrtowcs` and similar related functions in wchar2.h. This would
explain why this wasn't detected by tests or earlier.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[joanbrugueram at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

Joan Bruguera Micó <joanbrugueram at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |glibc_2.35
                 CC|                            |joanbrugueram at gmail dot com,
                   |                            |siddhesh at sourceware dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[joanbrugueram at gmail dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

--- Comment #1 from Joan Bruguera Micó <joanbrugueram at gmail dot com> ---
Created attachment 14050
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14050&action=edit
WIP Patch

PS: I have the following WIP patch, I need to finish testing & checking the
rules before sending to glibc-alpha.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

--- Comment #2 from Siddhesh Poyarekar <siddhesh at sourceware dot org> ---
(In reply to Joan Bruguera Micó from comment #1)
> Created attachment 14050 [details]
> WIP Patch
> 
> PS: I have the following WIP patch, I need to finish testing & checking the
> rules before sending to glibc-alpha.

Thanks, but that's not the correct fix I'm afraid.  The fix is to skip _chk in
case dest is NULL in wcsmbs/bits/wchar2.h.  That is, this function:

__fortify_function size_t                                                  
__NTH (mbsrtowcs (wchar_t *__restrict __dst, const char **__restrict __src,
                  size_t __len, mbstate_t *__restrict __ps))               
{                                                                          
  return __glibc_fortify_n (mbsrtowcs, __len, sizeof (wchar_t),            
                            __glibc_objsize (__dst),                       
                            __dst, __src, __len, __ps);                    
}                                                                          

should avoid calling __glibc_fortify_n when __dst is NULL.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

Siddhesh Poyarekar <siddhesh at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-04-06
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #3 from Siddhesh Poyarekar <siddhesh at sourceware dot org> ---
Also, please consider fixing mbsnrtowcs along with this since that will also
have te same issue.  Thanks!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

--- Comment #4 from Siddhesh Poyarekar <siddhesh at sourceware dot org> ---
Thinking about this a bit more, lets go with a variant of your patch, i.e. add
the -1 check at the top in __glibc_safe_or_unknown_len.  That should
preventively fix any future API (or existing API we may have missed) that has
similar semantics to mbsrtowcs.

Please post a patch and cc me, and I'll review it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

--- Comment #5 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Siddhesh Poyarekar
<siddhesh@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=33e03f9cd2be4f2cd62f93fda539cc07d9c8130e

commit 33e03f9cd2be4f2cd62f93fda539cc07d9c8130e
Author: Joan Bruguera <joanbrugueram@gmail.com>
Date:   Mon Apr 11 19:49:56 2022 +0200

    misc: Fix rare fortify crash on wchar funcs. [BZ 29030]

    If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size),
fortify
    checks should pass, and `__whatever_alias` should be called.

    Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked,
but
    on commit a643f60c53876b, this was moved into
`__glibc_safe_or_unknown_len`.

    A comment says the -1 case should work as: "The -1 check is redundant
because
    since it implies that __glibc_safe_len_cond is true.". But this fails when:
    * `__s > 1`
    * `__osz == -1` (i.e. unknown size at compile time)
    * `__l` is big enough
    * `__l * __s <= __osz` can be folded to a constant
    (I only found this to be true for `mbsrtowcs` and other functions in
wchar2.h)

    In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will
be
    called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program.

    This commit adds the explicit `__osz == -1` check again.
    moc crashes on startup due to this, see:
https://bugs.archlinux.org/task/74041

    Minimal test case (test.c):
        #include <wchar.h>

        int main (void)
        {
            const char *hw = "HelloWorld";
            mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
            return 0;
        }

    Build with:
        gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test

    Output:
        *** buffer overflow detected ***: terminated

    Fixes: BZ #29030
    Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
    Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[siddhesh at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

Siddhesh Poyarekar <siddhesh at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED
   Target Milestone|---                         |2.36

--- Comment #6 from Siddhesh Poyarekar <siddhesh at sourceware dot org> ---
I need to backport this and the changes were minor, so I pushed your fix with
those changes.  Thank you for your contribution!

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

--- Comment #7 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The release/2.35/master branch has been updated by Siddhesh Poyarekar
<siddhesh@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=c8ee1c85c07b3c9eaef46355cb1095300855e8fa

commit c8ee1c85c07b3c9eaef46355cb1095300855e8fa
Author: Joan Bruguera <joanbrugueram@gmail.com>
Date:   Mon Apr 11 19:49:56 2022 +0200

    misc: Fix rare fortify crash on wchar funcs. [BZ 29030]

    If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size),
fortify
    checks should pass, and `__whatever_alias` should be called.

    Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked,
but
    on commit a643f60c53876b, this was moved into
`__glibc_safe_or_unknown_len`.

    A comment says the -1 case should work as: "The -1 check is redundant
because
    since it implies that __glibc_safe_len_cond is true.". But this fails when:
    * `__s > 1`
    * `__osz == -1` (i.e. unknown size at compile time)
    * `__l` is big enough
    * `__l * __s <= __osz` can be folded to a constant
    (I only found this to be true for `mbsrtowcs` and other functions in
wchar2.h)

    In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will
be
    called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program.

    This commit adds the explicit `__osz == -1` check again.
    moc crashes on startup due to this, see:
https://bugs.archlinux.org/task/74041

    Minimal test case (test.c):
        #include <wchar.h>

        int main (void)
        {
            const char *hw = "HelloWorld";
            mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
            return 0;
        }

    Build with:
        gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test

    Output:
        *** buffer overflow detected ***: terminated

    Fixes: BZ #29030
    Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
    Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
    (cherry picked from commit 33e03f9cd2be4f2cd62f93fda539cc07d9c8130e)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/29030] GLIBC 2.35 regression - Fortify crash on certain valid uses of mbsrtowcs (*** buffer overflow detected ***: terminated)

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=29030

--- Comment #8 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The release/2.34/master branch has been updated by Siddhesh Poyarekar
<siddhesh@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=ca0faa140ff8cebe4c041d935f0f5eb480873d99

commit ca0faa140ff8cebe4c041d935f0f5eb480873d99
Author: Joan Bruguera <joanbrugueram@gmail.com>
Date:   Mon Apr 11 19:49:56 2022 +0200

    misc: Fix rare fortify crash on wchar funcs. [BZ 29030]

    If `__glibc_objsize (__o) == (size_t) -1` (i.e. `__o` is unknown size),
fortify
    checks should pass, and `__whatever_alias` should be called.

    Previously, `__glibc_objsize (__o) == (size_t) -1` was explicitly checked,
but
    on commit a643f60c53876b, this was moved into
`__glibc_safe_or_unknown_len`.

    A comment says the -1 case should work as: "The -1 check is redundant
because
    since it implies that __glibc_safe_len_cond is true.". But this fails when:
    * `__s > 1`
    * `__osz == -1` (i.e. unknown size at compile time)
    * `__l` is big enough
    * `__l * __s <= __osz` can be folded to a constant
    (I only found this to be true for `mbsrtowcs` and other functions in
wchar2.h)

    In this case `__l * __s <= __osz` is false, and `__whatever_chk_warn` will
be
    called by `__glibc_fortify` or `__glibc_fortify_n` and crash the program.

    This commit adds the explicit `__osz == -1` check again.
    moc crashes on startup due to this, see:
https://bugs.archlinux.org/task/74041

    Minimal test case (test.c):
        #include <wchar.h>

        int main (void)
        {
            const char *hw = "HelloWorld";
            mbsrtowcs (NULL, &hw, (size_t)-1, NULL);
            return 0;
        }

    Build with:
        gcc -O2 -Wp,-D_FORTIFY_SOURCE=2 test.c -o test && ./test

    Output:
        *** buffer overflow detected ***: terminated

    Fixes: BZ #29030
    Signed-off-by: Joan Bruguera <joanbrugueram@gmail.com>
    Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
    (cherry picked from commit 33e03f9cd2be4f2cd62f93fda539cc07d9c8130e)

-- 
You are receiving this mail because:
You are on the CC list for the bug.