From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?utf-8?B?T25kxZllaiBCw61sa2E=?= Subject: Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big Date: Sun, 20 Oct 2013 09:39:05 +0200 Message-ID: <20131020073905.GA10718@domone.podge> References: <1382179954-5169-1-git-send-email-apelisse@gmail.com> <52636E5A.1080909@web.de> <20131020060517.GA8436@domone.podge> <526377C1.8020907@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Antoine Pelisse , git@vger.kernel.org, Wataru Noguchi , Erik Faye-Lund , Johannes Schindelin , =?iso-8859-1?Q?Ren=E9?= Scharfe , msysGit To: Torsten =?iso-8859-1?Q?B=F6gershausen?= X-From: git-owner@vger.kernel.org Sun Oct 20 09:39:17 2013 Return-path: Envelope-to: gcvg-git-2@plane.gmane.org Received: from vger.kernel.org ([209.132.180.67]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1VXnbY-0004lr-VL for gcvg-git-2@plane.gmane.org; Sun, 20 Oct 2013 09:39:17 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751399Ab3JTHjM convert rfc822-to-quoted-printable (ORCPT ); Sun, 20 Oct 2013 03:39:12 -0400 Received: from popelka.ms.mff.cuni.cz ([195.113.20.131]:54052 "EHLO popelka.ms.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751113Ab3JTHjL (ORCPT ); Sun, 20 Oct 2013 03:39:11 -0400 Received: from domone.kolej.mff.cuni.cz (popelka.ms.mff.cuni.cz [195.113.20.131]) by popelka.ms.mff.cuni.cz (Postfix) with ESMTPS id 54A8C5413A; Sun, 20 Oct 2013 09:39:06 +0200 (CEST) Received: by domone.kolej.mff.cuni.cz (Postfix, from userid 1000) id 1FA6A5F96A; Sun, 20 Oct 2013 09:39:06 +0200 (CEST) Content-Disposition: inline In-Reply-To: <526377C1.8020907@web.de> User-Agent: Mutt/1.5.20 (2009-06-14) X-Virus-Scanned: clamav-milter 0.97.6 at popelka.ms.mff.cuni.cz X-Virus-Status: Clean X-Spam-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on popelka.ms.mff.cuni.cz Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org Archived-At: On Sun, Oct 20, 2013 at 08:27:13AM +0200, Torsten B=C3=B6gershausen wro= te: > Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on > popelka.ms.mff.cuni.cz > Status: O > Content-Length: 2690 > Lines: 89 >=20 > On 20.10.13 08:05, Ond=C5=99ej B=C3=ADlka wrote: > > On Sun, Oct 20, 2013 at 07:47:06AM +0200, Torsten B=C3=B6gershausen= wrote: > >> (may be s/path is too big/path is too long/ ?) > >> > >> On 19.10.13 12:52, Antoine Pelisse wrote: > >>> Currently, most buffers created with PATH_MAX length, are not che= cked > >>> when being written, and can overflow if PATH_MAX is not big enoug= h to > >>> hold the path. > >>> > >>> Fix that by using strlcpy() where strcpy() was used, and also run= some > >>> extra checks when copy is done with memcpy(). > >>> > >>> Reported-by: Wataru Noguchi > >>> Signed-off-by: Antoine Pelisse > >>> --- > >>> diff --git a/abspath.c b/abspath.c > >>> index 64adbe2..0e60ba4 100644 > >>> --- a/abspath.c > >>> +++ b/abspath.c > >>> @@ -216,11 +216,15 @@ const char *absolute_path(const char *path) > >>> const char *prefix_filename(const char *pfx, int pfx_len, const = char *arg) > >>> { > >>> static char path[PATH_MAX]; > >=20 > > Why do you need static there? > Good point. > get_pathname() from path.c may be better. >=20 > >>> + > >>> + if (pfx_len > PATH_MAX) > >> I think this should be=20 > >> if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0' > >>> + die("Too long prefix path: %s", pfx); > >>> + > >>> #ifndef GIT_WINDOWS_NATIVE > >>> if (!pfx_len || is_absolute_path(arg)) > >>> return arg; > >>> memcpy(path, pfx, pfx_len); > >>> - strcpy(path + pfx_len, arg); > >>> + strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len); > >> > >> I'm not sure how to handle overlong path in general, there are sev= eral ways: > >> a) Silently overwrite memory (with help of memcpy() and/or strcpy(= ) > >> b) Silently shorten the path using strlcpy() instead of strcpy() > >> c) Avoid the overwriting and call die(). > >> d) Prepare a longer buffer using xmalloc() > >> > > There is also > > e) modify allocation to place write protected page after buffer end= =2E >=20 > Yes, I think this is what electric fence, DUMA or valgrind do: >=20 You need to be selective which buffers are important. > http://sourceforge.jp/projects/freshmeat_efence/ > http://duma.sourceforge.net/ These are toys, this comes with fact that they need a 8kb of space for each 8byte malloc. Just run a git diff and differences are huge. $ /usr/bin/time git diff HEAD^ > x 0.06user 0.01system 0:00.07elapsed 97%CPU (0avgtext+0avgdata 19172maxresident)k 0inputs+8outputs (0major+5591minor)pagefaults 0swaps $ LD_PRELOAD=3Dlibefence.so /usr/bin/time git diff HEAD^ > x Electric Fence 2.2 Copyright (C) 1987-1999 Bruce Perens 3.49user 0.94system 0:04.45elapsed 99%CPU (0avgtext+0avgdata 91920maxresident)k 0inputs+8outputs (0major+118069minor)pagefaults 0swaps > http://valgrind.sourceforge.net/ >=20 > Theses are very good tools for developers, finding memory corruption > (or other bugs like using uninitialized memory). >=20 > One of the motivation I asked for test cases is that a git developer = can > run these test cases under valgrind and can verify that we are never = out of range. >=20 > For an end user a git "crash" caused by trying to write to a write pr= otected page > is better than silently corrupting memory. >=20 > And a range check, followed by die(), is even easier to debug. > For an end user. > /Torsten >=20