Re: [PATCH] http: inform about alternates-as-redirects behavior

[Jeff King <peff@peff.net>]

On Fri, Mar 03, 2017 at 10:13:14PM -0500, Jeff King wrote:

> > -	if (http_follow_config != HTTP_FOLLOW_ALWAYS)
> > -		return;
> > -
> 
> I was surprised from the description to see not just the addition of a
> warning, but a movement of the enforcement code.
> 
> I think it's necessary because the original did not bother even fetching
> http-alternates if we were not going to respect it. Whereas the new code
> will fetch and parse it, and warn only if we actually found something in
> it. Which seems reasonable.

One side effect of this is that it exposes[1] the http-alternates
parsing code to the server's input, even if we aren't planning on using
the result. That code is not very well audited. Just looking at the
context from your patches, I noticed one obvious memory access problem.
The fix is below.

-Peff

[1] Obviously this same code was exposed prior to the redirect-limiting
    patches, so it's not like there aren't tons of older clients that
    exhibit the same behavior. But IMHO one of the beneficial side
    effects of the redirect-limiting was that it avoided this largely
    unused and untested code entirely.

-- >8 --
Subject: http-walker: fix buffer underflow processing remote alternates

If we parse a remote alternates (or http-alternates) file,
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
(and even if we did allocate, we'd just copy in gigabytes of
garbage, not overflow a buffer).

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>
---
 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.404.g442e75cca