{"thread":{"id":"65625","subject":"[PATCH] http: handle absolute-path alternates from server root","startedAt":"2026-05-12T16:26:21Z","lastAt":"2026-05-22T04:55:05Z","messageCount":7,"participants":["Jeff King","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"543192","messageId":"20260512162619.GA69813@coredump.intra.peff.net","threadId":"65625","inReplyTo":null,"subject":"[PATCH] http: handle absolute-path alternates from server root","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-12T16:26:19Z","receivedAt":"2026-05-12T16:26:21Z","isPatch":true,"body":"When a dumb http server reports alternates with an absolute path, we try\nto paste that onto the root of the URL we're trying to fetch from. So if\nwe go to \"http://example.com/path/to/child.git\" and it tells us about an\nalternate at \"/parent.git\", we'll hit \"http://example.com/parent.git\".\n\nBut there's a bug in computing the base when the URL does not have any\npath component at all, like \"http://example.com\". When looking for the\nfirst slash after the host, strchr() returns NULL, and we compute a\nnonsense value for the length of the host portion. And then when we use\nthat length to copy the base of the URL into a strbuf, we're likely to\nfail.\n\nThe security implications are minimal here. We store the nonsense length\n(\"serverlen\") as an int, so on a 64-bit system it may effectively be\nanything (it is zero minus a 64-bit heap pointer, then truncated to\n32-bits and stuffed into a signed value). When we feed that length to\nstrbuf_add(), it is cast into a size_t and one of four things will\nhappen:\n\n  1. If serverlen was negative, it will turn into a very large positive\n     value and strbuf_add() will fail to allocate, ending the program.\n     Ditto if serverlen was positive but just very large.\n\n     This doesn't really get an attacker anything; the victim will just\n     fail to clone their evil repo.\n\n  2. If serverlen was small enough, we'll successfully extend the target\n     strbuf, and then copy an arbitrary set of bytes from \"base\". And\n     then one of these is true:\n\n       a. That set of bytes is much larger than the length of the \"base\"\n          string. This is an out-of-bounds read, but there's no\n          out-of-bounds write, since the strbuf code both allocates and\n          copies using the same size_t. This is likely to cause a\n          segfault as we try to read unmapped pages of memory.\n\n       b. Like (2a), but if the set of bytes is small enough we might\n          not segfault. We might read random memory from the process and\n          copy it into the \"target\" strbuf.\n\n          What happens then? We know that \"base\" ends with a NUL\n          terminator, which will be copied into \"target\" as well. So\n          even though target.len might be 1000 bytes (or whatever), when\n          interpreted as a NUL-terminated string, target.buf is still\n          the exact same string as \"base\".\n\n          And that's all we ever do with target: pass it around as a C\n          string, and then eventually strbuf_detach() it to become a C\n          string. So even though there was arbitrary memory copied into\n          the strbuf, we never access it.\n\n       c. The other interesting case is when serverlen is actually\n          _shorter_ than the length of base. And there we truncate the\n          string. Probably in a way that makes it totally invalid, but\n          if you were very unlucky you could turn something like:\n\n             http://victim.com.evil.domain:8000\n\n          into:\n\n            http://victim.com\n\n\t  Which looks like the start of a redirect attack, except that\n\t  the attacker could just have written \"http://victim.com\" in\n\t  the first place! Either way we feed it to\n\t  is_alternate_allowed(), which is where we check redirect and\n\t  protocol rules.\n\nI think we can just treat this like a regular bug.\n\nAnd it's quite a weird setup in the first place, as it implies that the\nroot of the web server is serving a repository (i.e., that you can get\nsomething useful from \"http://example.com/info/refs\"). The bug has been\nthere since b3661567cf ([PATCH] Add support for alternates in HTTP,\n2005-09-14) without anybody noticing.\n\nI kind of doubt anybody really cares about making this work, but it's\neasy enough to do so: the host-portion of the URL ends at either the\nfirst slash or the end-of-string. So we can just replace strchr() with\nstrchrnul().\n\nThe test setup is a little gross, as we take over the httpd document\nroot by shoving our bare-repo components into it. But it demonstrates\nthe problem and shows that our solution actually allows the alternate to\nfunction, if the server is configured to allow it.\n\nReported-by: slonkazoid <slonkazoid@slonk.ing>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n http-walker.c              |  2 +-\n t/t5550-http-fetch-dumb.sh | 20 ++++++++++++++++++++\n 2 files changed, 21 insertions(+), 1 deletion(-)\n\ndiff --git a/http-walker.c b/http-walker.c\nindex 1b6d496548..f252de089f 100644\n--- a/http-walker.c\n+++ b/http-walker.c\n@@ -268,7 +268,7 @@ static void process_alternates_response(void *callback_data)\n \t\t\t\t */\n \t\t\t\tconst char *colon_ss = strstr(base,\"://\");\n \t\t\t\tif (colon_ss) {\n-\t\t\t\t\tserverlen = (strchr(colon_ss + 3, '/')\n+\t\t\t\t\tserverlen = (strchrnul(colon_ss + 3, '/')\n \t\t\t\t\t\t     - base);\n \t\t\t\t\tokay = 1;\n \t\t\t\t}\ndiff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh\nindex 9d0a7f5c4b..b0080bf204 100755\n--- a/t/t5550-http-fetch-dumb.sh\n+++ b/t/t5550-http-fetch-dumb.sh\n@@ -555,4 +555,24 @@ test_expect_success 'dumb http can fetch index v1' '\n \tgit -C idx-v1 fsck\n '\n \n+test_expect_success 'absolute-path alternate when url has no path' '\n+\tsrc=$HTTPD_DOCUMENT_ROOT_PATH/repo.git &&\n+\talt=absolute-alt.git &&\n+\tgit clone --bare --shared \"$src\" \"$alt\" &&\n+\n+\t# Our repo has an alternate pointing to the absolute filesystem path,\n+\t# but that will not make any sense to an http client. So we will\n+\t# manually give it the equivalent path that the http server will\n+\t# understand.\n+\techo \"/dumb/repo.git/objects\" >\"$alt/objects/info/http-alternates\" &&\n+\n+\t# Now make our alt repository available at the root of the http\n+\t# server without any path (i.e., just http://localhost:1234).\n+\tgit -C \"$alt\" update-server-info &&\n+\tmv absolute-alt.git/* \"$HTTPD_DOCUMENT_ROOT_PATH\" &&\n+\n+\tgit -c http.followRedirects=true clone \"$HTTPD_URL\" alt-clone.git 2>err &&\n+\ttest_grep \"adding alternate object store: $HTTPD_URL/dumb/repo.git\" err\n+'\n+\n test_done\n-- \n2.54.0.420.gf0bcdff42b\n"},{"id":"543224","messageId":"xmqqo6ikjeqp.fsf@gitster.g","threadId":"65625","inReplyTo":"20260512162619.GA69813@coredump.intra.peff.net","subject":"Re: [PATCH] http: handle absolute-path alternates from server root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-05-13T01:10:54Z","receivedAt":"2026-05-13T01:10:57Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n>           ... Probably in a way that makes it totally invalid, but\n>           if you were very unlucky you could turn something like:\n>\n>              http://victim.com.evil.domain:8000\n>\n>           into:\n>\n>             http://victim.com\n>\n> \t  Which looks like the start of a redirect attack, except that\n> \t  the attacker could just have written \"http://victim.com\" in\n> \t  the first place! Either way we feed it to\n> \t  is_alternate_allowed(), which is where we check redirect and\n> \t  protocol rules.\n\nYuck.  I know I am the guilty party who introduced the dumb HTTP\nwalker but I wish we could kill it off after all these years. I did\nnot even recall that we supported the alternate object store in the\n\"protocol\" until I saw this patch X-<.\n\n> I think we can just treat this like a regular bug.\n\nAbsolutely.  Thanks.\n"},{"id":"543261","messageId":"20260513185825.GB147423@coredump.intra.peff.net","threadId":"65625","inReplyTo":"xmqqo6ikjeqp.fsf@gitster.g","subject":"Re: [PATCH] http: handle absolute-path alternates from server root","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-13T18:58:25Z","receivedAt":"2026-05-13T18:58:27Z","isPatch":true,"body":"On Wed, May 13, 2026 at 10:10:54AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >           ... Probably in a way that makes it totally invalid, but\n> >           if you were very unlucky you could turn something like:\n> >\n> >              http://victim.com.evil.domain:8000\n> >\n> >           into:\n> >\n> >             http://victim.com\n> >\n> > \t  Which looks like the start of a redirect attack, except that\n> > \t  the attacker could just have written \"http://victim.com\" in\n> > \t  the first place! Either way we feed it to\n> > \t  is_alternate_allowed(), which is where we check redirect and\n> > \t  protocol rules.\n> \n> Yuck.  I know I am the guilty party who introduced the dumb HTTP\n> walker but I wish we could kill it off after all these years. I did\n> not even recall that we supported the alternate object store in the\n> \"protocol\" until I saw this patch X-<.\n\nMe too. It's been the source of many obscure bugs, and I think a couple\nof vulnerabilities (even though clients never intend to use dumb clones\nin the first place).\n\nWe talked about dropping it a few years ago, but Eric countered that\ndumb clones are easier on the server in some cases (like gigantic\npublic-inbox repos that are packed to keep most of the old history in\none big pack that is never updated). The verbatim pack-reuse feature\ntries to get smart clones closer to that, but it's hard to beat serving\na static file from the server's perspective. I haven't measured anything\nin that area in a while, though.\n\n-Peff\n"},{"id":"543382","messageId":"agbOEsZ8NmE8SyfV@pks.im","threadId":"65625","inReplyTo":"20260513185825.GB147423@coredump.intra.peff.net","subject":"Re: [PATCH] http: handle absolute-path alternates from server root","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-15T07:41:06Z","receivedAt":"2026-05-15T07:41:12Z","isPatch":true,"body":"On Wed, May 13, 2026 at 02:58:25PM -0400, Jeff King wrote:\n> On Wed, May 13, 2026 at 10:10:54AM +0900, Junio C Hamano wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> > \n> > >           ... Probably in a way that makes it totally invalid, but\n> > >           if you were very unlucky you could turn something like:\n> > >\n> > >              http://victim.com.evil.domain:8000\n> > >\n> > >           into:\n> > >\n> > >             http://victim.com\n> > >\n> > > \t  Which looks like the start of a redirect attack, except that\n> > > \t  the attacker could just have written \"http://victim.com\" in\n> > > \t  the first place! Either way we feed it to\n> > > \t  is_alternate_allowed(), which is where we check redirect and\n> > > \t  protocol rules.\n> > \n> > Yuck.  I know I am the guilty party who introduced the dumb HTTP\n> > walker but I wish we could kill it off after all these years. I did\n> > not even recall that we supported the alternate object store in the\n> > \"protocol\" until I saw this patch X-<.\n> \n> Me too. It's been the source of many obscure bugs, and I think a couple\n> of vulnerabilities (even though clients never intend to use dumb clones\n> in the first place).\n> \n> We talked about dropping it a few years ago, but Eric countered that\n> dumb clones are easier on the server in some cases (like gigantic\n> public-inbox repos that are packed to keep most of the old history in\n> one big pack that is never updated). The verbatim pack-reuse feature\n> tries to get smart clones closer to that, but it's hard to beat serving\n> a static file from the server's perspective. I haven't measured anything\n> in that area in a while, though.\n\nIn theory we can get much closer with packfile URIs, too, can't we? If\nthe packfiles are directly accessible anyway the server could just\nannounce these directly and have the client fetch them. That should\nsignificantly reduce the load on the server even further.\n\nOf course, the big downside is that \"fetch.uriProtocols\" is empty by\ndefault, so Git will not use them. Makes me wonder whether this is\nsomething we want to eventually change, but I guess the current default\nbehaviour is somewhat insecure as it would allow the server to redirect\nclients to arbitrary locations. It would be great if we had a mechanism\nthat only allowed packfile URIs that use the same host, which would make\nthis a lot more reasonable to enable by default.\n\nPatrick\n"},{"id":"543418","messageId":"20260515170134.GC88375@coredump.intra.peff.net","threadId":"65625","inReplyTo":"agbOEsZ8NmE8SyfV@pks.im","subject":"Re: [PATCH] http: handle absolute-path alternates from server root","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-15T17:01:34Z","receivedAt":"2026-05-15T17:01:35Z","isPatch":true,"body":"On Fri, May 15, 2026 at 09:41:06AM +0200, Patrick Steinhardt wrote:\n\n> > We talked about dropping it a few years ago, but Eric countered that\n> > dumb clones are easier on the server in some cases (like gigantic\n> > public-inbox repos that are packed to keep most of the old history in\n> > one big pack that is never updated). The verbatim pack-reuse feature\n> > tries to get smart clones closer to that, but it's hard to beat serving\n> > a static file from the server's perspective. I haven't measured anything\n> > in that area in a while, though.\n> \n> In theory we can get much closer with packfile URIs, too, can't we? If\n> the packfiles are directly accessible anyway the server could just\n> announce these directly and have the client fetch them. That should\n> significantly reduce the load on the server even further.\n\nPackfile URIs help with the actual pack generation (even if we're\nblitting out bits from the disk with verbatim packfile reuse, we still\nhave to handle gaps and compute the checksum over the output pack).\n\nBut it doesn't help with the server computing the set of objects the\nclient needs in the first place. IIRC, packfile URIs work by the server\nsaying \"oh, I was going to send you object XYZ, but you can get it from\nthis stable pack instead\". So the server still has to compute the set of\nobjects (and send any that are not mentioned in URI packs). Bitmaps\nhelp, but there's still non-trivial computation and storage on the\nserver.\n\nContrast that with a client that instead pulls a packfile over dumb\nstorage on its own, and then comes to the server for a top-off fetch.\nThe server still has to do some computation, but it's usually quite\nsmall, because both sides agree quickly that there's no need to dig down\nfurther than the tips in that dumb packfile.\n\n> Of course, the big downside is that \"fetch.uriProtocols\" is empty by\n> default, so Git will not use them. Makes me wonder whether this is\n> something we want to eventually change, but I guess the current default\n> behaviour is somewhat insecure as it would allow the server to redirect\n> clients to arbitrary locations. It would be great if we had a mechanism\n> that only allowed packfile URIs that use the same host, which would make\n> this a lot more reasonable to enable by default.\n\nIt's been a while since I've looked at it, but I seem to recall that the\nserver-side tools for specifying which packfile URIs to use were not\nthat mature. Maybe that has changed, though (I'm probably 5 years out of\ndate since the last time I really thought about these things).\n\n-Peff\n"},{"id":"543822","messageId":"ag7xbkTF11N22waX@pks.im","threadId":"65625","inReplyTo":"20260515170134.GC88375@coredump.intra.peff.net","subject":"Re: [PATCH] http: handle absolute-path alternates from server root","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-05-21T11:50:06Z","receivedAt":"2026-05-21T11:50:12Z","isPatch":true,"body":"On Fri, May 15, 2026 at 01:01:34PM -0400, Jeff King wrote:\n> On Fri, May 15, 2026 at 09:41:06AM +0200, Patrick Steinhardt wrote:\n> \n> > > We talked about dropping it a few years ago, but Eric countered that\n> > > dumb clones are easier on the server in some cases (like gigantic\n> > > public-inbox repos that are packed to keep most of the old history in\n> > > one big pack that is never updated). The verbatim pack-reuse feature\n> > > tries to get smart clones closer to that, but it's hard to beat serving\n> > > a static file from the server's perspective. I haven't measured anything\n> > > in that area in a while, though.\n> > \n> > In theory we can get much closer with packfile URIs, too, can't we? If\n> > the packfiles are directly accessible anyway the server could just\n> > announce these directly and have the client fetch them. That should\n> > significantly reduce the load on the server even further.\n> \n> Packfile URIs help with the actual pack generation (even if we're\n> blitting out bits from the disk with verbatim packfile reuse, we still\n> have to handle gaps and compute the checksum over the output pack).\n> \n> But it doesn't help with the server computing the set of objects the\n> client needs in the first place. IIRC, packfile URIs work by the server\n> saying \"oh, I was going to send you object XYZ, but you can get it from\n> this stable pack instead\". So the server still has to compute the set of\n> objects (and send any that are not mentioned in URI packs). Bitmaps\n> help, but there's still non-trivial computation and storage on the\n> server.\n\nI guess it depends on the actual server-side implementation, but in the\ngeneral case this is of course true. A server could decide to for\nexample overserve objects in case the client does a full clone, or it\ncould arrange packfiles in a special way that allows it to serve at\nleast some kinds of requests efficiently.\n\n> Contrast that with a client that instead pulls a packfile over dumb\n> storage on its own, and then comes to the server for a top-off fetch.\n> The server still has to do some computation, but it's usually quite\n> small, because both sides agree quickly that there's no need to dig down\n> further than the tips in that dumb packfile.\n\nSo this here is in theory possible with packfile URIs, as well, by\ncomputing the top-off fetch depending on the packfile layout.\n\nBut this requires quite a bunch of server-side logic and very specific\nlayouts, I guess.\n\n> > Of course, the big downside is that \"fetch.uriProtocols\" is empty by\n> > default, so Git will not use them. Makes me wonder whether this is\n> > something we want to eventually change, but I guess the current default\n> > behaviour is somewhat insecure as it would allow the server to redirect\n> > clients to arbitrary locations. It would be great if we had a mechanism\n> > that only allowed packfile URIs that use the same host, which would make\n> > this a lot more reasonable to enable by default.\n> \n> It's been a while since I've looked at it, but I seem to recall that the\n> server-side tools for specifying which packfile URIs to use were not\n> that mature. Maybe that has changed, though (I'm probably 5 years out of\n> date since the last time I really thought about these things).\n\nPackfile URIs definitely need some love to become feasible, yes, and I\ndon't think they have evolved much since their introduction. I still\nfeel like they are the better mechanism for offloading traffic compared\nto bundle URIs though, as we already have packfiles around anyway.\n\nPatrick\n"},{"id":"543878","messageId":"20260522045503.GB861761@coredump.intra.peff.net","threadId":"65625","inReplyTo":"ag7xbkTF11N22waX@pks.im","subject":"Re: [PATCH] http: handle absolute-path alternates from server root","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-05-22T04:55:03Z","receivedAt":"2026-05-22T04:55:05Z","isPatch":true,"body":"On Thu, May 21, 2026 at 01:50:06PM +0200, Patrick Steinhardt wrote:\n\n> > Packfile URIs help with the actual pack generation (even if we're\n> > blitting out bits from the disk with verbatim packfile reuse, we still\n> > have to handle gaps and compute the checksum over the output pack).\n> > \n> > But it doesn't help with the server computing the set of objects the\n> > client needs in the first place. IIRC, packfile URIs work by the server\n> > saying \"oh, I was going to send you object XYZ, but you can get it from\n> > this stable pack instead\". So the server still has to compute the set of\n> > objects (and send any that are not mentioned in URI packs). Bitmaps\n> > help, but there's still non-trivial computation and storage on the\n> > server.\n> \n> I guess it depends on the actual server-side implementation, but in the\n> general case this is of course true. A server could decide to for\n> example overserve objects in case the client does a full clone, or it\n> could arrange packfiles in a special way that allows it to serve at\n> least some kinds of requests efficiently.\n\nTrue, though you still have to receive the client wants/haves before\ngetting to the packfile-uri phase. The alternative is for the server\nsend URIs during the ref advertisement. But we have that, too, these\ndays: the bundle-uri feature. (Which I completely forgot about while\nwriting my earlier email).\n\nSo I do think that bundle-uris can probably be an adequate substitute\nfor dumb-http in terms of reducing server load. Though...\n\n> Packfile URIs definitely need some love to become feasible, yes, and I\n> don't think they have evolved much since their introduction. I still\n> feel like they are the better mechanism for offloading traffic compared\n> to bundle URIs though, as we already have packfiles around anyway.\n\n...yeah, I agree that storing both bundles and packs can be annoying for\na server, depending on your setup. In theory it would not be hard for a\nslightly-clever server endpoint to store packfiles for regular Git to\nuse, and then generate the bundles on the fly by cat-ing the bundle\nheader and the packfile, both of which can be sent out as raw bytes\nwithout further processing.\n\nAnyway, we are far afield from the patch that started this thread. ;) I\ndo agree with the general notion that we _should_ be able to get\nsmart-http close to the server-side expense of dumb-http with a few\ntricks like these.\n\n-Peff\n"}]}