Re: [Libcdio-devel] Vulnerable use of strcpy in iso9660_fs.c

[Rocky Bernstein]

First of all Thomas, your suggestions are *greatly appreciated! *

Right now I am getting ready for eclipse-watching in the US and am out of
town and/or vacationing. But when I get back I'll soon travel to Singapore
to talk at BlackHat Asia 2014 and will spend a couple of weeks in Malaysia
after that.

I haven't forgotten about the batch of changes that are already backlogged.
When I get back late May I plan on starting in earnest to get everything
back into master.

However, if either Pete or Thomas want to address the problems you can do
so in a branch or, as far as I am concerned, do it in the master branch as
well. You guys rock!

And thanks for your work on this. Basically it is *you *who are keeping
this project alive.

In thanks
Rocky

On Sat, Apr 6, 2024 at 11:38 AM Thomas Schmitt via Libcdio-devel <
libcdio-devel@gnu.org> wrote:

> Hi,
>
> Pete Batard wrote:
> >   strncpy(cpy_result, p_psz_out, i_inlen);
>
> Known as nitpicker i want to to point out that this would avoid a memory
> corruption in case of overflow but would also truncate the name,
> potentially to an incomplete UTF-8 byte sequence at the end.
>
> I add the technical part of my private mail to Rocky of yesterday with
> assessment and proposal which is in part combinable with Pete's.
>
> ------------------------------------------------------------------
> My assessment for now:
>
> The  strcpy() gesture in function _iso9660_recname_to_cstring() is part
> of the conversion from UCS-2 to UTF-8.
> I don't see the need for strncpy(), because the result of
> cdio_charset_to_utf8() seems to be 0-terminated. (At least it does not
> reply a length which would be needed if no terminating 0 is appended.)
>
> But the memory safety depends on the caller having allocated cpy_result
> with a sufficient size.
> And there i see a potential problem with some languages.
>
> The allocation of cpy_result happens in _iso9660_dir_to_statbuf(),
> line 858 ff.:
>
>   if (!dir_len) return NULL;
>
>   i_fname = from_711(p_iso9660_dir->filename.len);
>
>   /* .. string in statbuf is one longer than in p_iso9660_dir's listing
> '\1' */
>   stat_len = sizeof(iso9660_stat_t) + i_fname + 2;
>
>   /* Reuse multiextent p_stat if not NULL */
>   if (!p_stat) {
>     p_stat = calloc(1, stat_len);
>     first_extent = true;
>
> The size of p_stat->filename is then (i_fname + 2), i.e.  what is needed
> for the UCS-2 representation of the name plus trailing 16-bit 0.
> p_stat->filename becomes parameter cpy_result:
>
> Line 956:
>         if (!_iso9660_recname_to_cstring(&p_iso9660_dir->filename.str[1],
>                                          i_inlen, NULL, p_stat->filename,
>                                          u_joliet_level))
> Line 965:
>     if (!_iso9660_recname_to_cstring(&p_iso9660_dir->filename.str[1],
>                                      i_inlen, NULL, p_stat->filename,
>                                      u_joliet_level))
>
> Normally text shrinks during the conversion from UCS-2 to UTF-8. But
> there are UCS-2/UTF-16 characters which need more than 2 bytes. E.g.:
>   https://www.compart.com/en/unicode/U+0800
>   Samaritan Letter Alaf
>   UTF-8 Encoding:       0xE0 0xA0 0x80
>   UTF-16 Encoding:      0x0800
>
> So if the UCS-2 name contains more than one of these UTF-8 3-byters the
> surplus space for the 16-bit 0 can be used up and the cpy_result can
> overflow, regardless of strcpy() versus strncpy().
> (Each 1-byte UTF-8 character in the conversion result adds another byte
>  of room for 3-byters. But i doubt that a string with samaritan letters
>  will contain many letters from 7-bit US-ASCII.)
>
> It looks like UTF-8 4-byters are not possible in UCS-2.
> If we encounter UTF-16 instead, the Joliet characters which yield 4-byte
> UTF-8 bytes are 4-byters themselves. (Conversion might yield poor results
> if really UCS-2 is expected. But memory should be safe.)
>
> --------------------------------------------------------------------------
> Proposal in addition to Pete's:
>
> In case of HAVE_JOLIET and u_joliet_level allocate in
>    _iso9660_dir_to_statbuf()
> 3/2 of the UCS-2 name length (i_fname of a good UCS-2 string is divisibe
> by 2):
>
>   stat_len = sizeof(iso9660_stat_t) + 3 * i_fname / 2 + 2;
>
> Rather an alternative to Pete's proposal:
>
> Further i propose to add a parameter
>   size_t cpy_result_size
> to
>   _iso9660_recname_to_cstring()
> and to feed it with (3 * i_fname / 2 + 2)  from _iso9660_dir_to_statbuf().
> In case of overflow, a message about programming error should be emitted.
>
>
> Have a nice day :)
>
> Thomas
>
>
>