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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 13, 2017, 05:59 UTC
Message-ID: <xmqqfuihu382.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170312123853.55ehe2jurozb6zad@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 19 quoted lines

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

OK.

Show 5 quoted lines

> 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).

Hmph.

Isn't the reason why the function wants to strip "objects" at the end because it then expects to be able to tack strings like "objects/info/packs", etc. after the result of stripping, i.e. $URL/../foo.git/, and get usable URLs to download other things?

I think it is very sensible to use strip_suffix() to notice that the alternate does not end with "/objects", but I am not sure if it is a good idea to proceed without stripping when it does not end with "/objects". Shouldn't we be ignoring (with warning, possibly) such a remote alternate?

Show 13 quoted lines

>  			}
> -			/* 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",