1. Forum
  2. Usenet
  3. LINUX.DEBIAN.BUGS.DIST

  • Bug#1077054: Charset conversion fails when zip is built with _FORTIFY_S

    From Santiago Vila@21:1/5 to All on Fri Apr 25 20:30:01 2025
    El 25/7/24 a las 17:12, Adam Sampson escribió:
    If zip 3.0-14 is built with _FORTIFY_SOURCE=3 (GCC 14.1, glibc 2.40),
    this can happen when compressing a file with non-ASCII characters in its UTF-8 name:

    $ echo -n "There’s a Baby in the House.flac" | od -c
    0000000 T h e r e 342 200 231 s a B a b y 0000020 i n t h e H o u s e . f l 0000040 a c
    $ zip /tmp/t.zip "There’s a Baby in the House.flac"
    *** buffer overflow detected ***: terminated

    The problem is in local_to_wide_string, where mbstowcs is being run with
    the UTF-8 source length rather than the widechar destination length --
    this correctly trips a fortify error because GCC 14 can infer the actual
    size of the destination.

    Hello. A very similar problem was reported here:

    https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1093629

    In theory, this was already fixed by Fedora here:

    https://src.fedoraproject.org/rpms/zip/raw/f41/f/buffer_overflow.patch

    in a slightly different way:

    --- a/fileio.c
    +++ b/fileio.c
    @@ -3502,7 +3502,7 @@
    if ((wc_string = (wchar_t *)malloc((wsize + 1) * sizeof(wchar_t))) == NULL) {
    ZIPERR(ZE_MEM, "local_to_wide_string");
    }
    - wsize = mbstowcs(wc_string, local_string, strlen(local_string) + 1);
    + wsize = mbstowcs(wc_string, local_string, wsize + 1);
    wc_string[wsize] = (wchar_t) 0;

    /* in case wchar_t is not zwchar */


    I plan to do that in zip 3.0-15.

    If that's not enough, please specify the exact way in which I
    can reproduce the error using Debian unstable.

    I tried using this line in debian/rules:

    CPPFLAGS := -Wdate-time -D_FORTIFY_SOURCE=3

    but did not notice any change in behavior.

    Thanks.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Adam Sampson@21:1/5 to Santiago Vila on Sat Apr 26 15:50:01 2025
    Hi Santiago,

    On Fri, Apr 25, 2025 at 08:25:38PM +0200, Santiago Vila wrote:
    In theory, this was already fixed by Fedora here:

    @@ -3502,7 +3502,7 @@
    if ((wc_string = (wchar_t *)malloc((wsize + 1) * sizeof(wchar_t))) == NULL) {
    ZIPERR(ZE_MEM, "local_to_wide_string");
    }
    - wsize = mbstowcs(wc_string, local_string, strlen(local_string) + 1);
    + wsize = mbstowcs(wc_string, local_string, wsize + 1);
    wc_string[wsize] = (wchar_t) 0;
    /* in case wchar_t is not zwchar */

    I don't think that's a correct fix. local_string here is the argument to
    the function, so it can be of any length (e.g. an input filename).

    mbstowcs returns the number of characters written to the buffer (see mbcstowcs(3)). So if the converted version of local_string is too big to
    fit in the buffer, it will return wsize + 1, and the assignment on the
    next line will write outside the bounds of the buffer -- which is
    potentially a security problem if this is being used with untrusted
    input.

    My patch added an error check here. You could also expand the buffer by
    one more place so that there's always space to add a 0, but then it will silently truncate the string which may cause other problems.

    Thanks,

    --
    Adam Sampson <ats@offog.org> <http://offog.org/>

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Santiago Vila@21:1/5 to All on Sat Apr 26 17:30:01 2025
    reopen 1077054
    thanks

    Thanks for your reply. I'll ask the authors.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Santiago Vila@21:1/5 to All on Sat Apr 26 18:50:01 2025
    Ok, let's see if we can be all in the loop at the same time :-)

    Everybody please always include Paul (author), Adam (bug submitter),
    the bug address (for archival purposes) and myself (who prefer
    to receive duplicates than risking not to receive a copy at all)
    in the To or Cc fields.

    This is the reply from Paul Marquess and it's addressed at Adam:

    Looking at the source I see that the code does an initial dummy conversion to get wsize

    /* for now try to convert as string - fails if a bad char in string */.
    wsize = mbstowcs(NULL, local_string, MB_CUR_MAX );
    if (wsize == (size_t)-1) {
    /* could not convert */
    return NULL;
    }

    That wsize is then used to malloc the wc_string buffer


    /* convert it */
    if ((wc_string = (wchar_t *)malloc((wsize + 1) * sizeof(wchar_t))) == NULL) {
    ZIPERR(ZE_MEM, "local_to_wide_string");
    }
    wsize = mbstowcs(wc_string, local_string, strlen(local_string) + 1);


    Is there a path through that that means mbstowcs will not create a wsize output buffer?

    Paul

    Thanks.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Adam Sampson@21:1/5 to Santiago Vila on Sat Apr 26 21:40:01 2025
    On Sat, Apr 26, 2025 at 06:40:11PM +0200, Santiago Vila wrote:
    Looking at the source I see that the code does an initial dummy
    conversion to get wsize
    [...]
    Is there a path through that that means mbstowcs will not create a
    wsize output buffer?

    Ah, no, that's OK then -- apologies for the noise.

    Thanks,

    --
    Adam Sampson <ats@offog.org> <http://offog.org/>

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)
  • From Santiago Vila@21:1/5 to All on Sat Apr 26 23:20:01 2025
    close 1077054 3.0-15
    thanks

    Thank you both for the double check.

    Thanks.

    --- SoupGate-Win32 v1.05
    * Origin: fsxNet Usenet Gateway (21:1/5)