threads / patch / 36149

patch, 2 partscheckout_entry(): use the strbuf throughout the function

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

## tl;dr

6 messages between Mar 13, 2014 and Mar 13, 2014. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Michael Haggerty· Mar 13, 2014, 09:19 UTC · lore

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

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
Michael Haggerty· Mar 13, 2014, 09:19 UTC · re: Michael Haggerty · lore

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(-)
Show changes to entry.c +16 −16
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
Duy Nguyen· Mar 13, 2014, 11:22 UTC · re: Michael Haggerty · lore

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

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
Michael Haggerty· Mar 13, 2014, 09:19 UTC · re: Michael Haggerty · lore

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

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(-)
Show changes to entry.c +17 −17
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
Duy Nguyen· Mar 13, 2014, 11:29 UTC · re: Michael Haggerty · lore

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

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
Jeff King· Mar 13, 2014, 13:44 UTC · re: Michael Haggerty · lore

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

On Thu, Mar 13, 2014 at 10:19:06AM +0100, Michael Haggerty wrote:
Show 5 quoted lines
> 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

← back to recent threads