[PATCH] Fix buffer overflow in git-grep

[PATCH] Fix buffer overflow in git-grep

[Dmitry Potapov]

If PATH_MAX on your system is smaller than any path stored in the git
repository, that can cause memory corruption inside of the grep_tree
function used by git-grep.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
 builtin-grep.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index ef29910..530a53d 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -441,14 +441,17 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
 	len = strlen(path_buf);
 
 	while (tree_entry(tree, &entry)) {
-		strcpy(path_buf + len, entry.path);
+		int te_len = tree_entry_len(entry.path, entry.sha1);
+		if (len + te_len >= PATH_MAX + tn_len)
+			die ("path too long: %s", path_buf+tn_len);
+		memcpy(path_buf + len, entry.path, te_len);
 
 		if (S_ISDIR(entry.mode))
 			/* Match "abc/" against pathspec to
 			 * decide if we want to descend into "abc"
 			 * directory.
 			 */
-			strcpy(path_buf + len + tree_entry_len(entry.path, entry.sha1), "/");
+			strcpy(path_buf + len + te_len, "/");
 
 		if (!pathspec_matches(paths, down))
 			;
-- 
1.5.6.3.1.gb5587a

Re: [PATCH] Fix buffer overflow in git-grep

[Johannes Schindelin]

Hi,

On Wed, 16 Jul 2008, Dmitry Potapov wrote:

> If PATH_MAX on your system is smaller than any path stored in the git 
> repository, that can cause memory corruption inside of the grep_tree 
> function used by git-grep.

Let me guess: Windows? *giggles*

> diff --git a/builtin-grep.c b/builtin-grep.c
> index ef29910..530a53d 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -441,14 +441,17 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
>  	len = strlen(path_buf);
>  
>  	while (tree_entry(tree, &entry)) {
> -		strcpy(path_buf + len, entry.path);
> +		int te_len = tree_entry_len(entry.path, entry.sha1);
> +		if (len + te_len >= PATH_MAX + tn_len)
> +			die ("path too long: %s", path_buf+tn_len);
> +		memcpy(path_buf + len, entry.path, te_len);

That is brutal.  Does grep_tree() not work on tree objects in memory?  In 
that case, you prevent the user from grepping, only because she is on a 
suboptimal platform, _even if_ even that platform could cope with it.

It's not like the path is ever used to access a file, right?

Maybe you should convert the path_buf to a strbuf instead.

Ciao,
Dscho

Re: [PATCH] Fix buffer overflow in git-grep

[Dmitry Potapov]

On Wed, Jul 16, 2008 at 12:35:06PM +0200, Johannes Schindelin wrote:
> >  
> >  	while (tree_entry(tree, &entry)) {
> > -		strcpy(path_buf + len, entry.path);
> > +		int te_len = tree_entry_len(entry.path, entry.sha1);
> > +		if (len + te_len >= PATH_MAX + tn_len)
> > +			die ("path too long: %s", path_buf+tn_len);
> > +		memcpy(path_buf + len, entry.path, te_len);
> 
> That is brutal.  Does grep_tree() not work on tree objects in memory?  In 
> that case, you prevent the user from grepping, only because she is on a 
> suboptimal platform, _even if_ even that platform could cope with it.

Sure, but other git commands do not work much better in this case.
In fact, what you called as "brutal" may be considered as very
polite comparing to what other git commands did.

For instance, git show will show you nothing at all and exit with 0.
The same problem with git whatchanged. The whole history mysteriously
disappeared at that commit, and git whatchanged exited with 0 without
any error or warning... Though git log will show you all history, but
if you run it with -p then it will also exit with zero at this commit
silently like previously history do not exist at all. So, I didn't see
any reason to make git grep to work in the situation where practically
any other git command does not. I guess, they should be corrected too,
but I did not have time to look at them yet.

> 
> It's not like the path is ever used to access a file, right?
> 
> Maybe you should convert the path_buf to a strbuf instead.

It is probably a good suggestion, but I just wanted to provided a quick
fix to what may be considered as security issue. Of course, you usually
do not grep on untrusted repos, but if you did and something nasty
happened to you. I don't think it will help Git's reputation as being
secure and reliable...

Now the question is whether we really want to fix all Git commands that
do not touch the work tree to work with filenames longer than PATH_MAX?

Dmitry

Re: [PATCH] Fix buffer overflow in git-grep

[Dmitry Potapov]

On Wed, Jul 16, 2008 at 3:54 PM, Dmitry Potapov <dpotapov@gmail.com> wrote:
>
> For instance, git show will show you nothing at all and exit with 0.
> The same problem with git whatchanged. The whole history mysteriously
> disappeared at that commit, and git whatchanged exited with 0 without
> any error or warning... Though git log will show you all history, but
> if you run it with -p then it will also exit with zero at this commit
> silently like previously history do not exist at all.

It turned out that git actually crashed while doing diff, but because it
was a forked process, the parent exited normally with code 0. I wonder
should not the parent process to exit with a non-zero code if the child
died by SIGSEG or another signal?

Dmitry

Re: [PATCH] Fix buffer overflow in git-grep

[Johannes Schindelin]

Hi,

On Wed, 16 Jul 2008, Dmitry Potapov wrote:

> On Wed, Jul 16, 2008 at 3:54 PM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> >
> > For instance, git show will show you nothing at all and exit with 0.
> > The same problem with git whatchanged. The whole history mysteriously
> > disappeared at that commit, and git whatchanged exited with 0 without
> > any error or warning... Though git log will show you all history, but
> > if you run it with -p then it will also exit with zero at this commit
> > silently like previously history do not exist at all.
> 
> It turned out that git actually crashed while doing diff, but because it
> was a forked process, the parent exited normally with code 0. I wonder
> should not the parent process to exit with a non-zero code if the child
> died by SIGSEG or another signal?

We'll probably need to use the MinGW pager handling for Unix, too, and 
check for a died child.

Not overly trivial, though.

Ciao,
Dscho

[PATCH] Fix buffer overflow in git diff

[Dmitry Potapov]

If PATH_MAX on your system is smaller than a path stored, it may cause
buffer overflow and stack corruption in diff_addremove() and diff_change()
functions when running git-diff

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
 diff-lib.c  |    8 ++++----
 diff.c      |   11 ++---------
 diff.h      |    9 ++++-----
 revision.c  |    4 ++--
 tree-diff.c |   26 ++++++++++++++++++++++----
 5 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index b17722d..e7eaff9 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -171,7 +171,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			if (silent_on_removed)
 				continue;
 			diff_addremove(&revs->diffopt, '-', ce->ce_mode,
-				       ce->sha1, ce->name, NULL);
+				       ce->sha1, ce->name);
 			continue;
 		}
 		changed = ce_match_stat(ce, &st, ce_option);
@@ -184,7 +184,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		newmode = ce_mode_from_stat(ce, st.st_mode);
 		diff_change(&revs->diffopt, oldmode, newmode,
 			    ce->sha1, (changed ? null_sha1 : ce->sha1),
-			    ce->name, NULL);
+			    ce->name);
 
 	}
 	diffcore_std(&revs->diffopt);
