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

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

[Jeff King]

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

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

[Jeff King]

On Sun, Mar 12, 2017 at 08:38:53AM -0400, Jeff King wrote:

> I posted this last week in the middle of another thread[1], but it
> didn't get any attention. So here it is again.

Oops, that [1] should be:

  http://public-inbox.org/git/20170304034914.cgyvz735lxhe2cno@sigill.intra.peff.net/

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

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

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

[Jeff King]

On Sun, Mar 12, 2017 at 10:59:09PM -0700, Junio C Hamano wrote:

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

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.

-Peff

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

[Jeff King]

On Mon, Mar 13, 2017 at 09:50:53AM -0400, Jeff King wrote:

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

OK, so here's v2 that just rejects things that don't end in "objects".
For the expected case, it would be more robust to look for "/objects",
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));
-- 
2.12.0.518.gfbf68a9d3

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yeah.  I admit that I didn't spend too much time last night auditing
the code but I didn't find anything that adds things like "info/refs"
that results in a URL outside the original "/objects", so I agree
that "don't strip and don't re-add 'objects'" would be the right way
to go if anybody cares deeply enough.  Let's leave that for others
and take this one.

Thanks.

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