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

[Jim Meyering]

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 @@
+x
' | 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 @@
+x
' | 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.