git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 6/6] tree-walk: harden make_traverse_path() length computations
Date: Wed, 31 Jul 2019 00:38:25 -0400	[thread overview]
Message-ID: <20190731043825.GF27170@sigill.intra.peff.net> (raw)
In-Reply-To: <20190731043659.GA27028@sigill.intra.peff.net>

The make_traverse_path() function isn't very careful about checking its
output buffer boundaries. In fact, it doesn't even _know_ the size of
the buffer it's writing to, and just assumes that the caller used
traverse_path_len() correctly. And even then we assume that our
traverse_info.pathlen components are all correct, and just blindly write
into the buffer.

Let's improve this situation a bit:

  - have the caller pass in their allocated buffer length, which we'll
    check against our own computations

  - check for integer underflow as we do our backwards-insertion of
    pathnames into the buffer

  - check that we do not run out items in our list to traverse before
    we've filled the expected number of bytes

None of these should be triggerable in practice (especially since our
switch to size_t everywhere in a previous commit), but it doesn't hurt
to check our assumptions.

Signed-off-by: Jeff King <peff@peff.net>
---
 tree-walk.c    | 28 ++++++++++++++++++++--------
 tree-walk.h    |  2 +-
 unpack-trees.c |  3 ++-
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/tree-walk.c b/tree-walk.c
index 313780f40c..bea819d826 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -183,21 +183,32 @@ void setup_traverse_info(struct traverse_info *info, const char *base)
 		info->prev = &dummy;
 }
 
-char *make_traverse_path(char *path, const struct traverse_info *info,
+char *make_traverse_path(char *path, size_t pathlen,
+			 const struct traverse_info *info,
 			 const char *name, size_t namelen)
 {
-	size_t pathlen = info->pathlen;
+	/* Always points to the end of the name we're about to add */
+	size_t pos = st_add(info->pathlen, namelen);
 
-	path[pathlen + namelen] = 0;
+	if (pos >= pathlen)
+		BUG("too small buffer passed to make_traverse_path");
+
+	path[pos] = 0;
 	for (;;) {
-		memcpy(path + pathlen, name, namelen);
-		if (!pathlen)
+		if (pos < namelen)
+			BUG("traverse_info pathlen does not match strings");
+		pos -= namelen;
+		memcpy(path + pos, name, namelen);
+
+		if (!pos)
 			break;
-		path[--pathlen] = '/';
+		path[--pos] = '/';
+
+		if (!info)
+			BUG("traverse_info ran out of list items");
 		name = info->name;
 		namelen = info->namelen;
 		info = info->prev;
-		pathlen -= namelen;
 	}
 	return path;
 }
@@ -209,7 +220,8 @@ void strbuf_make_traverse_path(struct strbuf *out,
 	size_t len = traverse_path_len(info, namelen);
 
 	strbuf_grow(out, len);
-	make_traverse_path(out->buf + out->len, info, name, namelen);
+	make_traverse_path(out->buf + out->len, out->alloc - out->len,
+			   info, name, namelen);
 	strbuf_setlen(out, out->len + len);
 }
 
diff --git a/tree-walk.h b/tree-walk.h
index 43b56fd8ff..abe2caf4e0 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -72,7 +72,7 @@ struct traverse_info {
 };
 
 int get_tree_entry(struct repository *, const struct object_id *, const char *, struct object_id *, unsigned short *);
-char *make_traverse_path(char *path, const struct traverse_info *info,
+char *make_traverse_path(char *path, size_t pathlen, const struct traverse_info *info,
 			 const char *name, size_t namelen);
 void strbuf_make_traverse_path(struct strbuf *out,
 			       const struct traverse_info *info,
diff --git a/unpack-trees.c b/unpack-trees.c
index b154f09547..50f257bd5c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -968,7 +968,8 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info,
 	ce->ce_flags = create_ce_flags(stage);
 	ce->ce_namelen = len;
 	oidcpy(&ce->oid, &n->oid);
-	make_traverse_path(ce->name, info, n->path, n->pathlen);
+	/* len+1 because the cache_entry allocates space for NUL */
+	make_traverse_path(ce->name, len + 1, info, n->path, n->pathlen);
 
 	return ce;
 }
-- 
2.23.0.rc0.426.gbdee707ba7

      parent reply	other threads:[~2019-07-31  4:38 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-31  4:37 [PATCH 0/6] harden tree-walking against integer overflow Jeff King
2019-07-31  4:38 ` [PATCH 1/6] setup_traverse_info(): stop copying oid Jeff King
2019-07-31  4:38 ` [PATCH 2/6] tree-walk: drop oid from traverse_info Jeff King
2019-07-31  4:38 ` [PATCH 3/6] tree-walk: use size_t consistently Jeff King
2019-08-01 18:17   ` Derrick Stolee
2019-07-31  4:38 ` [PATCH 4/6] tree-walk: accept a raw length for traverse_path_len() Jeff King
2019-07-31  4:38 ` [PATCH 5/6] tree-walk: add a strbuf wrapper for make_traverse_path() Jeff King
2019-07-31  4:38 ` Jeff King [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190731043825.GF27170@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --subject='Re: [PATCH 6/6] tree-walk: harden make_traverse_path() length computations' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).