Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On Tue, Oct 22, 2013 at 2:02 AM, Johannes Sixt j...@kdbg.org wrote: (or maybe at higher level to skip checking out those paths). More like this, yeah. The good thing is we do not stop checking out if one entry fails. But due to the lack of worktree entries, one may accidentally remove files in new commits. So setting CE_VALID on failed-to-checkout entries might help. I'm not sure. But I won't pursue because Windows is not really my itch. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
Am 21.10.2013 03:31, schrieb Duy Nguyen: On Mon, Oct 21, 2013 at 12:57 AM, Antoine Pelisse apeli...@gmail.com wrote: My main motive was to not *stop* the process when a long path is met. Because somebody created a repository on Linux with a long file-name doesn't mean you should not be able to clone it *at all* on Windows. That should be handled at the Windows compatibility layer if Windows cannot handle long paths as Linux Naah... PATH_MAX is a silly, arbitrary limit. The Git data model does not forbid paths longer than PATH_MAX, be it 4096 or 260. A generic solution is needed. (or maybe at higher level to skip checking out those paths). More like this, yeah. -- Hannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On Mon, Oct 21, 2013 at 9:02 PM, Johannes Sixt j...@kdbg.org wrote: Am 21.10.2013 03:31, schrieb Duy Nguyen: On Mon, Oct 21, 2013 at 12:57 AM, Antoine Pelisse apeli...@gmail.com wrote: My main motive was to not *stop* the process when a long path is met. Because somebody created a repository on Linux with a long file-name doesn't mean you should not be able to clone it *at all* on Windows. That should be handled at the Windows compatibility layer if Windows cannot handle long paths as Linux Naah... PATH_MAX is a silly, arbitrary limit. The Git data model does not forbid paths longer than PATH_MAX, be it 4096 or 260. A generic solution is needed. (or maybe at higher level to skip checking out those paths). More like this, yeah. I would argue that this is probably even a bug on Linux, only harder (if not impossible) to trigger by accident as there's probably no git-client that will generate such trees. But a malicious client might. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On Mon, Oct 21, 2013 at 09:07:26PM +0200, Erik Faye-Lund wrote: I would argue that this is probably even a bug on Linux, only harder (if not impossible) to trigger by accident as there's probably no git-client that will generate such trees. But a malicious client might. I've just been poking through the impacts of these overflows, for that exact reason. I don't think any of them are easily triggerable by somebody sending you a malicious tree (e.g., the `remove_subtree` one only triggers when we have seen that tree in the filesystem, so it must be limited to `PATH_MAX`). Some of them are triggerable if you use particular options (e.g., the one in `match_order` is easy to trigger if you use `diff -O`). Still, they should all be fixed, even for Linux. I shouldn't have to trace the provenance of the data back through 10 functions just to find out a buffer overflow isn't easily exploitable. :) -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On Mon, Oct 21, 2013 at 03:14:39PM -0400, Jeff King wrote: On Mon, Oct 21, 2013 at 09:07:26PM +0200, Erik Faye-Lund wrote: I would argue that this is probably even a bug on Linux, only harder (if not impossible) to trigger by accident as there's probably no git-client that will generate such trees. But a malicious client might. I've just been poking through the impacts of these overflows, for that exact reason. I don't think any of them are easily triggerable by somebody sending you a malicious tree (e.g., the `remove_subtree` one only triggers when we have seen that tree in the filesystem, so it must be limited to `PATH_MAX`). Some of them are triggerable if you use particular options (e.g., the one in `match_order` is easy to trigger if you use `diff -O`). Actually, I take that back. The one in checkout_entry is quite easy to trigger if the victim checks out your tree. The rest are much harder, though. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
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.0...@gmail.com
Signed-off-by: Antoine Pelisse apeli...@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?
+
+ 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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
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.0...@gmail.com
Signed-off-by: Antoine Pelisse apeli...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On Sun, Oct 20, 2013 at 08:27:13AM +0200, Torsten Bögershausen wrote:
Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on
popelka.ms.mff.cuni.cz
Status: O
Content-Length: 2690
Lines: 89
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.0...@gmail.com
Signed-off-by: Antoine Pelisse apeli...@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:
You need to be selective which buffers are important.
http://sourceforge.jp/projects/freshmeat_efence/
http://duma.sourceforge.net/
These are toys, this comes with fact that they need a 8kb of space for
each 8byte malloc. Just run a git diff and differences are huge.
$ /usr/bin/time git diff HEAD^ x
0.06user 0.01system 0:00.07elapsed 97%CPU (0avgtext+0avgdata
19172maxresident)k
0inputs+8outputs (0major+5591minor)pagefaults 0swaps
$ LD_PRELOAD=libefence.so /usr/bin/time git diff HEAD^ x
Electric Fence 2.2 Copyright (C) 1987-1999 Bruce Perens
br...@perens.com
3.49user 0.94system 0:04.45elapsed 99%CPU (0avgtext+0avgdata
91920maxresident)k
0inputs+8outputs (0major+118069minor)pagefaults 0swaps
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On Sun, Oct 20, 2013 at 12:47 PM, Torsten Bögershausen tbo...@web.de wrote: 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() d+) Use strbuf -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
My main motive was to not *stop* the process when a long path is met.
Because somebody created a repository on Linux with a long file-name
doesn't mean you should not be able to clone it *at all* on Windows.
On Sun, Oct 20, 2013 at 12:33 PM, Duy Nguyen pclo...@gmail.com wrote:
On Sun, Oct 20, 2013 at 12:47 PM, Torsten Bögershausen tbo...@web.de wrote:
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()
This one stop the process, as the application crashes :-)
b) Silently shorten the path using strlcpy() instead of strcpy()
I was expecting this solution to fail later in a non-blocking way
(e.g. Can't checkout file $truncated_path, continuing with other
files). Maybe it would be better to look at each specific call site
and see if there is a way to report a problem ('return error(Can't
checkout %s: path too long)')
c) Avoid the overwriting and call die().
This one also stops the process, with an error (of course that's
better than point a)
d) Prepare a longer buffer using xmalloc()
d+) Use strbuf
This of course looks like the best solution, but I believe PATH_MAX
exists for a reason, and maybe we can't simply ignore that value ?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
On Mon, Oct 21, 2013 at 12:57 AM, Antoine Pelisse apeli...@gmail.com wrote: My main motive was to not *stop* the process when a long path is met. Because somebody created a repository on Linux with a long file-name doesn't mean you should not be able to clone it *at all* on Windows. That should be handled at the Windows compatibility layer if Windows cannot handle long paths as Linux (or maybe at higher level to skip checking out those paths). For PATH_MAX value, see [1]. [1] http://thread.gmane.org/gmane.comp.version-control.git/169012/focus=169310 -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH] Prevent buffer overflows when path is too big
(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.0...@gmail.com
Signed-off-by: Antoine Pelisse apeli...@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/t.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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
