[PATCH] http-walker: fix buffer underflow processing remote alternates

From: Jeff King <peff@peff.net>
Date: Mar 12, 2017, 12:38 UTC
Message-ID: <20170312123853.55ehe2jurozb6zad@sigill.intra.peff.net>

If we parse a remote alternates (or http-alternates), we expect relative lines like:

  ../../foo.git/objects

which we convert into "$URL/../foo.git/" (and then use that as a base for fetching more objects).

But if the remote feeds us nonsense like just:

../

we will try to blindly strip the last 7 characters, assuming they contain the string "objects". Since we don't _have_ 7 characters at all, this results in feeding a small negative value to strbuf_add(), which converts it to a size_t, resulting in a big positive value. This should consistently fail (since we can't generally allocate the max size_t minus 7 bytes), so there shouldn't be any security implications.

Let's fix this by using strbuf_strip_suffix() to drop the characters we want. As a bonus this lets us handle names that do not end in "objects" (all git repos do, but there is nothing to say that an alternate object store needs to be a git repo).

And while we're here, we can add a few other parsing niceties, like dropping trailing whitespace, and handling names that do not end in "/".

Signed-off-by: Jeff King <peff@peff.net>
---
I posted this last week in the middle of another thread[1], but it
didn't get any attention. So here it is again.

I admit I don't care at all about http-alternates, but potential out-of-range errors worry me.

 http-walker.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/http-walker.c b/http-walker.c
index b34b6ace7..d62ca8953 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -296,11 +296,13 @@ static void process_alternates_response(void *callback_data)
 					okay = 1;
 				}
 			}
-			/* skip "objects\n" at end */
 			if (okay) {
 				struct strbuf target = STRBUF_INIT;
 				strbuf_add(&target, base, serverlen);
-				strbuf_add(&target, data + i, posn - i - 7);
+				strbuf_add(&target, data + i, posn - i);
+				strbuf_rtrim(&target);
+				strbuf_strip_suffix(&target, "objects");
+				strbuf_complete(&target, '/');
 
 				if (is_alternate_allowed(target.buf)) {
 					warning("adding alternate object store: %s",

-- 
2.12.0.518.gfbf68a9d3