Date: Thu, 23 May 2013 13:02:22 +0200 From: Stefan Esser <se@freebsd.org> To: freebsd-current@freebsd.org Cc: delphij@freebsd.org, gabor@freebsd.org Subject: Bug in BSD patch (was: Re: absolute paths in port patch files) Message-ID: <519DF73E.40307@freebsd.org> In-Reply-To: <519DBC27.9030600@gmx.com> References: <519DBC27.9030600@gmx.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?519DF73E.40307>