Ignore dangerous filenames instead of failing immediately

* src/pch.c (name_is_valid): New function.
(intuit_diff_type, best_name): Use name_is_valid() here.
(strip_leading_slashes): Remove name validation tests from here.
* tests/bad-filenames: Add more tests for covering more of the
file name guessing corner cases in intuit_diff_type(), update the
existing tests.
* NEWS: Update.

5 files changed, 120 insertions, 28 deletions

diff --git a/ChangeLog b/ChangeLog
index 4fd8a9b..defc7be 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2011-02-16  Andreas Gruenbacher <agruen@linbit.com>
+
+	* src/pch.c (name_is_valid): New function.
+	(intuit_diff_type, best_name): Use name_is_valid() here.
+	(strip_leading_slashes): Remove name validation tests from here.
+	* tests/bad-filenames: Add more tests for covering more of the
+	file name guessing corner cases in intuit_diff_type(), update the
+	existing tests.
+	* NEWS: Update.
+
 2011-02-15  Andreas Gruenbacher <agruen@linbit.com>
 
 	* src/patch.c (main): Fix use of initialized outst and add an
diff --git a/NEWS b/NEWS
index 65d3796..f13002a 100644
--- a/NEWS
+++ b/NEWS
@@ -1,4 +1,4 @@
-* patch now rejects a destination file name that is absolute or that contains
+* Patch now ignores destination file names that are absolute or that contain
   a component of "..".  This addresses CVE-2010-4651,
 * Support for most features of the "diff --git" format: renames and copies,
   permission changes, symlink diffs.  Caveats:
diff --git a/src/pch.c b/src/pch.c
index 68f7bc8..41c15b6 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -376,6 +376,31 @@ skip_hex_digits (char const *str)
   return s == str ? NULL : s;
 }
 
+static bool
+name_is_valid (char const *name)
+{
+  const char *n = name;
+
+  if (IS_ABSOLUTE_FILE_NAME (name))
+    {
+      say ("Ignoring potentially dangerous file name %s\n", quotearg (name));
+      return false;
+    }
+  for (n = name; *n; )
+    {
+      if (*n == '.' && *++n == '.' && ( ! *++n || ISSLASH (*n)))
+        {
+	  say ("Ignoring potentially dangerous file name %s\n", quotearg (name));
+	  return false;
+	}
+      while (*n && ! ISSLASH (*n))
+	n++;
+      while (ISSLASH (*n))
+	n++;
+    }
+  return true;
+}
+
 /* Determine what kind of diff is in the remaining part of the patch file. */
 
 static enum diff
@@ -829,7 +854,7 @@ intuit_diff_type (bool need_header, mode_t *p_file_type)
 	      else
 		{
 		  stat_errno[i] = 0;
-		  if (posixly_correct)
+		  if (posixly_correct && name_is_valid (p_name[i]))
 		    break;
 		}
 	      i0 = i;
@@ -1002,6 +1027,7 @@ best_name (char *const *name, int const *ignore)
   /* Of those, take the first name.  */
   for (i = OLD;  i <= INDEX;  i++)
     if (name[i] && !ignore[i]
+	&& name_is_valid (name[i])
 	&& components[i] == components_min
 	&& basename_len[i] == basename_len_min
 	&& len[i] == len_min)
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..0bc23eb 100644
--- a/tests/bad-filenames
+++ b/tests/bad-filenames
@@ -7,6 +7,7 @@
 . $srcdir/test-lib.sh
 
 use_local_patch
+use_tmpdir
 
 # ================================================================
 
@@ -23,27 +24,93 @@ EOF
 # 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
-status: 2
+check 'emit_patch ../z | patch -f -p1 --dry-run || echo status: $?' <<EOF
+patching file z
 EOF
 
-check 'emit_patch a/../z | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: a/../z
-status: 2
+check 'emit_patch /absolute/path | patch -f -p0 --dry-run || echo status: $?' <<EOF
+Ignoring potentially dangerous file name /absolute/path
+can't find file to patch at input line 3
+Perhaps you used the wrong -p or --strip option?
+The text leading up to this was:
+--------------------------
+|--- /dev/null
+|+++ /absolute/path
+--------------------------
+No file to patch.  Skipping patch.
+1 out of 1 hunk ignored
+status: 1
 EOF
 
-check 'emit_patch a/../z | patch -p1; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: ../z
-status: 2
+check 'emit_patch a/../z | patch -f -p0 --dry-run || echo status: $?' <<EOF
+Ignoring potentially dangerous file name a/../z
+can't find file to patch at input line 3
+Perhaps you used the wrong -p or --strip option?
+The text leading up to this was:
+--------------------------
+|--- /dev/null
+|+++ a/../z
+--------------------------
+No file to patch.  Skipping patch.
+1 out of 1 hunk ignored
+status: 1
 EOF
 
-check 'emit_patch a/.. | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: a/..
-status: 2
+check 'emit_patch a/../z | patch -f -p1 --dry-run || echo status: $?' <<EOF
+Ignoring potentially dangerous file name ../z
+can't find file to patch at input line 3
+Perhaps you used the wrong -p or --strip option?
+The text leading up to this was:
+--------------------------
+|--- /dev/null
+|+++ a/../z
+--------------------------
+No file to patch.  Skipping patch.
+1 out of 1 hunk ignored
+status: 1
 EOF
 
-check 'emit_patch ../z | patch -p0; echo status: $?' <<EOF
-$PATCH: **** rejecting file name with ".." component: ../z
-status: 2
+check 'emit_patch ../z | patch -f -p0 --dry-run || echo status: $?' <<EOF
+Ignoring potentially dangerous file name ../z
+can't find file to patch at input line 3
+Perhaps you used the wrong -p or --strip option?
+The text leading up to this was:
+--------------------------
+|--- /dev/null
+|+++ ../z
+--------------------------
+No file to patch.  Skipping patch.
+1 out of 1 hunk ignored
+status: 1
+EOF
+
+cat > d.diff <<EOF
+--- f
++++ ../g
+@@ -1 +1 @@
+-1
++1
+EOF
+
+check 'patch -f -p0 --dry-run < d.diff || echo status: $?' <<EOF
+can't find file to patch at input line 3
+Perhaps you used the wrong -p or --strip option?
+The text leading up to this was:
+--------------------------
+|--- f
+|+++ ../g
+--------------------------
+No file to patch.  Skipping patch.
+1 out of 1 hunk ignored
+status: 1
+EOF
+
+echo 1 > f
+check 'patch -f -p0 --dry-run < d.diff || echo status: $?' <<EOF
+patching file f
+EOF
+
+echo 1 > g
+check 'patch -f -p1 --dry-run < d.diff || echo status: $?' <<EOF
+patching file g
 EOF