[PATCH 0/2] Fix possible buffer overflow in remove_subtree()

[PATCH 0/2] Fix possible buffer overflow in remove_subtree()

[Michael Haggerty]

These patches are proposed for maint (but also apply cleanly to
master).

I presume that this is exploitable via Git commands, though I haven't
verified it explicitly [1].

I *think* that the rest of the file is OK.  open_output_fd() initially
looks suspicious, because it strcpy()s a string onto the end of its
path argument.  But that is only done when to_tempfile is set, which
in turn is handled consistently up the callstack up to the point where
it is initially set in checkout_entry() if topath is not NULL.  So as
long as the caller obeys checkout_entry()'s docstring and passes a
long enough buffer for topath, I think everything is OK.  In any case,
the string appended in open_output_fd() is not under the control of
the user, so even if there were a bug in this code path it shouldn't
be exploitable.

[1] For example, it is conceivable that there are some checks when
    writing a tree that prevent files with such long names from being
    written by Git.  But even if so, it is clearly a bug that could be
    hit locally on any filesystem where PATH_MAX is not a hard limit.

Michael Haggerty (2):
  checkout_entry(): use the strbuf throughout the function
  entry.c: fix possible buffer overflow in remove_subtree()

 entry.c | 64 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 32 insertions(+), 32 deletions(-)

-- 
1.9.0

[PATCH 1/2] checkout_entry(): use the strbuf throughout the function

[Michael Haggerty]

There is no need to break out the "buf" and "len" members into
separate temporary variables.  Rename path_buf to path and use
path.buf and path.len directly.  This makes it easier to reason about
the data flow in the function.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 entry.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/entry.c b/entry.c
index 7b7aa81..9adddb6 100644
--- a/entry.c
+++ b/entry.c
@@ -245,27 +245,25 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)
 int checkout_entry(struct cache_entry *ce,
 		   const struct checkout *state, char *topath)
 {
-	static struct strbuf path_buf = STRBUF_INIT;
-	char *path;
+	static struct strbuf path = STRBUF_INIT;
 	struct stat st;
-	int len;
 
 	if (topath)
 		return write_entry(ce, topath, state, 1);
 
-	strbuf_reset(&path_buf);
-	strbuf_add(&path_buf, state->base_dir, state->base_dir_len);
-	strbuf_add(&path_buf, ce->name, ce_namelen(ce));
-	path = path_buf.buf;
-	len = path_buf.len;
+	strbuf_reset(&path);
+	strbuf_add(&path, state->base_dir, state->base_dir_len);
+	strbuf_add(&path, ce->name, ce_namelen(ce));
 
-	if (!check_path(path, len, &st, state->base_dir_len)) {
+	if (!check_path(path.buf, path.len, &st, state->base_dir_len)) {
 		unsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);
 		if (!changed)
 			return 0;
 		if (!state->force) {
 			if (!state->quiet)
-				fprintf(stderr, "%s already exists, no checkout\n", path);
+				fprintf(stderr,
+					"%s already exists, no checkout\n",
+					path.buf);
 			return -1;
 		}
 
@@ -280,12 +278,14 @@ int checkout_entry(struct cache_entry *ce,
 			if (S_ISGITLINK(ce->ce_mode))
 				return 0;
 			if (!state->force)
-				return error("%s is a directory", path);
-			remove_subtree(path);
-		} else if (unlink(path))
-			return error("unable to unlink old '%s' (%s)", path, strerror(errno));
+				return error("%s is a directory", path.buf);
+			remove_subtree(path.buf);
+		} else if (unlink(path.buf))
+			return error("unable to unlink old '%s' (%s)",
+				     path.buf, strerror(errno));
 	} else if (state->not_new)
 		return 0;
-	create_directories(path, len, state);
-	return write_entry(ce, path, state, 0);
+
+	create_directories(path.buf, path.len, state);
+	return write_entry(ce, path.buf, state, 0);
 }
-- 
1.9.0

Re: [PATCH 1/2] checkout_entry(): use the strbuf throughout the function

[Duy Nguyen]

On Thu, Mar 13, 2014 at 4:19 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> There is no need to break out the "buf" and "len" members into
> separate temporary variables.  Rename path_buf to path and use
> path.buf and path.len directly.  This makes it easier to reason about
> the data flow in the function.

I wanted to keep changes to minimum when I did that. But I guess it
has its downside as you found out.
-- 
Duy

[PATCH 2/2] entry.c: fix possible buffer overflow in remove_subtree()

[Michael Haggerty]

remove_subtree() manipulated path in a fixed-size buffer even though
the length of the input, let alone the length of entries within the
directory, were not known in advance.  Change the function to take a
strbuf argument and use that object as its scratch space.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
In

    fd356f6aa8 entry.c: convert checkout_entry to use strbuf

