{"thread":{"id":"50542","subject":"[PATCH] receive-pack: fix use-after-free bug","startedAt":"2019-02-20T00:00:51Z","lastAt":"2019-02-20T00:00:51Z","messageCount":1,"participants":["Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"369682","messageId":"20190220000033.357-1-avarab@gmail.com","threadId":"50542","inReplyTo":null,"subject":"[PATCH] receive-pack: fix use-after-free bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-20T00:00:33Z","receivedAt":"2019-02-20T00:00:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The resolve_ref_unsafe() function can, and sometimes will in the case\nof this codepath, return the char * passed to it to the caller. In\nthis case we construct a strbuf, free it, and then continue using the\ndst_name after that free().\n\nThe code being fixed dates back to da3efdb17b (\"receive-pack: detect\naliased updates which can occur with symrefs\", 2010-04-19). When it\nwas originally added it didn't have this bug, it was introduced when\nit was subsequently modified to use strbuf in 6b01ecfe22 (\"ref\nnamespaces: Support remote repositories via upload-pack and\nreceive-pack\", 2011-07-08).\n\nThis is theoretically a security issue, the C standard makes no\nguarantees that a value you use after free() hasn't been poked at or\nchanged by something else on the system, but in practice modern OSs\nwill have mapped the relevant page to this process, so nothing else\nwould have used it. We do no further allocations between the free()\nand use-after-free, so we ourselves didn't corrupt or change the\nvalue.\n\nJeff investigated that and found: \"It probably would be an issue if\nthe allocation were larger. glibc at least will use mmap()/munmap()\nafter some cutoff[1], in which case we'd get a segfault from hitting\nthe unmapped page. But for small allocations, it just bumps brk() and\nthe memory is still available for further allocations after\nfree(). [...] If you had a sufficiently large refname you might be\nable to trigger the bug [...]. I tried to push such a ref. I had to\nmanually make a packed-refs file with the long name to avoid\nfilesystem limits (though probably you could have a long a/b/c/ name\non ext4).  But the result can't actually be pushed, because it all has\nto fit into a 64k pkt-line as part of the push protocol.\".\n\nAn a alternative and more succinct way of implementing this would have\nbeen to do the strbuf_release() at the end of check_aliased_update()\nand use \"goto out\" instead of the early \"return\" statements. Hopefully\nthis approach of using a helper instead makes it easier to follow.\n\n1. Jeff: \"Weirdly, the mmap() cutoff on my glibc system is 135168\n   bytes. Which is...2^17 + 2^12? 33 pages? I'm sure there's a good\n   reason for that, but I didn't dig into it.\"\n\nReported-by: 王健强 <jianqiang.wang@securitygossip.com>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nThis fixes an issue reported by 王健强 to git-security@. As noted in\nthe commit message the consensus was that the use-after-free wasn't\nexploitable in practice, and that the patch should therefore be\ndiscussed on the main list.\n\nThe bug first appeared in v1.7.7, so it's not a 2.21 rc issue, as\nmight be inferred from the timing of this patch.\n\n builtin/receive-pack.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex d58b7750b6..0874f5c1b7 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1198,17 +1198,12 @@ static void run_update_post_hook(struct command *commands)\n \t}\n }\n \n-static void check_aliased_update(struct command *cmd, struct string_list *list)\n+static void check_aliased_update_internal(struct command *cmd,\n+\t\t\t\t\t  struct string_list *list,\n+\t\t\t\t\t  const char *dst_name, int flag)\n {\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tconst char *dst_name;\n \tstruct string_list_item *item;\n \tstruct command *dst_cmd;\n-\tint flag;\n-\n-\tstrbuf_addf(&buf, \"%s%s\", get_git_namespace(), cmd->ref_name);\n-\tdst_name = resolve_ref_unsafe(buf.buf, 0, NULL, &flag);\n-\tstrbuf_release(&buf);\n \n \tif (!(flag & REF_ISSYMREF))\n \t\treturn;\n@@ -1247,6 +1242,18 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \t\t\"inconsistent aliased update\";\n }\n \n+static void check_aliased_update(struct command *cmd, struct string_list *list)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tconst char *dst_name;\n+\tint flag;\n+\n+\tstrbuf_addf(&buf, \"%s%s\", get_git_namespace(), cmd->ref_name);\n+\tdst_name = resolve_ref_unsafe(buf.buf, 0, NULL, &flag);\n+\tcheck_aliased_update_internal(cmd, list, dst_name, flag);\n+\tstrbuf_release(&buf);\n+}\n+\n static void check_aliased_updates(struct command *commands)\n {\n \tstruct command *cmd;\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"}]}