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

[Jim Meyering]

Jim Meyering wrote:
> 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.
>
> 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.

While seemingly correct, that patch provokes a minor "make distcheck"
failure due to a new test leaving behind the file, "tests/target".
With this incremental change, "make distcheck" now passes:

>From 356b65f3c6fc7e366a01c4dc8c389447d2fa6273 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 7 Feb 2011 16:52:30 +0100
Subject: [PATCH] perform bad-filenames tests in temporary dir

---
 tests/bad-filenames |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/tests/bad-filenames b/tests/bad-filenames
index 315447c..00e6533 100644
--- a/tests/bad-filenames
+++ b/tests/bad-filenames
@@ -7,6 +7,7 @@
 . $srcdir/test-lib.sh

 use_local_patch
+use_tmpdir

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

--
1.7.4.2.g597a6


Here's the merged change-set:


>From 8d2bb2690cf53a3770c342215b24a6e623c3377f Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 7 Feb 2011 11:17:49 +0100
Subject: [PATCH-v2] 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.
Now that we create a file, use "use_tmpdir".
Reported by Alexey Tourbin via Dmitry V. Levin.
---
 ChangeLog           |   13 +++++++++++++
 src/pch.c           |   21 +++++++++++++++++++++
 src/util.c          |   11 -----------
 tests/bad-filenames |   30 ++++++++++++++++++++++--------
 4 files changed, 56 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..00e6533 100644
--- a/tests/bad-filenames
+++ b/tests/bad-filenames
@@ -7,43 +7,57 @@
 . $srcdir/test-lib.sh

 use_local_patch
+use_tmpdir

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

-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