Re: [PATCH] Prevent buffer overflows when path is too big

["Torsten Bögershausen" <tboegi@web.de>]

(may be s/path is too big/path is too long/ ?)

On 19.10.13 12:52, Antoine Pelisse wrote:
> Currently, most buffers created with PATH_MAX length, are not checked
> when being written, and can overflow if PATH_MAX is not big enough to
> hold the path.
> 
> Fix that by using strlcpy() where strcpy() was used, and also run some
> extra checks when copy is done with memcpy().
> 
> Reported-by: Wataru Noguchi <wnoguchi.0727@gmail.com>
> Signed-off-by: Antoine Pelisse <apelisse@gmail.com>
> ---
>  abspath.c        | 10 +++++++---
>  diffcore-order.c |  2 +-
>  entry.c          | 14 ++++++++++----
>  unpack-trees.c   |  2 ++
>  4 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/abspath.c b/abspath.c
> index 64adbe2..0e60ba4 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -216,11 +216,15 @@ const char *absolute_path(const char *path)
>  const char *prefix_filename(const char *pfx, int pfx_len, const char *arg)
>  {
>  	static char path[PATH_MAX];
> +
> +	if (pfx_len > PATH_MAX)
I think this should be 
if (pfx_len > PATH_MAX-1) /* Keep 1 char for '\0'
> +		die("Too long prefix path: %s", pfx);
> +
>  #ifndef GIT_WINDOWS_NATIVE
>  	if (!pfx_len || is_absolute_path(arg))
>  		return arg;
>  	memcpy(path, pfx, pfx_len);
> -	strcpy(path + pfx_len, arg);
> +	strlcpy(path + pfx_len, arg, PATH_MAX - pfx_len);

I'm not sure how to handle overlong path in general, there are several ways:
a) Silently overwrite memory (with help of memcpy() and/or strcpy()
b) Silently shorten the path using strlcpy() instead of strcpy()
c) Avoid the overwriting and call die().
d) Prepare a longer buffer using xmalloc()

Today we do a), this is not a good thing and the worst choice.


A little side note:
  It would be good to have test cases for either b), c) or d).

  As PATH_MAX is OS dependend, we need both a main program written in c
  and a test case written in t/txxxx.sh.
  Some existing code can be used for inspiration, e.g. 
  test-wildmatch.c in combination with t/t3070-wildmatch.sh
  This willl allow us to reproduce the error, and define how git should behave.

End of the side note, let's look closer at the suggested patch, implementing b)

Silently shortening an overlong path like
"/foo/bar/baz" could result something like

"/foo/bar/ba" /* That filename may be part of the repo too */
or
"/foo/bar/" /*  This is a directory, not a file name */

In either case the end user has no idea why git choose another file name.
And this could be hard to debug.
After a couple of hours she/he may send a message asking for help to the mailing list,
and we end up in more people doing debugging.

c) Is much easier to debug:
  Git can not handle this situation, and we print out the parameters in die()

I would prefer c) over b), make clear that git can't handle that situation.

d) Would mean some more re-factoring: Check all callers to prefix_filename().
Some of them call xstrdup() after prefix_filename(), which mean that we could
change prefix_filename() to always return new string which is long enough via xmalloc(),
and not a static buffer.

So we come to the next point (and this is my personal experience,
so please don't get me wrong):
how much time can you spend on this?

If the answer is kind of "very little", I would go for c)
  Avoid the silent memory corruption, and say to the user "we can not handle this"

If the answer is kind of "little", I would go for c) and a test program,
  covering all the different code path in abspath()
  (WHich may deserve a refactoring as well, since the code for GIT_WINDOWS_NATIVE
  is very similar to the non-GIT_WINDOWS_NATIVE)

If the answer is kind of "more than little", a different strategie may be better:
  Start sending a patch for c)
  I think we have enough volunteers here for a review, so we can life without the test code.
  On top of that, some volunteer can develop d).
  
So far I have only looked at abspath(), and your patch touches more places.
I think more and more that calling die()
with all information included why we call die() is a good starting point.

It will allow the users to see what is going on.
May be the repo can be re-arranged to use shorter path names than what we can handle.
[snip]
 
/Torsten

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.