[bug-patch] [PATCH] do not reject an absolute first-file-name in -p0 patch

[Jim Meyering]

The latest version of patch introduced a regression.
Thanks to Alexey and Dmitry for reporting it:
  https://bugzilla.redhat.com/show_bug.cgi?id=667529#c14
Here's a patch to address it.

>From c25ead7a219c7c06077bdea3741139a4c07be5f6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 7 Feb 2011 11:17:49 +0100
Subject: [PATCH] do not reject an absolute first-file-name in -p0 patch

The fix for CVE-2010-4651 was too aggressive;  adjust it so
that the name validation is applied only to target names.
* src/util.c (strip_leading_slashes): Move target validation code...
* src/pch.c (validate_target_name): ...to this new function.
(maybe_reverse): Call it from here.
(intuit_diff_type): Likewise.
* tests/bad-filenames: Add more tests to exercise this case.
Adjust expected output to reflect diagnostic wording changes.
Reported by Alexey Tourbin via Dmitry V. Levin.
---
 ChangeLog           |   13 +++++++++++++
 src/pch.c           |   21 +++++++++++++++++++++
 src/util.c          |   11 -----------
 tests/bad-filenames |   29 +++++++++++++++++++++--------
 4 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index e72c82c..3062722 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2011-02-07  Jim Meyering  <address@hidden>
+
+       do not reject an absolute first-file-name in -p0 patch
+       The fix for CVE-2010-4651 was too aggressive;  adjust it so
+       that the name validation is applied only to target names.
+       * src/util.c (strip_leading_slashes): Move target validation code...
+       * src/pch.c (validate_target_name): ...to this new function.
+       (maybe_reverse): Call it from here.
+       (intuit_diff_type): Likewise.
+       * tests/bad-filenames: Add more tests to exercise this case.
+       Adjust expected output to reflect diagnostic wording changes.
+       Reported by Alexey Tourbin via Dmitry V. Levin.
+
 2011-02-03  Ozan Çağlayan <address@hidden>

        Create directory test case
diff --git a/src/pch.c b/src/pch.c
index 68f7bc8..f661b69 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -189,11 +189,31 @@ grow_hunkmax (void)
     return false;
 }

+static void
+validate_target_name (char const *n)
+{
+  char const *p = n;
+  if (IS_ABSOLUTE_FILE_NAME (p))
+    fatal ("rejecting absolute target file name: %s", quotearg (p));
+  while (*p)
+    {
+      if (*p == '.' && *++p == '.' && ( ! *++p || ISSLASH (*p)))
+       fatal ("rejecting target file name with \"..\" component: %s",
+              quotearg (n));
+      while (*p && ! ISSLASH (*p))
+       p++;
+      while (ISSLASH (*p))
+       p++;
+    }
+}
+
 static bool
 maybe_reverse (char const *name, bool nonexistent, bool is_empty)
 {
   bool looks_reversed = (! is_empty) < p_says_nonexistent[reverse ^ is_empty];

+  validate_target_name (name);
+
   /* Allow to create and delete empty files when we know that they are empty:
      in the "diff --git" format, we know that from the index header.  */
   if (is_empty
@@ -929,6 +949,7 @@ intuit_diff_type (bool need_header, mode_t *p_file_type)
        inerrno = stat_errno[i];
        invc = version_controlled[i];
        instat = st[i];
+       validate_target_name (inname);
       }

     return retval;
diff --git a/src/util.c b/src/util.c
index 553cfbd..f1187ff 100644
--- a/src/util.c
+++ b/src/util.c
@@ -1415,17 +1415,6 @@ strip_leading_slashes (char *name, int strip_leading)
              n = p+1;
        }
     }
-  if (IS_ABSOLUTE_FILE_NAME (n))
-    fatal ("rejecting absolute file name: %s", quotearg (n));
-  for (p = n; *p; )
-    {
-      if (*p == '.' && *++p == '.' && ( ! *++p || ISSLASH (*p)))
-       fatal ("rejecting file name with \"..\" component: %s", quotearg (n));
-      while (*p && ! ISSLASH (*p))
-       p++;
-      while (ISSLASH (*p))
-       p++;
-    }
   if ((strip_leading < 0 || s <= 0) && *n)
     {
       memmove (name, n, strlen (n) + 1);
diff --git a/tests/bad-filenames b/tests/bad-filenames
index f53a613..315447c 100644
--- a/tests/bad-filenames
+++ b/tests/bad-filenames
@@ -10,40 +10,53 @@ use_local_patch

 # ================================================================

-emit_patch()
+emit_2()
 {
 cat <<EOF
---- /dev/null
-+++ $1
+--- $1
++++ $2
 @@ -0,0 +1 @@
 +x
 EOF
 }

+emit_patch() { emit_2 /dev/null "$1"; }
+
 # Ensure that patch rejects an output file name that is absolute
 # or that contains a ".." component.

 check 'emit_patch /absolute/path | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting absolute file name: /absolute/path
+$PATCH: **** rejecting absolute target file name: /absolute/path
 status: 2
 EOF

 check 'emit_patch a/../z | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: a/../z
+$PATCH: **** rejecting target file name with ".." component: a/../z
 status: 2
 EOF

 check 'emit_patch a/../z | patch -p1; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: ../z
+$PATCH: **** rejecting target file name with ".." component: ../z
 status: 2
 EOF

 check 'emit_patch a/.. | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: a/..
+$PATCH: **** rejecting target file name with ".." component: a/..
 status: 2
 EOF

 check 'emit_patch ../z | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: ../z
+$PATCH: **** rejecting target file name with ".." component: ../z
 status: 2
 EOF
+
+check 'emit_2 /abs/path target | patch -p0; echo status: $?' <<EOF
+patching file target
+status: 0
+EOF
+
+echo x > target
+check 'emit_2 /abs/path target | patch -R -p0; echo status: $?' <<EOF
+patching file target
+status: 0
+EOF
--
1.7.4.2.g597a6