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

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

On 20.10.13 08:05, Ondřej Bílka wrote:
> On Sun, Oct 20, 2013 at 07:47:06AM +0200, Torsten Bögershausen wrote:
>> (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>
>>> ---
>>> 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];
> 
> Why do you need static there?
Good point.
get_pathname() from path.c may be better.

>>> +
>>> +	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()
>>
> There is also
> e) modify allocation to place write protected page after buffer end.

Yes, I think this is what electric fence, DUMA or valgrind do:

http://sourceforge.jp/projects/freshmeat_efence/
http://duma.sourceforge.net/
http://valgrind.sourceforge.net/

Theses are very good tools for developers, finding memory corruption
(or other bugs like using uninitialized memory).

One of the motivation I asked for test cases is that a git developer can
run these test cases under valgrind and can verify that we are never out of range.

For an end user a git "crash" caused by trying to write to a write protected page
is better than silently corrupting memory.

And a range check, followed by die(), is even easier to debug.
For an end user.
/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.