{"thread":{"id":"45360","subject":"[PATCH] http-walker: fix buffer underflow processing remote alternates","startedAt":"2017-03-12T12:39:42Z","lastAt":"2017-03-13T17:20:17Z","messageCount":6,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313855","messageId":"20170312123853.55ehe2jurozb6zad@sigill.intra.peff.net","threadId":"45360","inReplyTo":null,"subject":"[PATCH] http-walker: fix buffer underflow processing remote alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T12:38:54Z","receivedAt":"2017-03-12T12:39:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we parse a remote alternates (or http-alternates), we\nexpect relative lines like:\n\n  ../../foo.git/objects\n\nwhich we convert into \"$URL/../foo.git/\" (and then use that\nas a base for fetching more objects).\n\nBut if the remote feeds us nonsense like just:\n\n  ../\n\nwe will try to blindly strip the last 7 characters, assuming\nthey contain the string \"objects\". Since we don't _have_ 7\ncharacters at all, this results in feeding a small negative\nvalue to strbuf_add(), which converts it to a size_t,\nresulting in a big positive value. This should consistently\nfail (since we can't generally allocate the max size_t minus\n7 bytes), so there shouldn't be any security implications.\n\nLet's fix this by using strbuf_strip_suffix() to drop the\ncharacters we want. As a bonus this lets us handle names\nthat do not end in \"objects\" (all git repos do, but there is\nnothing to say that an alternate object store needs to be a\ngit repo).\n\nAnd while we're here, we can add a few other parsing\nniceties, like dropping trailing whitespace, and handling\nnames that do not end in \"/\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI posted this last week in the middle of another thread[1], but it\ndidn't get any attention. So here it is again.\n\nI admit I don't care at all about http-alternates, but potential\nout-of-range errors worry me.\n\n http-walker.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex b34b6ace7..d62ca8953 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -296,11 +296,13 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t\tokay = 1;\n \t\t\t\t}\n \t\t\t}\n-\t\t\t/* skip \"objects\\n\" at end */\n \t\t\tif (okay) {\n \t\t\t\tstruct strbuf target = STRBUF_INIT;\n \t\t\t\tstrbuf_add(&target, base, serverlen);\n-\t\t\t\tstrbuf_add(&target, data + i, posn - i - 7);\n+\t\t\t\tstrbuf_add(&target, data + i, posn - i);\n+\t\t\t\tstrbuf_rtrim(&target);\n+\t\t\t\tstrbuf_strip_suffix(&target, \"objects\");\n+\t\t\t\tstrbuf_complete(&target, '/');\n \n \t\t\t\tif (is_alternate_allowed(target.buf)) {\n \t\t\t\t\twarning(\"adding alternate object store: %s\",\n-- \n2.12.0.518.gfbf68a9d3\n"},{"id":"313856","messageId":"20170312123944.2de33av45aqvgrdr@sigill.intra.peff.net","threadId":"45360","inReplyTo":"20170312123853.55ehe2jurozb6zad@sigill.intra.peff.net","subject":"Re: [PATCH] http-walker: fix buffer underflow processing remote alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T12:39:44Z","receivedAt":"2017-03-12T12:39:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 12, 2017 at 08:38:53AM -0400, Jeff King wrote:\n\n> I posted this last week in the middle of another thread[1], but it\n> didn't get any attention. So here it is again.\n\nOops, that [1] should be:\n\n  http://public-inbox.org/git/20170304034914.cgyvz735lxhe2cno@sigill.intra.peff.net/\n\n-Peff\n"},{"id":"313875","messageId":"xmqqfuihu382.fsf@gitster.mtv.corp.google.com","threadId":"45360","inReplyTo":"20170312123853.55ehe2jurozb6zad@sigill.intra.peff.net","subject":"Re: [PATCH] http-walker: fix buffer underflow processing remote alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T05:59:09Z","receivedAt":"2017-03-13T05:59:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> If we parse a remote alternates (or http-alternates), we\n> expect relative lines like:\n>\n>   ../../foo.git/objects\n>\n> which we convert into \"$URL/../foo.git/\" (and then use that\n> as a base for fetching more objects).\n>\n> But if the remote feeds us nonsense like just:\n>\n>   ../\n>\n> we will try to blindly strip the last 7 characters, assuming\n> they contain the string \"objects\". Since we don't _have_ 7\n> characters at all, this results in feeding a small negative\n> value to strbuf_add(), which converts it to a size_t,\n> resulting in a big positive value. This should consistently\n> fail (since we can't generally allocate the max size_t minus\n> 7 bytes), so there shouldn't be any security implications.\n\nOK.\n\n> Let's fix this by using strbuf_strip_suffix() to drop the\n> characters we want. As a bonus this lets us handle names\n> that do not end in \"objects\" (all git repos do, but there is\n> nothing to say that an alternate object store needs to be a\n> git repo).\n\nHmph.\n\nIsn't the reason why the function wants to strip \"objects\" at the\nend because it then expects to be able to tack strings like\n\"objects/info/packs\", etc. after the result of stripping, i.e.\n$URL/../foo.git/, and get usable URLs to download other things?\n\nI think it is very sensible to use strip_suffix() to notice that the\nalternate does not end with \"/objects\", but I am not sure if it is a\ngood idea to proceed without stripping when it does not end with\n\"/objects\".  Shouldn't we be ignoring (with warning, possibly) such\na remote alternate?\n\n>  \t\t\t}\n> -\t\t\t/* skip \"objects\\n\" at end */\n>  \t\t\tif (okay) {\n>  \t\t\t\tstruct strbuf target = STRBUF_INIT;\n>  \t\t\t\tstrbuf_add(&target, base, serverlen);\n> -\t\t\t\tstrbuf_add(&target, data + i, posn - i - 7);\n> +\t\t\t\tstrbuf_add(&target, data + i, posn - i);\n> +\t\t\t\tstrbuf_rtrim(&target);\n> +\t\t\t\tstrbuf_strip_suffix(&target, \"objects\");\n> +\t\t\t\tstrbuf_complete(&target, '/');\n>  \n>  \t\t\t\tif (is_alternate_allowed(target.buf)) {\n>  \t\t\t\t\twarning(\"adding alternate object store: %s\",\n"},{"id":"313890","messageId":"20170313135053.3pi56dfdlcva66ml@sigill.intra.peff.net","threadId":"45360","inReplyTo":"xmqqfuihu382.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] http-walker: fix buffer underflow processing remote alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-13T13:50:53Z","receivedAt":"2017-03-13T13:51:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 12, 2017 at 10:59:09PM -0700, Junio C Hamano wrote:\n\n> > Let's fix this by using strbuf_strip_suffix() to drop the\n> > characters we want. As a bonus this lets us handle names\n> > that do not end in \"objects\" (all git repos do, but there is\n> > nothing to say that an alternate object store needs to be a\n> > git repo).\n> \n> Hmph.\n> \n> Isn't the reason why the function wants to strip \"objects\" at the\n> end because it then expects to be able to tack strings like\n> \"objects/info/packs\", etc. after the result of stripping, i.e.\n> $URL/../foo.git/, and get usable URLs to download other things?\n\nGood point. I think that behavior is a misfeature in the first place. It\nshould be leaving the path as-is and tacking \"info/packs\", etc.\n\nBut without fixing that, yeah, there is not much value in the \"maybe\nstrip\" behavior (unless you happen to provide the incorrect path that\ndoes not include \"objects\" in the first place, but then it would not\nwork as a _local_ alternate).\n\n> I think it is very sensible to use strip_suffix() to notice that the\n> alternate does not end with \"/objects\", but I am not sure if it is a\n> good idea to proceed without stripping when it does not end with\n> \"/objects\".  Shouldn't we be ignoring (with warning, possibly) such\n> a remote alternate?\n\nYeah, probably warn and ignore is the best bet. I'll re-work the patch.\n\n-Peff\n"},{"id":"313891","messageId":"20170313140445.bjdxnjylirn3e5z3@sigill.intra.peff.net","threadId":"45360","inReplyTo":"20170313135053.3pi56dfdlcva66ml@sigill.intra.peff.net","subject":"[PATCH v2] http-walker: fix buffer underflow processing remote alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-13T14:04:45Z","receivedAt":"2017-03-13T14:04:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 13, 2017 at 09:50:53AM -0400, Jeff King wrote:\n\n> > Isn't the reason why the function wants to strip \"objects\" at the\n> > end because it then expects to be able to tack strings like\n> > \"objects/info/packs\", etc. after the result of stripping, i.e.\n> > $URL/../foo.git/, and get usable URLs to download other things?\n> \n> Good point. I think that behavior is a misfeature in the first place. It\n> should be leaving the path as-is and tacking \"info/packs\", etc.\n> \n> But without fixing that, yeah, there is not much value in the \"maybe\n> strip\" behavior (unless you happen to provide the incorrect path that\n> does not include \"objects\" in the first place, but then it would not\n> work as a _local_ alternate).\n> \n> > I think it is very sensible to use strip_suffix() to notice that the\n> > alternate does not end with \"/objects\", but I am not sure if it is a\n> > good idea to proceed without stripping when it does not end with\n> > \"/objects\".  Shouldn't we be ignoring (with warning, possibly) such\n> > a remote alternate?\n> \n> Yeah, probably warn and ignore is the best bet. I'll re-work the patch.\n\nOK, so here's v2 that just rejects things that don't end in \"objects\".\nFor the expected case, it would be more robust to look for \"/objects\",\nbut we do not add back in the \"/\" when making requests. So technically:\n\n  ../some-path/my-objects\n\nworks now, and would not if we were more picky. I doubt anybody cares,\nbut I went for the minimal behavior change here. If anybody wants to get\nmore fancy, the correct path is to leave the path intact here and teach\nthe appending side to stop re-adding \"objects\".\n\n-- >8 --\nSubject: http-walker: fix buffer underflow processing remote alternates\n\nIf we parse a remote alternates (or http-alternates), we\nexpect relative lines like:\n\n  ../../foo.git/objects\n\nwhich we convert into \"$URL/../foo.git/\" (and then use that\nas a base for fetching more objects).\n\nBut if the remote feeds us nonsense like just:\n\n  ../\n\nwe will try to blindly strip the last 7 characters, assuming\nthey contain the string \"objects\". Since we don't _have_ 7\ncharacters at all, this results in feeding a small negative\nvalue to strbuf_add(), which converts it to a size_t,\nresulting in a big positive value. This should consistently\nfail (since we can't generall allocate the max size_t minus\n7 bytes), so there shouldn't be any security implications.\n\nLet's fix this by using strbuf_strip_suffix() to drop the\ncharacters we want. If they're not present, we'll ignore the\nalternate (in theory we could use it as-is, but the rest of\nthe http-walker code unconditionally tacks \"objects/\" back\non, so it is it not prepared to handle such a case).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-walker.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex b34b6ace7..507c200f0 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -296,13 +296,16 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t\tokay = 1;\n \t\t\t\t}\n \t\t\t}\n-\t\t\t/* skip \"objects\\n\" at end */\n \t\t\tif (okay) {\n \t\t\t\tstruct strbuf target = STRBUF_INIT;\n \t\t\t\tstrbuf_add(&target, base, serverlen);\n-\t\t\t\tstrbuf_add(&target, data + i, posn - i - 7);\n-\n-\t\t\t\tif (is_alternate_allowed(target.buf)) {\n+\t\t\t\tstrbuf_add(&target, data + i, posn - i);\n+\t\t\t\tif (!strbuf_strip_suffix(&target, \"objects\")) {\n+\t\t\t\t\twarning(\"ignoring alternate that does\"\n+\t\t\t\t\t\t\" not end in 'objects': %s\",\n+\t\t\t\t\t\ttarget.buf);\n+\t\t\t\t\tstrbuf_release(&target);\n+\t\t\t\t} else if (is_alternate_allowed(target.buf)) {\n \t\t\t\t\twarning(\"adding alternate object store: %s\",\n \t\t\t\t\t\ttarget.buf);\n \t\t\t\t\tnewalt = xmalloc(sizeof(*newalt));\n-- \n2.12.0.518.gfbf68a9d3\n\n"},{"id":"313899","messageId":"xmqq7f3tt7p3.fsf@gitster.mtv.corp.google.com","threadId":"45360","inReplyTo":"20170313140445.bjdxnjylirn3e5z3@sigill.intra.peff.net","subject":"Re: [PATCH v2] http-walker: fix buffer underflow processing remote alternates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-13T17:20:08Z","receivedAt":"2017-03-13T17:20:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> but we do not add back in the \"/\" when making requests. So technically:\n>\n>   ../some-path/my-objects\n>\n> works now, and would not if we were more picky. I doubt anybody cares,\n> but I went for the minimal behavior change here. If anybody wants to get\n> more fancy, the correct path is to leave the path intact here and teach\n> the appending side to stop re-adding \"objects\".\n\nYeah.  I admit that I didn't spend too much time last night auditing\nthe code but I didn't find anything that adds things like \"info/refs\"\nthat results in a URL outside the original \"/objects\", so I agree\nthat \"don't strip and don't re-add 'objects'\" would be the right way\nto go if anybody cares deeply enough.  Let's leave that for others\nand take this one.\n\nThanks.\n\n> -- >8 --\n> Subject: http-walker: fix buffer underflow processing remote alternates\n>\n> If we parse a remote alternates (or http-alternates), we\n> expect relative lines like:\n>\n>   ../../foo.git/objects\n>\n> which we convert into \"$URL/../foo.git/\" (and then use that\n> as a base for fetching more objects).\n>\n> But if the remote feeds us nonsense like just:\n>\n>   ../\n>\n> we will try to blindly strip the last 7 characters, assuming\n> they contain the string \"objects\". Since we don't _have_ 7\n> characters at all, this results in feeding a small negative\n> value to strbuf_add(), which converts it to a size_t,\n> resulting in a big positive value. This should consistently\n> fail (since we can't generall allocate the max size_t minus\n> 7 bytes), so there shouldn't be any security implications.\n>\n> Let's fix this by using strbuf_strip_suffix() to drop the\n> characters we want. If they're not present, we'll ignore the\n> alternate (in theory we could use it as-is, but the rest of\n> the http-walker code unconditionally tacks \"objects/\" back\n> on, so it is it not prepared to handle such a case).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  http-walker.c | 11 +++++++----\n>  1 file changed, 7 insertions(+), 4 deletions(-)\n>\n> diff --git a/http-walker.c b/http-walker.c\n> index b34b6ace7..507c200f0 100644\n> --- a/http-walker.c\n> +++ b/http-walker.c\n> @@ -296,13 +296,16 @@ static void process_alternates_response(void *callback_data)\n>  \t\t\t\t\tokay = 1;\n>  \t\t\t\t}\n>  \t\t\t}\n> -\t\t\t/* skip \"objects\\n\" at end */\n>  \t\t\tif (okay) {\n>  \t\t\t\tstruct strbuf target = STRBUF_INIT;\n>  \t\t\t\tstrbuf_add(&target, base, serverlen);\n> -\t\t\t\tstrbuf_add(&target, data + i, posn - i - 7);\n> -\n> -\t\t\t\tif (is_alternate_allowed(target.buf)) {\n> +\t\t\t\tstrbuf_add(&target, data + i, posn - i);\n> +\t\t\t\tif (!strbuf_strip_suffix(&target, \"objects\")) {\n> +\t\t\t\t\twarning(\"ignoring alternate that does\"\n> +\t\t\t\t\t\t\" not end in 'objects': %s\",\n> +\t\t\t\t\t\ttarget.buf);\n> +\t\t\t\t\tstrbuf_release(&target);\n> +\t\t\t\t} else if (is_alternate_allowed(target.buf)) {\n>  \t\t\t\t\twarning(\"adding alternate object store: %s\",\n>  \t\t\t\t\t\ttarget.buf);\n>  \t\t\t\t\tnewalt = xmalloc(sizeof(*newalt));\n"}]}