patchhttp-walker: fix buffer underflow processing remote alternates

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

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

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

> > 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?
> 
> Good point. I think that behavior is a misfeature in the first place. It
> should be leaving the path as-is and tacking "info/packs", etc.
> 
> But without fixing that, yeah, there is not much value in the "maybe
> strip" behavior (unless you happen to provide the incorrect path that
> does not include "objects" in the first place, but then it would not
> work as a _local_ alternate).
> 
> > 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?
> 
> Yeah, probably warn and ignore is the best bet. I'll re-work the patch.

> but we do not add back in the "/" when making requests. So technically:
>
>   ../some-path/my-objects
>
> works now, and would not if we were more picky. I doubt anybody cares,
> but I went for the minimal behavior change here. If anybody wants to get
> more fancy, the correct path is to leave the path intact here and teach
> the appending side to stop re-adding "objects".

> -- >8 --
> Subject: http-walker: fix buffer underflow processing remote alternates
>
> 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 generall 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. If they're not present, we'll ignore the
> alternate (in theory we could use it as-is, but the rest of
> the http-walker code unconditionally tacks "objects/" back
> on, so it is it not prepared to handle such a case).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  http-walker.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/http-walker.c b/http-walker.c
> index b34b6ace7..507c200f0 100644
> --- a/http-walker.c
> +++ b/http-walker.c
> @@ -296,13 +296,16 @@ 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);
> -
> -				if (is_alternate_allowed(target.buf)) {
> +				strbuf_add(&target, data + i, posn - i);
> +				if (!strbuf_strip_suffix(&target, "objects")) {
> +					warning("ignoring alternate that does"
> +						" not end in 'objects': %s",
> +						target.buf);
> +					strbuf_release(&target);
> +				} else if (is_alternate_allowed(target.buf)) {
>  					warning("adding alternate object store: %s",
>  						target.buf);
>  					newalt = xmalloc(sizeof(*newalt));