[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [bug-patch] [PATCH] do not validate target name when it is specified
From: |
Andreas Gruenbacher |
Subject: |
Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line |
Date: |
Wed, 16 Feb 2011 01:21:14 +0100 |
User-agent: |
KMail/1.13.5 (Linux/2.6.34.7-0.4-desktop; KDE/4.4.4; x86_64; ; ) |
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
- [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/10
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/13
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/14
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/14
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/14
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/14
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/14
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line,
Andreas Gruenbacher <=
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Jim Meyering, 2011/02/16
- Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line, Andreas Gruenbacher, 2011/02/17