From owner-freebsd-current@FreeBSD.ORG Thu May 23 11:02:39 2013 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 05970C48 for ; Thu, 23 May 2013 11:02:39 +0000 (UTC) (envelope-from se@freebsd.org) Received: from nm22-vm0.bullet.mail.ird.yahoo.com (nm22-vm0.bullet.mail.ird.yahoo.com [212.82.109.250]) by mx1.freebsd.org (Postfix) with SMTP id 3E5A36AC for ; Thu, 23 May 2013 11:02:38 +0000 (UTC) Received: from [77.238.189.56] by nm22.bullet.mail.ird.yahoo.com with NNFMP; 23 May 2013 11:02:31 -0000 Received: from [46.228.39.82] by tm9.bullet.mail.ird.yahoo.com with NNFMP; 23 May 2013 11:02:31 -0000 Received: from [127.0.0.1] by smtp119.mail.ir2.yahoo.com with NNFMP; 23 May 2013 11:02:31 -0000 X-Yahoo-Newman-Id: 387626.1981.bm@smtp119.mail.ir2.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: FfjsqUkVM1l0en6e1Os9u3monQ5ZXRqBxcKOdYIahYRcIHk krhNC0WBYOlv09DmmDQyIQsHn4LR80K2YFJHjBefYlAR65YXMO2dycBZCbk. qHr3rJuL.Uj2y5M.R0ZR4mZyFga_.P5kjmn0hwr0DR8.gQ3eThqWmOUTaQnl iHDxZR62Rk2ZW41kuqSqpy3n.ovOuKp3pdwUL_nbi18tb5YqI2DWPtFfCZh7 woBCCHmiozhEe.74Xz7.v8VCpKTv_xQfABhIosm7M4bLwBLfYEeZU_F9EDmz 8vkUoSXD78MeRUF6Pi2yxWNbyWhxqTmUSXlMFO3o5PM54BreS_OIt_WEdA2d 4VccCNzCZMSokRAHrhDOjh7vx7_3p4xXRd1Ua4HM71VaZyx4kBRue_WMeP4g JSBGHM_TAF5_2v8FHxxrjgjjZgHiOqnzKZ3DFmlhDChLzVzbEXB9UZA5vm0J _xif6VAeU1jzN.sNAP1ygb1H9t_HWI840fmsNYnPX2WOQbnOOUPyOjIIHVME GWb40OlNmCYFjbEzeZfI8YjGWXrxZThoInzwX6Dk.DDdQzmXt6FowtA-- X-Yahoo-SMTP: iDf2N9.swBDAhYEh7VHfpgq0lnq. X-Rocket-Received: from [192.168.119.11] (se@87.158.30.123 with ) by smtp119.mail.ir2.yahoo.com with SMTP; 23 May 2013 11:02:31 +0000 UTC Message-ID: <519DF73E.40307@freebsd.org> Date: Thu, 23 May 2013 13:02:22 +0200 From: Stefan Esser User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/20130509 Thunderbird/17.0.6 MIME-Version: 1.0 To: freebsd-current@freebsd.org Subject: Bug in BSD patch (was: Re: absolute paths in port patch files) References: <519DBC27.9030600@gmx.com> In-Reply-To: <519DBC27.9030600@gmx.com> X-Enigmail-Version: 1.5.1 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: delphij@freebsd.org, gabor@freebsd.org X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 23 May 2013 11:02:39 -0000 Am 23.05.2013 08:50, schrieb dt71@gmx.com: > In the ports system, some patch files use absolute paths. Run > > ls -d /usr/ports/*/*/files | xargs -IX grep -rnE '^([+][+][+]|---) /' X > > to see what I mean. For example, there is: > > /usr/ports/textproc/texi2html/files/patch-texi2html.pl:2:+++ > /usr/local/bin/texi2html 2012-07-09 10:53:16.000000000 +0200 This should not be a problem, but I was able to reproduce the behaviour you observed and I think it is due to a bug in BSD patch (see test output at the end of this mail). But lets first look at specified behaviour (AKA theory). Quoting from the patch(1) man page: ----------------------------------------------------------------- patch will choose the file name by performing the following steps, with the first match used: 1. If patch is operating in strict IEEE Std 1003.1-2008 (“POSIX.1”) mode, the first of the “old”, “new” and “index” file names that exist is used. Otherwise, patch will examine either the “old” and “new” file names or, for a non-context diff, the “index” file name, and choose the file name with the fewest path components, the short- est basename, and the shortest total file name length (in that order). ----------------------------------------------------------------- Your patch file example has the following file information: --- texi2html.pl 2012-07-09 10:54:41.000000000 +0200 +++ /usr/local/bin/texi2html 2012-07-09 10:53:16.000000000 +0200 Patch will modify "texi2html.pl" in the work directory. The other file name (/usr/local/bin/texi2html) is ignored. So, there is no problem with this patch, if patch works as advertised. I assume that the same is true for most other patch files, but it seems a good idea to verify "good" names are used for the file to patch. Such a test could be added to the "checkpatch" function in portlint. While it would take some effort to reproduce the whole file selection logic used by patch, it is easy to warn about all absolute path names, whether actually used by patch or not. I have used the following, slightly more complex function to look for patches with absolute path names that might cause problems: # cd /usr/ports # ls -d */*/files | xargs grep -2rnE '^(---|\+\+\+|\*\*\*|index:) /' | \ grep -v '/dev/null' | \ sed -nE 's/^--|(\/([-+_.A-Za-z0-9]|::)+)[:][0-9]+[:].*/\1/p' | \ uniq -d | \ xargs grep -nE '^(---|\+\+\+|\*\*\*|index:) /' I'm looking at lines starting with "---", "+++", "***", or "index:" and followed by a blank and slash, but excluding /dev/null. Lines with matches are replaced by the patch file name and separated by blank lines for each chunk. The "uniq -d" permits only patch chunks with more than one absolute path name through, and the final grep selects the matching lines. This command does not detect any patch file with more than one absolute path in the whole ports tree. (You can easily verify that it works as advertised by adding absolute path names to file names in a patch file.) > Some patch files refer to target files in the /tmp directory. > Theoretically, this means that malicious regular users are able to > fiddle with the patching process: by creating the target files in the > /tmp directory, they are able to silently cause patches to apply to > bogus files in the /tmp directory instead of the intended files in the > port's work directory. In the extreme case, a malicious user could cause > ports to be built without certain security patches. The user could also > try a symlink attack. This is not impossible, due to the way the target file is selected by patch. If a target file is in a deep directory within the work directory, then a file in e.g. /tmp might be selected as the target, instead. Such a file could be placed there, waiting for the upgrade of an existing port being compiled on that system. But it is highly unlikely, that the chunk will apply cleanly, and thus patch will abort without changing the bogus target file, in most cases. Security conscious people might mislike "highly unlikely" and "in most cases", though ;-) > Some patch files refer to target files that "will be" installed, such as > /usr/local/bin/texi2html. A patch in the textproc/texi2html port was the > basis for me finding out about this issue: the port was already > installed, and was being built to be reinstalled, and the patching > process tried to modify the installed /usr/local/bin/texi2html file, but > failed (the following files were created: /usr/local/bin/texi2html.orig > and /usr/local/bin/texi2html.rej). However, theoretically, if the > patching process succeeds on the already-installed files, then later, > unpatched files will be reinstalled. I could indeed reproduce the behaviour you describe! This appears to be a problem with the new BSD patch in -CURRENT: # gnupatch -d /usr/ports/textproc/texi2html/work/texi2html-5.0 -E -p0 \ -V simple -C < files/patch-texi2html.pl Hmm... Looks like a unified diff to me... The text leading up to this was: -------------------------- |--- texi2html.pl 2012-07-09 10:54:41.000000000 +0200 |+++ /usr/local/bin/texi2html 2012-07-09 10:53:16.000000000 +0200 -------------------------- Patching file texi2html.pl using Plan A... Hunk #1 succeeded at 1933. done # bsdpatch -d /usr/ports/textproc/texi2html/work/texi2html-5.0 -E -p0 \ -V simple -C < files/patch-texi2html.pl Hmm... Looks like a unified diff to me... The text leading up to this was: -------------------------- |--- texi2html.pl 2012-07-09 10:54:41.000000000 +0200 |+++ /usr/local/bin/texi2html 2012-07-09 10:53:16.000000000 +0200 -------------------------- Patching file /usr/local/bin/texi2html using Plan A... Reversed (or previously applied) patch detected! Assume -R? [y] Obviously, BSD patch does not select the file with the shortest path name as the target. I have not checked the source code, but you should definitely open a PR for this bug! Regards, STefan