Xin, good day.
Wed, Jul 08, 2009 at 05:05:44PM -0700, Xin LI wrote:
> >>> The offending code lays in the function file_compress:
> >>>> /* Add (usually) .gz to filename */
> >>>> if ((size_t)snprintf(outfile, outsize, "%s%s",
> >>>> file, suffixes[0].zipped) >= outsize)
> >>>> memcpy(outfile - suffixes[0].ziplen - 1,
> >>>> suffixes[0].zipped, suffixes[0].ziplen + 1);
> >> The memcpy() call looks like a complete madness: it will write before
> >> the beginning of the 'outfile', so it will be buffer underflow in any
> >> case (unless I am terribly mistaken and missing some obvious point).
> >
> >> I'd change the above code to warn and return if snprintf will discard
> >> some trailing characters, the patch is attached.
>
> I have attached another possible fix, which catches the problem when
> parsing the command line. The point is that, I think we really want to
> catch bad input as early as possible.
Yes, it is good to catch it here too.
> Index: gzip.c
> ===================================================================
> --- gzip.c (?????? 195435)
> +++ gzip.c (????????????)
> @@ -372,6 +372,8 @@
> case 'S':
> len = strlen(optarg);
> if (len != 0) {
> + if (len >= PATH_MAX)
> + errx(1, "incorrect suffix: '%s'",
> optarg);
> suffixes[0].zipped = optarg;
> suffixes[0].ziplen = len;
> } else {
But the place with the memcpy() should be patched too. Two reasons:
- suffix could not (yet) overflow PATH_MAX, but filename + suffix --
can;
- I am really worried about the usage of memcpy with underflow;
I had tried to study the reasons for it via NetBSD CVS, but
it just appeared one day and the reason for going to
'outfile - suffixes[0].ziplen - 1' (with .gz its outfile - 4)
are unknown; I am still taking this as the programming error.
So, unless you know why we're underflowing the passed pointer,
the memcpy block should be patched too, for future safety and
code correctness.
--
Eygene
_ ___ _.--. #
\`.|\..----...-'` `-._.-'_.-'` # Remember that it is hard
/ ' ` , __.--' # to read the on-line manual
)/' _/ \ `-_, / # while single-stepping the kernel.
`-'" `"\_ ,_.-;_.-\_ ', fsc/as #
_.-'_./ {_.' ; / # -- FreeBSD Developers handbook
{_.-``-' {_/ #
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-security
To unsubscribe, send any mail to "[email protected]"
