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
-----Original Message-----
From: Santiago Vila <sanv...@debian.org>
Sent: 26 April 2025 15:53
To: Paul Marquess <paul.marqu...@outlook.com>
Subject: Fwd: Bug#1077054: Charset conversion fails when zip is built with
_FORTIFY_SOURCE
Hi. I was afraid that the applied fix would not be enough, and apparently it's
not.
I'm not sure how to proceed here. I could copy and paste his reply to
sourceforge, but I believe it would be more efficient if you could reply here
directly.
If it's ok for you, this would be enough in this case:
To: Adam Sampson <a...@offog.org>
Cc: 1077...@bugs.debian.org
because I receive copies of 1077...@bugs.debian.org automatically.
Thanks.
-------- Mensaje reenviado --------
Asunto: Re: Bug#1077054: Charset conversion fails when zip is built with
_FORTIFY_SOURCE
Fecha: Sat, 26 Apr 2025 14:08:57 +0100
De: Adam Sampson <a...@offog.org>
Para: Santiago Vila <sanv...@debian.org>
CC: 1077...@bugs.debian.org
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 <a...@offog.org> <http://offog.org/>