@@ -208,7 +208,7 @@ static void diff_index_show_file(struct rev_info *revs,
 				 const unsigned char *sha1, unsigned int mode)
 {
 	diff_addremove(&revs->diffopt, prefix[0], mode,
-		       sha1, ce->name, NULL);
+		       sha1, ce->name);
 }
 
 static int get_stat_data(struct cache_entry *ce,
@@ -312,7 +312,7 @@ static int show_modified(struct oneway_unpack_data *cbdata,
 		return 0;
 
 	diff_change(&revs->diffopt, oldmode, mode,
-		    old->sha1, sha1, old->name, NULL);
+		    old->sha1, sha1, old->name);
 	return 0;
 }
 
diff --git a/diff.c b/diff.c
index 78c4d3a..386de82 100644
--- a/diff.c
+++ b/diff.c
@@ -3356,9 +3356,8 @@ int diff_result_code(struct diff_options *opt, int status)
 void diff_addremove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
-		    const char *base, const char *path)
+		    const char *concatpath)
 {
-	char concatpath[PATH_MAX];
 	struct diff_filespec *one, *two;
 
 	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES) && S_ISGITLINK(mode))
@@ -3380,9 +3379,6 @@ void diff_addremove(struct diff_options *options,
 		addremove = (addremove == '+' ? '-' :
 			     addremove == '-' ? '+' : addremove);
 
-	if (!path) path = "";
-	sprintf(concatpath, "%s%s", base, path);
-
 	if (options->prefix &&
 	    strncmp(concatpath, options->prefix, options->prefix_length))
 		return;
@@ -3403,9 +3399,8 @@ void diff_change(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
-		 const char *base, const char *path)
+		 const char *concatpath)
 {
-	char concatpath[PATH_MAX];
 	struct diff_filespec *one, *two;
 
 	if (DIFF_OPT_TST(options, IGNORE_SUBMODULES) && S_ISGITLINK(old_mode)
@@ -3418,8 +3413,6 @@ void diff_change(struct diff_options *options,
 		tmp = old_mode; old_mode = new_mode; new_mode = tmp;
 		tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
 	}
-	if (!path) path = "";
-	sprintf(concatpath, "%s%s", base, path);
 
 	if (options->prefix &&
 	    strncmp(concatpath, options->prefix, options->prefix_length))
diff --git a/diff.h b/diff.h
index 5dc0cb5..50fb5dd 100644
--- a/diff.h
+++ b/diff.h
@@ -14,12 +14,12 @@ typedef void (*change_fn_t)(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
-		 const char *base, const char *path);
+		 const char *fullpath);
 
 typedef void (*add_remove_fn_t)(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
-		    const char *base, const char *path);
+		    const char *fullpath);
 
 typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 		struct diff_options *options, void *data);
