Re: [PATCH v2] http-walker: fix buffer underflow processing remote alternates
From: Junio C Hamano <hidden>
Date: 2017-03-13 17:20:17
Jeff King [off-list ref] 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.
quoted hunk
-- >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 <redacted> --- 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));