from last October, checkout_entry() was changed to use strbuf, but
this callee must have been overlooked.

 entry.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/entry.c b/entry.c
index 9adddb6..77c6882 100644
--- a/entry.c
+++ b/entry.c
@@ -44,33 +44,33 @@ static void create_directories(const char *path, int path_len,
 	free(buf);
 }
 
-static void remove_subtree(const char *path)
+static void remove_subtree(struct strbuf *path)
 {
-	DIR *dir = opendir(path);
+	DIR *dir = opendir(path->buf);
 	struct dirent *de;
-	char pathbuf[PATH_MAX];
-	char *name;
+	int origlen = path->len;
 
 	if (!dir)
-		die_errno("cannot opendir '%s'", path);
-	strcpy(pathbuf, path);
-	name = pathbuf + strlen(path);
-	*name++ = '/';
+		die_errno("cannot opendir '%s'", path->buf);
 	while ((de = readdir(dir)) != NULL) {
 		struct stat st;
+
 		if (is_dot_or_dotdot(de->d_name))
 			continue;
-		strcpy(name, de->d_name);
-		if (lstat(pathbuf, &st))
-			die_errno("cannot lstat '%s'", pathbuf);
+
+		strbuf_addch(path, '/');
+		strbuf_addstr(path, de->d_name);
+		if (lstat(path->buf, &st))
+			die_errno("cannot lstat '%s'", path->buf);
 		if (S_ISDIR(st.st_mode))
-			remove_subtree(pathbuf);
-		else if (unlink(pathbuf))
-			die_errno("cannot unlink '%s'", pathbuf);
+			remove_subtree(path);
+		else if (unlink(path->buf))
+			die_errno("cannot unlink '%s'", path->buf);
+		strbuf_setlen(path, origlen);
 	}
 	closedir(dir);
-	if (rmdir(path))
-		die_errno("cannot rmdir '%s'", path);
+	if (rmdir(path->buf))
+		die_errno("cannot rmdir '%s'", path->buf);
 }
 
 static int create_file(const char *path, unsigned int mode)
@@ -279,7 +279,7 @@ int checkout_entry(struct cache_entry *ce,
 				return 0;
 			if (!state->force)
 				return error("%s is a directory", path.buf);
-			remove_subtree(path.buf);
+			remove_subtree(&path);
 		} else if (unlink(path.buf))
 			return error("unable to unlink old '%s' (%s)",
 				     path.buf, strerror(errno));
-- 
1.9.0

Re: [PATCH 2/2] entry.c: fix possible buffer overflow in remove_subtree()

[Duy Nguyen]

On Thu, Mar 13, 2014 at 4:19 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> remove_subtree() manipulated path in a fixed-size buffer even though
> the length of the input, let alone the length of entries within the
> directory, were not known in advance.  Change the function to take a
> strbuf argument and use that object as its scratch space.

Converting more PATH_MAX to strbuf could be a micro project. Not sure
if we still any more of them though.

The patches look fine btw.
-- 
Duy

Re: [PATCH 0/2] Fix possible buffer overflow in remove_subtree()

[Jeff King]

On Thu, Mar 13, 2014 at 10:19:06AM +0100, Michael Haggerty wrote:

> These patches are proposed for maint (but also apply cleanly to
> master).
> 
> I presume that this is exploitable via Git commands, though I haven't
> verified it explicitly [1].

It's possible to overflow this buffer, like:

    git init repo && cd repo &&
    blob=$(git hash-object -w /dev/null) &&
    big=$(perl -e 'print "a" x 250')
    (for i in $(seq 1 17); do mkdir $big && cd $big; done)
    printf "100644 blob $blob\t$big\n" >tree &&
    tree=$(git mktree <tree) &&
    git read-tree $tree &&
    git checkout-index -f --all

but I'm not sure if it is easily exploitable for two reasons:

  1. We never actually return from the function with the smashed stack.
     Immediately after overflowing the buffer, we call lstat(), which
     will fail with ENAMETOOLONG (at least on Linux), causing us to call
     into die_errno and exit.

  2. The overflow relies on us trying to move a very deep hierarchy out
     of the way, but I could not convince git to _create_ such a
     hierarchy in the first place. Here I do it using relative paths and
     recursion, but git generally tries to create paths from the top of
     the repo using full pathnames. So it gets ENAMETOOLONG trying to
     create the paths in the first place.

Of course somebody may be more clever than I am, or perhaps some systems
have a PATH_MAX that is not enforced by the kernel. It's still a
suspicious bit of code, and I think your patches are a strict
improvement. Besides fixing this potential problem, I notice that we
currently put 4096 bytes on the stack for a recursive function. Removing
"a/a/a..." can put up to 8M on the stack, which might be too much on
some systems (besides just being silly and wasteful).

-Peff