@@ -164,14 +164,13 @@ extern void diff_addremove(struct diff_options *,
 			   int addremove,
 			   unsigned mode,
 			   const unsigned char *sha1,
-			   const char *base,
-			   const char *path);
+			   const char *fullpath);
 
 extern void diff_change(struct diff_options *,
 			unsigned mode1, unsigned mode2,
 			const unsigned char *sha1,
 			const unsigned char *sha2,
-			const char *base, const char *path);
+			const char *fullpath);
 
 extern void diff_unmerge(struct diff_options *,
 			 const char *path,
diff --git a/revision.c b/revision.c
index fc66755..8dc3ca7 100644
--- a/revision.c
+++ b/revision.c
@@ -259,7 +259,7 @@ static int tree_difference = REV_TREE_SAME;
 static void file_add_remove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
-		    const char *base, const char *path)
+		    const char *fullpath)
 {
 	int diff = REV_TREE_DIFFERENT;
 
@@ -285,7 +285,7 @@ static void file_change(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
-		 const char *base, const char *path)
+		 const char *fullpath)
 {
 	tree_difference = REV_TREE_DIFFERENT;
 	DIFF_OPT_SET(options, HAS_CHANGES);
diff --git a/tree-diff.c b/tree-diff.c
index e1e2e6c..4c67ea5 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -15,6 +15,15 @@ static char *malloc_base(const char *base, int baselen, const char *path, int pa
 	return newbase;
 }
 
+static char *malloc_fullname(const char *base, int baselen, const char *path, int pathlen)
+{
+	char *fullname = xmalloc(baselen + pathlen + 1);
+	memcpy(fullname, base, baselen);
+	memcpy(fullname + baselen, path, pathlen);
+	fullname[baselen + pathlen] = 0;
+	return fullname;
+}
+
 static void show_entry(struct diff_options *opt, const char *prefix, struct tree_desc *desc,
 		       const char *base, int baselen);
 
@@ -24,6 +33,7 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 	const char *path1, *path2;
 	const unsigned char *sha1, *sha2;
 	int cmp, pathlen1, pathlen2;
+	char *fullname;
 
 	sha1 = tree_entry_extract(t1, &path1, &mode1);
 	sha2 = tree_entry_extract(t2, &path2, &mode2);
@@ -56,14 +66,20 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
 		int retval;
 		char *newbase = malloc_base(base, baselen, path1, pathlen1);
 		if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))
