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?

No.  They generally do not have root access ;-)

As explained, I feel that it is slightly safer not to prompt,
given an invalid file name.

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

As I said: "slightly...".  No big deal.

> 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 *);

I would eliminate this declaration, and instead simply move
the definition of name_is_valid to precede the first use.
Less duplication is better.

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

Omitting the space-after-"*" would be more consistent:

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++;

When viewing the above using a fixed-width font, and 8-space
TAB stops shows why using mixed TABs and spaces for indentation
is bad.  It makes the patch much harder to read.

Have you considered indenting only with spaces?
I've made that the policy in a few projects, and it
has worked well.

> +    }
> +  return true;
> +}