{"thread":{"id":"36149","subject":"[PATCH 1/2] checkout_entry(): use the strbuf throughout the function","startedAt":"2014-03-13T09:19:06Z","lastAt":"2014-03-13T13:44:09Z","messageCount":6,"participants":["Michael Haggerty","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"236663","messageId":"1394702348-6433-1-git-send-email-mhagger@alum.mit.edu","threadId":"36149","inReplyTo":null,"subject":"[PATCH 0/2] Fix possible buffer overflow in remove_subtree()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-13T09:19:06Z","receivedAt":"2014-03-13T09:19:06Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"These patches are proposed for maint (but also apply cleanly to\nmaster).\n\nI presume that this is exploitable via Git commands, though I haven't\nverified it explicitly [1].\n\nI *think* that the rest of the file is OK.  open_output_fd() initially\nlooks suspicious, because it strcpy()s a string onto the end of its\npath argument.  But that is only done when to_tempfile is set, which\nin turn is handled consistently up the callstack up to the point where\nit is initially set in checkout_entry() if topath is not NULL.  So as\nlong as the caller obeys checkout_entry()'s docstring and passes a\nlong enough buffer for topath, I think everything is OK.  In any case,\nthe string appended in open_output_fd() is not under the control of\nthe user, so even if there were a bug in this code path it shouldn't\nbe exploitable.\n\n[1] For example, it is conceivable that there are some checks when\n    writing a tree that prevent files with such long names from being\n    written by Git.  But even if so, it is clearly a bug that could be\n    hit locally on any filesystem where PATH_MAX is not a hard limit.\n\nMichael Haggerty (2):\n  checkout_entry(): use the strbuf throughout the function\n  entry.c: fix possible buffer overflow in remove_subtree()\n\n entry.c | 64 ++++++++++++++++++++++++++++++++--------------------------------\n 1 file changed, 32 insertions(+), 32 deletions(-)\n\n-- \n1.9.0\n"},{"id":"236661","messageId":"1394702348-6433-2-git-send-email-mhagger@alum.mit.edu","threadId":"36149","inReplyTo":"1394702348-6433-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/2] checkout_entry(): use the strbuf throughout the function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-13T09:19:07Z","receivedAt":"2014-03-13T09:19:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"There is no need to break out the \"buf\" and \"len\" members into\nseparate temporary variables.  Rename path_buf to path and use\npath.buf and path.len directly.  This makes it easier to reason about\nthe data flow in the function.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n entry.c | 32 ++++++++++++++++----------------\n 1 file changed, 16 insertions(+), 16 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 7b7aa81..9adddb6 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -245,27 +245,25 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)\n int checkout_entry(struct cache_entry *ce,\n \t\t   const struct checkout *state, char *topath)\n {\n-\tstatic struct strbuf path_buf = STRBUF_INIT;\n-\tchar *path;\n+\tstatic struct strbuf path = STRBUF_INIT;\n \tstruct stat st;\n-\tint len;\n \n \tif (topath)\n \t\treturn write_entry(ce, topath, state, 1);\n \n-\tstrbuf_reset(&path_buf);\n-\tstrbuf_add(&path_buf, state->base_dir, state->base_dir_len);\n-\tstrbuf_add(&path_buf, ce->name, ce_namelen(ce));\n-\tpath = path_buf.buf;\n-\tlen = path_buf.len;\n+\tstrbuf_reset(&path);\n+\tstrbuf_add(&path, state->base_dir, state->base_dir_len);\n+\tstrbuf_add(&path, ce->name, ce_namelen(ce));\n \n-\tif (!check_path(path, len, &st, state->base_dir_len)) {\n+\tif (!check_path(path.buf, path.len, &st, state->base_dir_len)) {\n \t\tunsigned changed = ce_match_stat(ce, &st, CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE);\n \t\tif (!changed)\n \t\t\treturn 0;\n \t\tif (!state->force) {\n \t\t\tif (!state->quiet)\n-\t\t\t\tfprintf(stderr, \"%s already exists, no checkout\\n\", path);\n+\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t\"%s already exists, no checkout\\n\",\n+\t\t\t\t\tpath.buf);\n \t\t\treturn -1;\n \t\t}\n \n@@ -280,12 +278,14 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\tif (S_ISGITLINK(ce->ce_mode))\n \t\t\t\treturn 0;\n \t\t\tif (!state->force)\n-\t\t\t\treturn error(\"%s is a directory\", path);\n-\t\t\tremove_subtree(path);\n-\t\t} else if (unlink(path))\n-\t\t\treturn error(\"unable to unlink old '%s' (%s)\", path, strerror(errno));\n+\t\t\t\treturn error(\"%s is a directory\", path.buf);\n+\t\t\tremove_subtree(path.buf);\n+\t\t} else if (unlink(path.buf))\n+\t\t\treturn error(\"unable to unlink old '%s' (%s)\",\n+\t\t\t\t     path.buf, strerror(errno));\n \t} else if (state->not_new)\n \t\treturn 0;\n-\tcreate_directories(path, len, state);\n-\treturn write_entry(ce, path, state, 0);\n+\n+\tcreate_directories(path.buf, path.len, state);\n+\treturn write_entry(ce, path.buf, state, 0);\n }\n-- \n1.9.0\n"},{"id":"236662","messageId":"1394702348-6433-3-git-send-email-mhagger@alum.mit.edu","threadId":"36149","inReplyTo":"1394702348-6433-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/2] entry.c: fix possible buffer overflow in remove_subtree()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-13T09:19:08Z","receivedAt":"2014-03-13T09:19:08Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"remove_subtree() manipulated path in a fixed-size buffer even though\nthe length of the input, let alone the length of entries within the\ndirectory, were not known in advance.  Change the function to take a\nstrbuf argument and use that object as its scratch space.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\nIn\n\n    fd356f6aa8 entry.c: convert checkout_entry to use strbuf\n\nfrom last October, checkout_entry() was changed to use strbuf, but\nthis callee must have been overlooked.\n\n entry.c | 34 +++++++++++++++++-----------------\n 1 file changed, 17 insertions(+), 17 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 9adddb6..77c6882 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -44,33 +44,33 @@ static void create_directories(const char *path, int path_len,\n \tfree(buf);\n }\n \n-static void remove_subtree(const char *path)\n+static void remove_subtree(struct strbuf *path)\n {\n-\tDIR *dir = opendir(path);\n+\tDIR *dir = opendir(path->buf);\n \tstruct dirent *de;\n-\tchar pathbuf[PATH_MAX];\n-\tchar *name;\n+\tint origlen = path->len;\n \n \tif (!dir)\n-\t\tdie_errno(\"cannot opendir '%s'\", path);\n-\tstrcpy(pathbuf, path);\n-\tname = pathbuf + strlen(path);\n-\t*name++ = '/';\n+\t\tdie_errno(\"cannot opendir '%s'\", path->buf);\n \twhile ((de = readdir(dir)) != NULL) {\n \t\tstruct stat st;\n+\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n-\t\tstrcpy(name, de->d_name);\n-\t\tif (lstat(pathbuf, &st))\n-\t\t\tdie_errno(\"cannot lstat '%s'\", pathbuf);\n+\n+\t\tstrbuf_addch(path, '/');\n+\t\tstrbuf_addstr(path, de->d_name);\n+\t\tif (lstat(path->buf, &st))\n+\t\t\tdie_errno(\"cannot lstat '%s'\", path->buf);\n \t\tif (S_ISDIR(st.st_mode))\n-\t\t\tremove_subtree(pathbuf);\n-\t\telse if (unlink(pathbuf))\n-\t\t\tdie_errno(\"cannot unlink '%s'\", pathbuf);\n+\t\t\tremove_subtree(path);\n+\t\telse if (unlink(path->buf))\n+\t\t\tdie_errno(\"cannot unlink '%s'\", path->buf);\n+\t\tstrbuf_setlen(path, origlen);\n \t}\n \tclosedir(dir);\n-\tif (rmdir(path))\n-\t\tdie_errno(\"cannot rmdir '%s'\", path);\n+\tif (rmdir(path->buf))\n+\t\tdie_errno(\"cannot rmdir '%s'\", path->buf);\n }\n \n static int create_file(const char *path, unsigned int mode)\n@@ -279,7 +279,7 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\t\treturn 0;\n \t\t\tif (!state->force)\n \t\t\t\treturn error(\"%s is a directory\", path.buf);\n-\t\t\tremove_subtree(path.buf);\n+\t\t\tremove_subtree(&path);\n \t\t} else if (unlink(path.buf))\n \t\t\treturn error(\"unable to unlink old '%s' (%s)\",\n \t\t\t\t     path.buf, strerror(errno));\n-- \n1.9.0\n"},{"id":"236666","messageId":"CACsJy8AG5T4sA6j0AGtkX2uhVNDvppT=tnNrQNuZ1rhHKArP0w@mail.gmail.com","threadId":"36149","inReplyTo":"1394702348-6433-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/2] checkout_entry(): use the strbuf throughout the function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-13T11:22:31Z","receivedAt":"2014-03-13T11:22:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 13, 2014 at 4:19 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> There is no need to break out the \"buf\" and \"len\" members into\n> separate temporary variables.  Rename path_buf to path and use\n> path.buf and path.len directly.  This makes it easier to reason about\n> the data flow in the function.\n\nI wanted to keep changes to minimum when I did that. But I guess it\nhas its downside as you found out.\n-- \nDuy\n"},{"id":"236667","messageId":"CACsJy8D9FrsZ0FmXZoXGGzqQK7UfE+K39259TjUkTOZp+F5OOA@mail.gmail.com","threadId":"36149","inReplyTo":"1394702348-6433-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/2] entry.c: fix possible buffer overflow in remove_subtree()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-03-13T11:29:14Z","receivedAt":"2014-03-13T11:29:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Mar 13, 2014 at 4:19 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> remove_subtree() manipulated path in a fixed-size buffer even though\n> the length of the input, let alone the length of entries within the\n> directory, were not known in advance.  Change the function to take a\n> strbuf argument and use that object as its scratch space.\n\nConverting more PATH_MAX to strbuf could be a micro project. Not sure\nif we still any more of them though.\n\nThe patches look fine btw.\n-- \nDuy\n"},{"id":"236669","messageId":"20140313134409.GA22046@sigill.intra.peff.net","threadId":"36149","inReplyTo":"1394702348-6433-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Fix possible buffer overflow in remove_subtree()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-13T13:44:09Z","receivedAt":"2014-03-13T13:44:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 13, 2014 at 10:19:06AM +0100, Michael Haggerty wrote:\n\n> These patches are proposed for maint (but also apply cleanly to\n> master).\n> \n> I presume that this is exploitable via Git commands, though I haven't\n> verified it explicitly [1].\n\nIt's possible to overflow this buffer, like:\n\n    git init repo && cd repo &&\n    blob=$(git hash-object -w /dev/null) &&\n    big=$(perl -e 'print \"a\" x 250')\n    (for i in $(seq 1 17); do mkdir $big && cd $big; done)\n    printf \"100644 blob $blob\\t$big\\n\" >tree &&\n    tree=$(git mktree <tree) &&\n    git read-tree $tree &&\n    git checkout-index -f --all\n\nbut I'm not sure if it is easily exploitable for two reasons:\n\n  1. We never actually return from the function with the smashed stack.\n     Immediately after overflowing the buffer, we call lstat(), which\n     will fail with ENAMETOOLONG (at least on Linux), causing us to call\n     into die_errno and exit.\n\n  2. The overflow relies on us trying to move a very deep hierarchy out\n     of the way, but I could not convince git to _create_ such a\n     hierarchy in the first place. Here I do it using relative paths and\n     recursion, but git generally tries to create paths from the top of\n     the repo using full pathnames. So it gets ENAMETOOLONG trying to\n     create the paths in the first place.\n\nOf course somebody may be more clever than I am, or perhaps some systems\nhave a PATH_MAX that is not enforced by the kernel. It's still a\nsuspicious bit of code, and I think your patches are a strict\nimprovement. Besides fixing this potential problem, I notice that we\ncurrently put 4096 bytes on the stack for a recursive function. Removing\n\"a/a/a...\" can put up to 8M on the stack, which might be too much on\nsome systems (besides just being silly and wasteful).\n\n-Peff\n"}]}