{"thread":{"id":"33400","subject":"[PATCH] show-branch: use strbuf instead of static buffer","startedAt":"2013-04-05T21:15:50Z","lastAt":"2013-04-06T04:58:33Z","messageCount":3,"participants":["Jeff King","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"213304","messageId":"20130405211550.GA4880@sigill.intra.peff.net","threadId":"33400","inReplyTo":null,"subject":"[PATCH] show-branch: use strbuf instead of static buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-05T21:15:50Z","receivedAt":"2013-04-05T21:15:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we generate relative names (e.g., \"master~20^2\"), we\nformat the name into a static buffer, then xstrdup the\nresult to attach it to the commit. Since the first thing we\nadd into the static buffer is the already-computed name of\nthe child commit, the names may get longer and longer as\nthe traversal gets deeper, and we may eventually overflow\nthe fixed-size buffer.\n\nFix this by converting the fixed-size buffer into a dynamic\nstrbuf. The performance implications should be minimal, as\nwe end up allocating a heap copy of the name anyway (and now\nwe can just detach the heap copy from the strbuf).\n\nReported-by: Eric Roman \nSigned-off-by: Jeff King \n---\nThis is a fix for a bug report that came to me off-list. A real-world\nexample can be seen by running \"git show-branch --all\" on a fresh clone\nof:\n\n https://chromium.googlesource.com/chromium/src.git\n\n(but that repo is 1.7G, so I don't recommend cloning it unless you're\nreally interested). Its master branch consists of a strange sequence of\nmerges that results in naming commits like master^2^2^2^2... and so on\n(it's unclear to me why, but it looks like maybe syncing up separate svn\nand git repositories?). Which is odd, but looking at graph, I think the\nnames show-branch is generating are correct; they're just really long.\nAnd of course odd history is no excuse to overflow a buffer.\n\nThough this is a stack overflow, I don't know that it's exploitable for\nanything interesting; an attacker does not get to write arbitrary data,\nbut rather only a sequence of \"^%d\" and \"~%d\" relative history markers.\nPerhaps in theory one could devise a history such that the sequence\nmarkers spelled out some malicious code, but it would be quite a\nchallenge (and given that you have only ascii [^~0-9] to work with,\nprobably impossible).\n\nI prepared this on \"master\", but it should be suitable for \"maint\"; the\ncode dates all the way back to git v0.99.\n\n builtin/show-branch.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d208fd6..90fc6b1 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -162,29 +162,28 @@ static void name_commits(struct commit_list *list,\n \t\t\tnth = 0;\n \t\t\twhile (parents) {\n \t\t\t\tstruct commit *p = parents->item;\n-\t\t\t\tchar newname[1000], *en;\n+\t\t\t\tstruct strbuf newname = STRBUF_INIT;\n \t\t\t\tparents = parents->next;\n \t\t\t\tnth++;\n \t\t\t\tif (p->util)\n \t\t\t\t\tcontinue;\n-\t\t\t\ten = newname;\n \t\t\t\tswitch (n->generation) {\n \t\t\t\tcase 0:\n-\t\t\t\t\ten += sprintf(en, \"%s\", n->head_name);\n+\t\t\t\t\tstrbuf_addstr(&newname, n->head_name);\n \t\t\t\t\tbreak;\n \t\t\t\tcase 1:\n-\t\t\t\t\ten += sprintf(en, \"%s^\", n->head_name);\n+\t\t\t\t\tstrbuf_addf(&newname, \"%s^\", n->head_name);\n \t\t\t\t\tbreak;\n \t\t\t\tdefault:\n-\t\t\t\t\ten += sprintf(en, \"%s~%d\",\n-\t\t\t\t\t\tn->head_name, n->generation);\n+\t\t\t\t\tstrbuf_addf(&newname, \"%s~%d\",\n+\t\t\t\t\t\t n->head_name, n->generation);\n \t\t\t\t\tbreak;\n \t\t\t\t}\n \t\t\t\tif (nth == 1)\n-\t\t\t\t\ten += sprintf(en, \"^\");\n+\t\t\t\t\tstrbuf_addch(&newname, '^');\n \t\t\t\telse\n-\t\t\t\t\ten += sprintf(en, \"^%d\", nth);\n-\t\t\t\tname_commit(p, xstrdup(newname), 0);\n+\t\t\t\t\tstrbuf_addf(&newname, \"^%d\", nth);\n+\t\t\t\tname_commit(p, strbuf_detach(&newname, NULL), 0);\n \t\t\t\ti++;\n \t\t\t\tname_first_parent_chain(p);\n \t\t\t}\n-- \n1.8.2.rc0.33.gd915649\n"},{"id":"213350","messageId":"20130405234915.GU30308@google.com","threadId":"33400","inReplyTo":"20130405211550.GA4880@sigill.intra.peff.net","subject":"Re: [PATCH] show-branch: use strbuf instead of static buffer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-04-05T23:49:15Z","receivedAt":"2013-04-05T23:49:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> When we generate relative names (e.g., \"master~20^2\"), we\n> format the name into a static buffer, then xstrdup the\n> result to attach it to the commit. Since the first thing we\n> add into the static buffer is the already-computed name of\n> the child commit, the names may get longer and longer as\n> the traversal gets deeper, and we may eventually overflow\n> the fixed-size buffer.\n\nGood catch.\n\n[...]\n> Though this is a stack overflow, I don't know that it's exploitable for\n> anything interesting; an attacker does not get to write arbitrary data,\n> but rather only a sequence of \"^%d\" and \"~%d\" relative history markers.\n> Perhaps in theory one could devise a history such that the sequence\n> markers spelled out some malicious code, but it would be quite a\n> challenge\n\nOverwrite the return address and return-to-libc?\n\n[...]\n> --- a/builtin/show-branch.c\n> +++ b/builtin/show-branch.c\n\nVery clean and obviously correct. Thanks.\n\nReviewed-by: Jonathan Nieder \n\nA test would be nice, though.\n\nHope that helps,\nJonathan\n"},{"id":"213313","messageId":"20130406045833.GB26544@sigill.intra.peff.net","threadId":"33400","inReplyTo":"20130405234915.GU30308@google.com","subject":"Re: [PATCH] show-branch: use strbuf instead of static buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-04-06T04:58:33Z","receivedAt":"2013-04-06T04:58:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2013 at 04:49:15PM -0700, Jonathan Nieder wrote:\n\n> > Though this is a stack overflow, I don't know that it's exploitable for\n> > anything interesting; an attacker does not get to write arbitrary data,\n> > but rather only a sequence of \"^%d\" and \"~%d\" relative history markers.\n> > Perhaps in theory one could devise a history such that the sequence\n> > markers spelled out some malicious code, but it would be quite a\n> > challenge\n> \n> Overwrite the return address and return-to-libc?\n\nStill hard, since you need to construct a usable address (and arguments)\nout of sequences of \"^[0-9]+\" and \"~[0-9]+\". But I'd love to see a\nworking exploit if somebody thinks they can do it. :)\n\n> Very clean and obviously correct. Thanks.\n> \n> Reviewed-by: Jonathan Nieder \n\nThanks.\n\n> A test would be nice, though.\n\nWhat should it be testing? That a giant chain of second-parent merges\nthat exceeds 1000 bytes doesn't segfault? Tests like that are not all\nthat interesting, because they do not catch real-world regressions. We\nhave closed this barn door; it is not impossible that it will be\nre-opened, but it is not likely. A test that checks only for a very\nspecific type of failure is only ever going to see that failure.\n\nIf you want to design a suite of tests that check that show-branch gives\ncorrect output for particular brands of large repo, that would be\ngeneric and potentially useful. But I don't think it's actually worth\nspending a lot of time on (reviewing the code for more static buffers\nand sprintfs would probably be a much more fruitful use of time).\n\n-Peff\n"}]}