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

From: Dmitry Potapov <dpotapov@gmail.com>
Date: Jul 16, 2008, 11:54 UTC
Message-ID: <20080716115420.GD2925@dpotapov.dyndns.org>
In-Reply-To: <alpine.DEB.1.00.0807161232110.8503@eeepc-johanness>

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