Jeff King <[email protected]> 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 <[email protected]> > --- > 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));
