Bug in BSD patch (was: Re: absolute paths in port patch files)

Am 23.05.2013 08:50, schrieb dt71_at_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