From owner-freebsd-security@FreeBSD.ORG Thu Jul 30 23:51:28 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 5A6F11065673 for ; Thu, 30 Jul 2009 23:51:28 +0000 (UTC) (envelope-from delphij@delphij.net) Received: from tarsier.delphij.net (delphij-pt.tunnel.tserv2.fmt.ipv6.he.net [IPv6:2001:470:1f03:2c9::2]) by mx1.freebsd.org (Postfix) with ESMTP id C43588FC1E for ; Thu, 30 Jul 2009 23:51:27 +0000 (UTC) (envelope-from delphij@delphij.net) Received: from tarsier.geekcn.org (tarsier.geekcn.org [211.166.10.233]) (using TLSv1 with cipher ADH-CAMELLIA256-SHA (256/256 bits)) (No client certificate requested) by tarsier.delphij.net (Postfix) with ESMTPS id 434B55C027 for ; Fri, 31 Jul 2009 07:51:26 +0800 (CST) Received: from localhost (tarsier.geekcn.org [211.166.10.233]) by tarsier.geekcn.org (Postfix) with ESMTP id 12BC955CD9AC; Fri, 31 Jul 2009 07:51:26 +0800 (CST) X-Virus-Scanned: amavisd-new at geekcn.org Received: from tarsier.geekcn.org ([211.166.10.233]) by localhost (mail.geekcn.org [211.166.10.233]) (amavisd-new, port 10024) with ESMTP id iwW7MdkmzD5P; Fri, 31 Jul 2009 07:50:22 +0800 (CST) Received: from charlie.delphij.net (adsl-76-237-33-62.dsl.pltn13.sbcglobal.net [76.237.33.62]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by tarsier.geekcn.org (Postfix) with ESMTPSA id EAD3755CD9A9; Fri, 31 Jul 2009 07:50:10 +0800 (CST) DomainKey-Signature: a=rsa-sha1; s=default; d=delphij.net; c=nofws; q=dns; h=message-id:date:from:reply-to:organization:user-agent: mime-version:to:cc:subject:references:in-reply-to: x-enigmail-version:openpgp:content-type; b=TNv8amiwlytxSkXVJNqPxFSdwOfuJAGlFFUJIazqihjoXhSEWGwbU8QKMZzzfAFZY jEed5trWwWC/kYDJ0ZrDQ== Message-ID: <4A7231A1.2050104@delphij.net> Date: Thu, 30 Jul 2009 16:49:53 -0700 From: Xin LI Organization: The FreeBSD Project User-Agent: Thunderbird 2.0.0.22 (X11/20090701) MIME-Version: 1.0 To: rea-fbsd@codelabs.ru References: <20090708193339.GA4836@minerva.freedsl.mg> <4A553080.5060205@delphij.net> <4A553458.70005@delphij.net> In-Reply-To: X-Enigmail-Version: 0.95.7 OpenPGP: id=18EDEBA0; url=http://www.delphij.net/delphij.asc Content-Type: multipart/mixed; boundary="------------090909090002070501050501" Cc: rrl , freebsd-security@freebsd.org, d@delphij.net Subject: Re: gzip memory corruption X-BeenThere: freebsd-security@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: d@delphij.net List-Id: "Security issues \[members-only posting\]" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Jul 2009 23:51:28 -0000 This is a multi-part message in MIME format. --------------090909090002070501050501 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi, Eygene, Eygene Ryabinkin wrote: [...] > 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. Sorry for the late response. I am busy recently. After carefully investigating the code, I have the following observations: - The usage of memcpy() here is wrong. It's definitely a bug. - The intention of it seems to be to make a pathname that fits into MAXPATHLEN, say, if we have /very/long/name which makes /very/long/name.gz longer than MAXPATHLEN, it might truncate it into, i.e. /very/long/n.gz instead. I feel really uncomfortable for the latter case, we should stop for that case instead of proceeding further. Having checked with GNU's gzip, it looks like that they arbitrarily set an upper limit of the suffix length to 30. This is unrelated to the memcpy bug but let's address it here as well. My revised patch would make the memcpy into a fatal errx, and reduce the allowed suffix length to 30 to match GNU behavior. Please let me know if this version looks better, I'll propose it to re@ and commit if they approved it. Cheers, - -- Xin LI http://www.delphij.net/ FreeBSD - The Power to Serve! -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.12 (FreeBSD) iEYEARECAAYFAkpyMaAACgkQi+vbBBjt66DLngCgv+VoeLsZN1NM5qFHX5hc0lPM 5WgAnjTeMukfn8akGrDpz8ozDDG/7AdV =7ywC -----END PGP SIGNATURE----- --------------090909090002070501050501 Content-Type: text/plain; name="gzip.c-S-underflow.diff" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="gzip.c-S-underflow.diff" Index: gzip.c =================================================================== --- gzip.c (revision 195945) +++ gzip.c (working copy) @@ -150,6 +150,8 @@ }; #define NUM_SUFFIXES (sizeof suffixes / sizeof suffixes[0]) +#define SUFFIX_MAXLEN 30 + static const char gzip_version[] = "FreeBSD gzip 20090621"; #ifndef SMALL @@ -372,6 +374,8 @@ case 'S': len = strlen(optarg); if (len != 0) { + if (len >= SUFFIX_MAXLEN) + errx(1, "incorrect suffix: '%s'", optarg); suffixes[0].zipped = optarg; suffixes[0].ziplen = len; } else { @@ -1236,8 +1240,7 @@ /* 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); + errx(1, "Path name too long"); #ifndef SMALL if (check_outfile(outfile) == 0) { --------------090909090002070501050501--