[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: Jim Meyering
Subject: Re: [bug-patch] [PATCH] do not validate target name when it is specified on the command line
Date: Wed, 16 Feb 2011 15:27:42 +0100

Andreas Gruenbacher wrote:

> 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?

Perhaps not unsafe, but I now see that it can be improved...

Patch still stats the offending file.
For example, running this

printf '
--- /dev/null
+++ a/../z
@@ -0,0 +1 @@
' | src/patch -f -p1 --dry-run

Usually prints the expected output:

    Ignoring potentially dangerous file name ../z
    can't find file to patch at input line 4
    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

But if you create ../z, then it will print the new "Ignoring ..."
diagnostic twice.  I.e., this

touch ../z
printf '
--- /dev/null
+++ a/../z
@@ -0,0 +1 @@
' | src/patch -f -p1 --dry-run 2>&1|grep -c Ignoring

prints "2".

Thus, if you create a file named "z" in tests/
then "make check" will fail.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]