+		{
+			newbase[baselen + pathlen1] = 0;
 			opt->change(opt, mode1, mode2,
-				    sha1, sha2, base, path1);
+				    sha1, sha2, newbase);
+			newbase[baselen + pathlen1] = '/';
+		}
 		retval = diff_tree_sha1(sha1, sha2, newbase, opt);
 		free(newbase);
 		return retval;
 	}
 
-	opt->change(opt, mode1, mode2, sha1, sha2, base, path1);
+	fullname = malloc_fullname(base, baselen, path1, pathlen1);
+	opt->change(opt, mode1, mode2, sha1, sha2, fullname);
+	free(fullname);
 	return 0;
 }
 
@@ -205,10 +221,10 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 	unsigned mode;
 	const char *path;
 	const unsigned char *sha1 = tree_entry_extract(desc, &path, &mode);
+	int pathlen = tree_entry_len(path, sha1);
 
 	if (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode)) {
 		enum object_type type;
-		int pathlen = tree_entry_len(path, sha1);
 		char *newbase = malloc_base(base, baselen, path, pathlen);
 		struct tree_desc inner;
 		void *tree;
@@ -224,7 +240,9 @@ static void show_entry(struct diff_options *opt, const char *prefix, struct tree
 		free(tree);
 		free(newbase);
 	} else {
-		opt->add_remove(opt, prefix[0], mode, sha1, base, path);
+		char *fullname = malloc_fullname(base, baselen, path, pathlen);
+		opt->add_remove(opt, prefix[0], mode, sha1, fullname);
+		free(fullname);
 	}
 }
 
-- 
1.5.6.3.3.gfcafb

[PATCH] Fix buffer overflow in prepare_attr_stack

[Dmitry Potapov]

If PATH_MAX on your system is smaller than a path stored in the git repo,
it may cause the buffer overflow in prepare_attr_stack.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
 attr.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/attr.c b/attr.c
