Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line

[Andreas Gruenbacher]

On Monday 14 February 2011 10:34:59 Jim Meyering wrote:
> Andreas Gruenbacher wrote:
> 
> > On Monday 14 February 2011 10:16:12 Jim Meyering wrote:
> >> I see what you mean, but invalid names seem important enough that I would
> >> not want to be prompted -- not even with a warning -- about the patch
> >> in question.
> >
> > On the other hand, immediately aborting when we see an invalid name (like
> > in the current git) is also not appreciated?
> 
> When it comes to security, even low-risk things like this,
> I think it pays to be extra careful, even if that ends up
> causing minor inconvenience.

I guess I don't see the point.  That's what the code in the current repo
does, but apparently that's too much?

> >> When being prompted, it is too easy to miss the preceding
> >> warning among the already relatively verbose output.
> >
> > What harm does it do if the warning is overlooked?
> 
> With a prompt, it's too easy for the naive user to type in some variant
> of the invalid file name.  Obviously neither you nor I would try
> "../../f" when patch says that "../f" doesn't work, but for a beginner,
> even ../../../etc/passwd might not raise an eyebrow.  Issuing the prompt
> makes abuse via social engineering a tiny bit easier.

A person with that level of technical understanding might just as well apply a
patch while in the root directory, no?

Here's a patch that implements what I have in mind.  Do you really think that
this approach is too unsafe?

Thanks,
Andreas


diff --git a/src/pch.c b/src/pch.c
index 68f7bc8..924c11a 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -74,6 +74,7 @@ static char *p_c_function;            /* the C function a 
hunk is in */
 
 static char *scan_linenum (char *, lin *);
 static enum diff intuit_diff_type (bool, mode_t *);
+static bool name_is_valid (char const *);
 static enum nametype best_name (char * const *, int const *);
 static int prefix_components (char *, bool);
 static size_t pget_line (size_t, int, bool, bool);
@@ -829,7 +830,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;
@@ -963,6 +964,31 @@ prefix_components (char *filename, bool checkdirs)
   return count;
 }
 
+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;
+}
+
 /* Return the index of the best of NAME[OLD], NAME[NEW], and NAME[INDEX].
    Ignore null names, and ignore NAME[i] if IGNORE[i] is nonzero.
    Return NONE if all names are ignored.  */
@@ -1002,6 +1028,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