threads / patch / 14479

patchFix buffer overflow in git-grep

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

## tl;dr

10 messages between Jul 16, 2008 and Jul 16, 2008. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Dmitry Potapov· Jul 16, 2008, 10:15 UTC · lore

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(-)
Show changes to builtin-grep.c +5 −2
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
Johannes Schindelin· Jul 16, 2008, 10:35 UTC · re: Dmitry Potapov · lore

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

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*
Show 13 quoted lines
> 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

Dmitry Potapov· Jul 16, 2008, 11:54 UTC · re: Johannes Schindelin · lore

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

On Wed, Jul 16, 2008 at 12:35:06PM +0200, Johannes Schindelin wrote:
Show 11 quoted lines
> >  
> >  	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
Dmitry Potapov· Jul 16, 2008, 14:33 UTC · re: Dmitry Potapov · lore

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

On Wed, Jul 16, 2008 at 3:54 PM, Dmitry Potapov <dpotapov@gmail.com> wrote:
Show 7 quoted lines
>
> 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
Johannes Schindelin· Jul 16, 2008, 14:47 UTC · re: Dmitry Potapov · lore

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

Hi,
On Wed, 16 Jul 2008, Dmitry Potapov wrote:
Show 13 quoted lines
> 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

Dmitry Potapov· Jul 16, 2008, 14:54 UTC · re: Dmitry Potapov · lore

[PATCH] Fix buffer overflow in git diff

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(-)
Show changes to 5 files +34 −24

diff-lib.c, diff.c, diff.h, revision.c, tree-diff.c

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
Dmitry Potapov· Jul 16, 2008, 14:54 UTC · re: Dmitry Potapov · lore

[PATCH] Fix buffer overflow in prepare_attr_stack

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(-)
Show changes to attr.c +9 −6
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
Dmitry Potapov· Jul 16, 2008, 15:39 UTC · re: Johannes Sixt · lore

[PATCH v2] Fix buffer overflow in prepare_attr_stack

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:
Show 6 quoted lines
> Dmitry Potapov schrieb:
> > +			pathbuf.len = 0;
> 
> +			strbuf_reset(&pathbuf);
> 
> -- Hannes
 attr.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)
Show changes to attr.c +9 −6
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
Dmitry Potapov· Jul 16, 2008, 15:33 UTC · re: Johannes Schindelin · lore

[PATCH v2] Fix buffer overflow in git-grep

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(-)
Show changes to builtin-grep.c +14 −12
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

← back to recent threads