index 0fb47d3..73b6d6d 100644
--- a/attr.c
+++ b/attr.c
@@ -459,7 +459,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
 {
 	struct attr_stack *elem, *info;
 	int len;
-	char pathbuf[PATH_MAX];
+	struct strbuf pathbuf;
+
+	strbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));
 
 	/*
 	 * At the bottom of the attribute stack is the built-in
@@ -510,13 +512,14 @@ static void prepare_attr_stack(const char *path, int dirlen)
 			len = strlen(attr_stack->origin);
 			if (dirlen <= len)
 				break;
-			memcpy(pathbuf, path, dirlen);
-			memcpy(pathbuf + dirlen, "/", 2);
-			cp = strchr(pathbuf + len + 1, '/');
+			pathbuf.len = 0;
+			strbuf_add(&pathbuf, path, dirlen);
+			strbuf_addch(&pathbuf, '/');
+			cp = strchr(pathbuf.buf + len + 1, '/');
 			strcpy(cp + 1, GITATTRIBUTES_FILE);
-			elem = read_attr(pathbuf, 0);
+			elem = read_attr(pathbuf.buf, 0);
 			*cp = '\0';
-			elem->origin = strdup(pathbuf);
+			elem->origin = strdup(pathbuf.buf);
 			elem->prev = attr_stack;
 			attr_stack = elem;
 			debug_push(elem);
-- 
1.5.6.3.3.gfcafb

Re: [PATCH] Fix buffer overflow in prepare_attr_stack

[Johannes Sixt]

Dmitry Potapov schrieb:
> +			pathbuf.len = 0;

+			strbuf_reset(&pathbuf);

-- Hannes

[PATCH v2] Fix buffer overflow in prepare_attr_stack

[Dmitry Potapov]

If PATH_MAX on your system is smaller than a path stored in the git repo,
it may cause the buffer overflow in prepare_attr_stack.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---

On Wed, Jul 16, 2008 at 05:21:27PM +0200, Johannes Sixt wrote:
> Dmitry Potapov schrieb:
> > +			pathbuf.len = 0;
> 
> +			strbuf_reset(&pathbuf);
> 
> -- Hannes

 attr.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/attr.c b/attr.c
index 0fb47d3..17f6a4d 100644
--- a/attr.c
+++ b/attr.c
@@ -459,7 +459,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
 {
 	struct attr_stack *elem, *info;
 	int len;
-	char pathbuf[PATH_MAX];
+	struct strbuf pathbuf;
+
+	strbuf_init(&pathbuf, dirlen+2+strlen(GITATTRIBUTES_FILE));
 
 	/*
 	 * At the bottom of the attribute stack is the built-in
@@ -510,13 +512,14 @@ static void prepare_attr_stack(const char *path, int dirlen)
 			len = strlen(attr_stack->origin);
 			if (dirlen <= len)
 				break;
-			memcpy(pathbuf, path, dirlen);
-			memcpy(pathbuf + dirlen, "/", 2);
-			cp = strchr(pathbuf + len + 1, '/');
+			strbuf_reset(&pathbuf);
+			strbuf_add(&pathbuf, path, dirlen);
+			strbuf_addch(&pathbuf, '/');
+			cp = strchr(pathbuf.buf + len + 1, '/');
 			strcpy(cp + 1, GITATTRIBUTES_FILE);
-			elem = read_attr(pathbuf, 0);
+			elem = read_attr(pathbuf.buf, 0);
 			*cp = '\0';
-			elem->origin = strdup(pathbuf);
+			elem->origin = strdup(pathbuf.buf);
 			elem->prev = attr_stack;
 			attr_stack = elem;
 			debug_push(elem);
-- 
1.5.6.3.3.geccd

[PATCH v2] Fix buffer overflow in git-grep

[Dmitry Potapov]

If PATH_MAX on your system is smaller than any path stored in the git
repository, that can cause memory corruption inside of the grep_tree
function used by git-grep.

Signed-off-by: Dmitry Potapov <dpotapov@gmail.com>
---
I have converted grep_tree code to use path_buf.

 builtin-grep.c |   26 ++++++++++++++------------
 1 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index ef29910..507bb95 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -427,33 +427,35 @@ static int grep_tree(struct grep_opt *opt, const char **paths,
 	struct name_entry entry;
 	char *down;
 	int tn_len = strlen(tree_name);
-	char *path_buf = xmalloc(PATH_MAX + tn_len + 100);
+	struct strbuf pathbuf;
+
+	strbuf_init(&pathbuf, PATH_MAX + tn_len);
 
 	if (tn_len) {
-		tn_len = sprintf(path_buf, "%s:", tree_name);
-		down = path_buf + tn_len;
-		strcat(down, base);
-	}
-	else {
-		down = path_buf;
-		strcpy(down, base);
+		strbuf_add(&pathbuf, tree_name, tn_len);
+		strbuf_addch(&pathbuf, ':');
+		tn_len = pathbuf.len;
 	}
-	len = strlen(path_buf);
+	strbuf_addstr(&pathbuf, base);
+	len = pathbuf.len;
 
 	while (tree_entry(tree, &entry)) {
-		strcpy(path_buf + len, entry.path);
+		int te_len = tree_entry_len(entry.path, entry.sha1);
+		pathbuf.len = len;
+		strbuf_add(&pathbuf, entry.path, te_len);
 
 		if (S_ISDIR(entry.mode))
 			/* Match "abc/" against pathspec to
 			 * decide if we want to descend into "abc"
 			 * directory.
 			 */
-			strcpy(path_buf + len + tree_entry_len(entry.path, entry.sha1), "/");
+			strbuf_addch(&pathbuf, '/');
 
+		down = pathbuf.buf + tn_len;
 		if (!pathspec_matches(paths, down))
 			;
 		else if (S_ISREG(entry.mode))
-			hit |= grep_sha1(opt, entry.sha1, path_buf, tn_len);
+			hit |= grep_sha1(opt, entry.sha1, pathbuf.buf, tn_len);
 		else if (S_ISDIR(entry.mode)) {
 			enum object_type type;
 			struct tree_desc sub;
-- 
1.5.6.3.1.g4e6bb