From owner-freebsd-security@FreeBSD.ORG Thu Jul 9 03:34:34 2009 Return-Path: Delivered-To: freebsd-security@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C5BAC1065691 for ; Thu, 9 Jul 2009 03:34:34 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) Received: from 0.mx.codelabs.ru (0.mx.codelabs.ru [144.206.177.45]) by mx1.freebsd.org (Postfix) with ESMTP id 6F90D8FC21 for ; Thu, 9 Jul 2009 03:34:33 +0000 (UTC) (envelope-from rea-fbsd@codelabs.ru) DomainKey-Signature: a=rsa-sha1; q=dns; c=simple; s=one; d=codelabs.ru; h=Received:Date:From:To:Cc:Subject:Message-ID:Reply-To:References:MIME-Version:Content-Type:Content-Disposition:In-Reply-To:Sender; b=mrEOUVKXZbdyz/HHOjOK0fXpokPXEqMjZ50sDBmfufKaTuyVcrnzxdy2cXTmlQVuu57RqnbNGVHFSGopiGIgzxeK+VPNFcmJU6O/MC9jbSyPr6pHZqtzO8J1hh4s73obJW8I90bN1DcicYNKQr9xt1H2qEXPrTAeTmtSHvK3k48=; Received: from phoenix.codelabs.ru (ppp91-77-10-253.pppoe.mtu-net.ru [91.77.10.253]) by 0.mx.codelabs.ru with esmtpsa (TLSv1:AES256-SHA:256) id 1MOk8p-0001oL-MF; Thu, 09 Jul 2009 07:17:47 +0400 Date: Thu, 9 Jul 2009 07:17:46 +0400 From: Eygene Ryabinkin To: d@delphij.net Message-ID: References: <20090708193339.GA4836@minerva.freedsl.mg> <4A553080.5060205@delphij.net> <4A553458.70005@delphij.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4A553458.70005@delphij.net> Sender: rea-fbsd@codelabs.ru Cc: rrl , freebsd-security@freebsd.org Subject: Re: gzip memory corruption X-BeenThere: freebsd-security@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: rea-fbsd@codelabs.ru List-Id: "Security issues \[members-only posting\]" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Jul 2009 03:34:35 -0000 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 {_.-``-' {_/ #