{"thread":{"id":"27485","subject":"[PATCH maint 0/3] do not write files outside of work-dir","startedAt":"2011-05-27T16:00:37Z","lastAt":"2011-06-07T19:32:48Z","messageCount":20,"participants":["Erik Faye-Lund","Junio C Hamano","Johannes Schindelin","Johannes Sixt","Theo Niessink","Tait"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"168877","messageId":"1306512040-1468-1-git-send-email-kusmabite@gmail.com","threadId":"27485","inReplyTo":null,"subject":"[PATCH maint 0/3] do not write files outside of work-dir","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-05-27T16:00:37Z","receivedAt":"2011-05-27T16:00:37Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Theo Niessink has uncovered a serious sercurity issue in Git for Windows,\nwhere cloning an evil repository can arbitrarily overwrite files outside\nthe repository. Since many Windows users run as administrators, this can\nbe used for very nasty purposes.\n\nThe first two patches fix \"git add\" so it reject paths outside of the\nrepository when specified in the \"C:\\...\"-form on Windows.\n\nPatch 3/3 makes sure we don't try to actually write to these files.\n\nThis series applies cleanly to 'maint', and I strongly encourage that\nwe apply at the very least 3/3 there.\n\nErik Faye-Lund (1):\n verify_path: consider dos drive prefix\n\nTheo Niessink (2):\n A Windows path starting with a backslash is absolute\n real_path: do not assume '/' is the path seperator\n\n abspath.c | 4 ++--\n cache.h | 2 +-\n compat/mingw.h | 9 +++++++++\n git-compat-util.h | 4 ++++\n read-cache.c | 5 ++++-\n 5 files changed, 20 insertions(+), 4 deletions(-)\n\n-- \n1.7.5.3.3.g435ff\n"},{"id":"168878","messageId":"1306512040-1468-2-git-send-email-kusmabite@gmail.com","threadId":"27485","inReplyTo":"1306512040-1468-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH 1/3] A Windows path starting with a backslash is absolute","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-05-27T16:00:38Z","receivedAt":"2011-05-27T16:00:38Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Theo Niessink \n\nThis fixes prefix_path() not recognizing e.g. \\foo\\bar as an absolute path\non Windows.\n\nSigned-off-by: Theo Niessink \nSigned-off-by: Erik Faye-Lund \n---\n cache.h | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex dd34fed..555bf7f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -734,7 +734,7 @@ extern char *expand_user_path(const char *path);\n char *enter_repo(char *path, int strict);\n static inline int is_absolute_path(const char *path)\n {\n-\treturn path[0] == '/' || has_dos_drive_prefix(path);\n+\treturn is_dir_sep(path[0]) || has_dos_drive_prefix(path);\n }\n int is_directory(const char *);\n const char *real_path(const char *path);\n-- \n1.7.5.3.3.g435ff\n"},{"id":"168879","messageId":"1306512040-1468-3-git-send-email-kusmabite@gmail.com","threadId":"27485","inReplyTo":"1306512040-1468-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH 2/3] real_path: do not assume '/' is the path seperator","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-05-27T16:00:39Z","receivedAt":"2011-05-27T16:00:39Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"From: Theo Niessink \n\nreal_path currently assumes it's input had '/' as path seperator.\nThis assumption does not hold true for the code-path from\nprefix_path (on Windows), where real_path can be called before\nnormalize_path_copy.\n\nFix real_path so it doesn't make this assumption. Create a helper\nfunction to reverse-search for the last path-seperator in a string.\n\nSigned-off-by: Theo Niessink \nSigned-off-by: Erik Faye-Lund \n---\n abspath.c | 4 ++--\n compat/mingw.h | 9 +++++++++\n git-compat-util.h | 4 ++++\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex 3005aed..01858eb 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -40,7 +40,7 @@ const char *real_path(const char *path)\n \n \twhile (depth--) {\n \t\tif (!is_directory(buf)) {\n-\t\t\tchar *last_slash = strrchr(buf, '/');\n+\t\t\tchar *last_slash = find_last_dir_sep(buf);\n \t\t\tif (last_slash) {\n \t\t\t\t*last_slash = '\\0';\n \t\t\t\tlast_elem = xstrdup(last_slash + 1);\n@@ -65,7 +65,7 @@ const char *real_path(const char *path)\n \t\t\tif (len + strlen(last_elem) + 2 > PATH_MAX)\n \t\t\t\tdie (\"Too long path name: '%s/%s'\",\n \t\t\t\t\t\tbuf, last_elem);\n-\t\t\tif (len && buf[len-1] != '/')\n+\t\t\tif (len && !is_dir_sep(buf[len-1]))\n \t\t\t\tbuf[len++] = '/';\n \t\t\tstrcpy(buf + len, last_elem);\n \t\t\tfree(last_elem);\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 62eccd3..b188776 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -297,6 +297,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format\n \n #define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')\n #define is_dir_sep(c) ((c) == '/' || (c) == '\\\\')\n+static inline char *mingw_find_last_dir_sep(const char *path)\n+{\n+\tchar *ret = NULL;\n+\tfor (; *path; ++path)\n+\t\tif (is_dir_sep(*path))\n+\t\t\tret = (char *)path;\n+\treturn ret;\n+}\n+#define find_last_dir_sep mingw_find_last_dir_sep\n #define PATH_SEP ';'\n #define PRIuMAX \"I64u\"\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 40498b3..08d58f1 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -215,6 +215,10 @@ extern char *gitbasename(char *);\n #define is_dir_sep(c) ((c) == '/')\n #endif\n \n+#ifndef find_last_dir_sep\n+#define find_last_dir_sep(path) strrchr(path, '/')\n+#endif\n+\n #if __HP_cc >= 61000\n #define NORETURN __attribute__((noreturn))\n #define NORETURN_PTR\n-- \n1.7.5.3.3.g435ff\n"},{"id":"168880","messageId":"1306512040-1468-4-git-send-email-kusmabite@gmail.com","threadId":"27485","inReplyTo":"1306512040-1468-1-git-send-email-kusmabite@gmail.com","subject":"[PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-05-27T16:00:40Z","receivedAt":"2011-05-27T16:00:40Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"If someone manage to create a repo with a 'C:' entry in the\nroot-tree, files can be written outside of the working-dir. This\nopens up a can-of-worms of exploits.\n\nFix it by explicitly checking for a dos drive prefix when verifying\na paht. While we're at it, make sure that paths beginning with '\\' is\nconsidered absolute as well.\n\nNoticed-by: Theo Niessink \nSigned-off-by: Erik Faye-Lund \n---\n read-cache.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex f38471c..68faa51 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -753,11 +753,14 @@ int verify_path(const char *path)\n {\n \tchar c;\n \n+\tif (has_dos_drive_prefix(path))\n+\t\treturn 0;\n+\n \tgoto inside;\n \tfor (;;) {\n \t\tif (!c)\n \t\t\treturn 1;\n-\t\tif (c == '/') {\n+\t\tif (is_dir_sep(c)) {\n inside:\n \t\t\tc = *path++;\n \t\t\tswitch (c) {\n-- \n1.7.5.3.3.g435ff\n"},{"id":"168885","messageId":"7vr57krppq.fsf@alter.siamese.dyndns.org","threadId":"27485","inReplyTo":"1306512040-1468-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH maint 0/3] do not write files outside of work-dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-27T17:57:37Z","receivedAt":"2011-05-27T17:57:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,\n> where cloning an evil repository can arbitrarily overwrite files outside\n> the repository. Since many Windows users run as administrators, this can\n> be used for very nasty purposes.\n\nWhich of my integration branches do msysGit/Git for Windows folks base\ntheir releases these days? I could carry this through the regular \"next to\nmaster and then sometime later to maint\" schedule, but if you are not\nusing maint and basing primarily on master then I'd rather skip the \"and\nthen sometime later to maint\" part.\n"},{"id":"168887","messageId":"alpine.DEB.1.00.1105272007500.16250@s15462909.onlinehome-server.info","threadId":"27485","inReplyTo":"7vr57krppq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH maint 0/3] do not write files outside of work-dir","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2011-05-27T18:09:02Z","receivedAt":"2011-05-27T18:09:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 27 May 2011, Junio C Hamano wrote:\n\n> Erik Faye-Lund writes:\n> \n> > Theo Niessink has uncovered a serious sercurity issue in Git for \n> > Windows, where cloning an evil repository can arbitrarily overwrite \n> > files outside the repository. Since many Windows users run as \n> > administrators, this can be used for very nasty purposes.\n> \n> Which of my integration branches do msysGit/Git for Windows folks base \n> their releases these days? I could carry this through the regular \"next \n> to master and then sometime later to maint\" schedule, but if you are not \n> using maint and basing primarily on master then I'd rather skip the \"and \n> then sometime later to maint\" part.\n\nWe follow 'next'.\n\n[Cc:ing the msysGit list, as I don't know whether Pat or Sebastian follow \ngit@vger]\n\nThanks,\nJohannes\n"},{"id":"168890","messageId":"4DDFF473.7030104@kdbg.org","threadId":"27485","inReplyTo":"1306512040-1468-4-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-05-27T18:58:59Z","receivedAt":"2011-05-27T18:58:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 27.05.2011 18:00, schrieb Erik Faye-Lund:\n> If someone manage to create a repo with a 'C:' entry in the\n> root-tree, files can be written outside of the working-dir. This\n> opens up a can-of-worms of exploits.\n> \n> Fix it by explicitly checking for a dos drive prefix when verifying\n> a paht. While we're at it, make sure that paths beginning with '\\' is\n> considered absolute as well.\n\nI think we do agree that the only way to avoid the security breach is to\ncheck a path before it is used to write a file. In practice, it means to\ndisallow paths in the top-most level of the index that are two\ncharacters long and are letter-colon.\n\nIMHO, it is pointless to avoid that an evil path enters the repository,\nbecause there are so many and a few more ways to create an evil repository.\n\n> diff --git a/read-cache.c b/read-cache.c\n> index f38471c..68faa51 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -753,11 +753,14 @@ int verify_path(const char *path)\n> {\n> \tchar c;\n> \n> +\tif (has_dos_drive_prefix(path))\n> +\t\treturn 0;\n> +\n\nIsn't verify_path used to avoid that a bogus path enters the index? (I\ndon't know, I'm not familiar with this infrastructure.)\n\n> \tgoto inside;\n> \tfor (;;) {\n> \t\tif (!c)\n> \t\t\treturn 1;\n> -\t\tif (c == '/') {\n> +\t\tif (is_dir_sep(c)) {\n> inside:\n\nAnd if so, at this point, all backslashes should have been converted to\nforward-slashes already. If not, then this would just paper over the\nreal bug.\n\n> \t\t\tc = *path++;\n> \t\t\tswitch (c) {\n\n-- Hannes\n"},{"id":"168892","messageId":"7v7h9crm1z.fsf@alter.siamese.dyndns.org","threadId":"27485","inReplyTo":"alpine.DEB.1.00.1105272007500.16250@s15462909.onlinehome-server.info","subject":"Re: [PATCH maint 0/3] do not write files outside of work-dir","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-27T19:16:40Z","receivedAt":"2011-05-27T19:16:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Which of my integration branches do msysGit/Git for Windows folks base \n>> their releases these days? I could carry this through the regular \"next \n>> to master and then sometime later to maint\" schedule, but if you are not \n>> using maint and basing primarily on master then I'd rather skip the \"and \n>> then sometime later to maint\" part.\n>\n> We follow 'next'.\n>\n> [Cc:ing the msysGit list, as I don't know whether Pat or Sebastian follow \n> git@vger]\n\nThanks.\n\nThen my preference would be to queue this to \"next\", wait for msysGit to\ncut a release based on that, and then graduate it to \"master\" on my side.\n"},{"id":"168984","messageId":"BANLkTikdeq7cuhi0uo7Q6wqDJK3nxjmP-g@mail.gmail.com","threadId":"27485","inReplyTo":"4DDFF473.7030104@kdbg.org","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-05-30T09:32:37Z","receivedAt":"2011-05-30T09:32:37Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Fri, May 27, 2011 at 8:58 PM, Johannes Sixt wrote:\n> Am 27.05.2011 18:00, schrieb Erik Faye-Lund:\n>> If someone manage to create a repo with a 'C:' entry in the\n>> root-tree, files can be written outside of the working-dir. This\n>> opens up a can-of-worms of exploits.\n>>\n>> Fix it by explicitly checking for a dos drive prefix when verifying\n>> a paht. While we're at it, make sure that paths beginning with '\\' is\n>> considered absolute as well.\n>\n> I think we do agree that the only way to avoid the security breach is to\n> check a path before it is used to write a file. In practice, it means to\n> disallow paths in the top-most level of the index that are two\n> characters long and are letter-colon.\n>\n> IMHO, it is pointless to avoid that an evil path enters the repository,\n> because there are so many and a few more ways to create an evil repository.\n>\n\nYes, but this patch doesn't prevent that; it prevents an evil path\nfrom entering the index and from being checked out if the index is\nevil.\n\n>> diff --git a/read-cache.c b/read-cache.c\n>> index f38471c..68faa51 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -753,11 +753,14 @@ int verify_path(const char *path)\n>>  {\n>>       char c;\n>>\n>> +     if (has_dos_drive_prefix(path))\n>> +             return 0;\n>> +\n>\n> Isn't verify_path used to avoid that a bogus path enters the index? (I\n> don't know, I'm not familiar with this infrastructure.)\n>\n\nYes, it's being used to do that. But it's also being used when reading\nthe index into memory, which is \"the good stuf\" for our purposes.\n\nThis is the same guard which makes Git on Linux bard on an index\ncontaining paths like \"/tmp/foo\"\n\n>>       goto inside;\n>>       for (;;) {\n>>               if (!c)\n>>                       return 1;\n>> -             if (c == '/') {\n>> +             if (is_dir_sep(c)) {\n>>  inside:\n>\n> And if so, at this point, all backslashes should have been converted to\n> forward-slashes already. If not, then this would just paper over the\n> real bug.\n\nSHOULD, yes. But we could have an evil tree/index which doesn't, and\nthis if intended to make sure we reject such paths.\n\nSo I don't see how this is papering over the bug; this IS the bug (as\nfar as I can tell).\n\nBut I think I might have been a bit too care-less; I didn't fix the\nswitch-case to check for multiple backslashes on Windows. It's not\nimmediately obvious if this is needed or not, but I don't think it can\ncause harm; we should never have created an index like that anyway.\n\nSo something like this on top, perhaps?\n\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 68faa51..9367349 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -763,15 +763,11 @@ int verify_path(const char *path)\n \t\tif (is_dir_sep(c)) {\n inside:\n \t\t\tc = *path++;\n-\t\t\tswitch (c) {\n-\t\t\tdefault:\n-\t\t\t\tcontinue;\n-\t\t\tcase '/': case '\\0':\n-\t\t\t\tbreak;\n-\t\t\tcase '.':\n+\t\t\tif (c == '.') {\n \t\t\t\tif (verify_dotfile(path))\n \t\t\t\t\tcontinue;\n-\t\t\t}\n+\t\t\t} else if (!is_dir_sep(c) && c != '\\0')\n+\t\t\t\tcontinue;\n \t\t\treturn 0;\n \t\t}\n \t\tc = *path++;\n"},{"id":"168987","messageId":"C8718F35FD1A4C3C84A4D353D27621E0@martinic.local","threadId":"27485","inReplyTo":"BANLkTikdeq7cuhi0uo7Q6wqDJK3nxjmP-g@mail.gmail.com","subject":"RE: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Theo Niessink","fromEmail":"theo@taletn.com","sentAt":"2011-05-30T10:58:59Z","receivedAt":"2011-05-30T10:58:59Z","isPatch":true,"sender":{"key":"theo@taletn.com","avatar":"https://avatars.githubusercontent.com/u/5729397?v=4"},"body":"Erik Faye-Lund wrote:\n> But I think I might have been a bit too care-less; I didn't fix the\n> switch-case to check for multiple backslashes on Windows. It's not\n> immediately obvious if this is needed or not, but I don't think it can\n> cause harm; we should never have created an index like that anyway.\n> \n> So something like this on top, perhaps?\n\nNitpick: If you already know that c != '\\0' and !is_dir_sep(c), then why do\ncontinue? It will check for '\\0' and is_dir_sep(c) again, but you already\nknow that both ifs will be false. So you could just as easy jump straight to\nc = *path++, which IMHO also makes the code easier to follow:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 68faa51..089cd3e 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -763,17 +763,15 @@ int verify_path(const char *path)\n \t\tif (is_dir_sep(c)) {\n inside:\n \t\t\tc = *path++;\n-\t\t\tswitch (c) {\n-\t\t\tdefault:\n-\t\t\t\tcontinue;\n-\t\t\tcase '/': case '\\0':\n-\t\t\t\tbreak;\n-\t\t\tcase '.':\n+\t\t\tif (c == '.') {\n+\t\t\t\t\n \t\t\t\tif (verify_dotfile(path))\n \t\t\t\t\tcontinue;\n-\t\t\t}\n+\t\t\t} else if (!is_dir_sep(c) && c != '\\0')\n+\t\t\t\tgoto next;\n \t\t\treturn 0;\n \t\t}\n+next:\n \t\tc = *path++;\n \t}\n }\n"},{"id":"168988","messageId":"BANLkTi=o6p=E4bM+CG77yKrFFvQ8sBS07g@mail.gmail.com","threadId":"27485","inReplyTo":"C8718F35FD1A4C3C84A4D353D27621E0@martinic.local","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-05-30T11:17:56Z","receivedAt":"2011-05-30T11:17:56Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Mon, May 30, 2011 at 12:58 PM, Theo Niessink wrote:\n> Erik Faye-Lund wrote:\n>> But I think I might have been a bit too care-less; I didn't fix the\n>> switch-case to check for multiple backslashes on Windows. It's not\n>> immediately obvious if this is needed or not, but I don't think it can\n>> cause harm; we should never have created an index like that anyway.\n>>\n>> So something like this on top, perhaps?\n>\n> Nitpick: If you already know that c != '\\0' and !is_dir_sep(c), then why do\n> continue? It will check for '\\0' and is_dir_sep(c) again, but you already\n> know that both ifs will be false. So you could just as easy jump straight to\n> c = *path++, which IMHO also makes the code easier to follow:\n\nVery good point, thanks for noticing. I just rewrote the logic from\nswitch/case to if/else, but with the rewrite these redundant compares\nbecame more obvious. I think your version is better, indeed.\n"},{"id":"169025","messageId":"4DE3FCB9.1010401@kdbg.org","threadId":"27485","inReplyTo":"BANLkTikdeq7cuhi0uo7Q6wqDJK3nxjmP-g@mail.gmail.com","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-05-30T20:23:21Z","receivedAt":"2011-05-30T20:23:21Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.05.2011 11:32, schrieb Erik Faye-Lund:\n> On Fri, May 27, 2011 at 8:58 PM, Johannes Sixt wrote:\n>> Am 27.05.2011 18:00, schrieb Erik Faye-Lund:\n>>> If someone manage to create a repo with a 'C:' entry in the\n>>> root-tree, files can be written outside of the working-dir. This\n>>> opens up a can-of-worms of exploits.\n>>>\n>>> Fix it by explicitly checking for a dos drive prefix when verifying\n>>> a paht. While we're at it, make sure that paths beginning with '\\' is\n>>> considered absolute as well.\n>>\n>> I think we do agree that the only way to avoid the security breach is to\n>> check a path before it is used to write a file. In practice, it means to\n>> disallow paths in the top-most level of the index that are two\n>> characters long and are letter-colon.\n>>\n>> IMHO, it is pointless to avoid that an evil path enters the repository,\n>> because there are so many and a few more ways to create an evil repository.\n>>\n> \n> Yes, but this patch doesn't prevent that; it prevents an evil path\n> from entering the index and from being checked out if the index is\n> evil.\n> \n>>> diff --git a/read-cache.c b/read-cache.c\n>>> index f38471c..68faa51 100644\n>>> --- a/read-cache.c\n>>> +++ b/read-cache.c\n>>> @@ -753,11 +753,14 @@ int verify_path(const char *path)\n>>> {\n>>> char c;\n>>>\n>>> + if (has_dos_drive_prefix(path))\n>>> + return 0;\n>>> +\n>>\n>> Isn't verify_path used to avoid that a bogus path enters the index? (I\n>> don't know, I'm not familiar with this infrastructure.)\n>>\n> \n> Yes, it's being used to do that. But it's also being used when reading\n> the index into memory, which is \"the good stuf\" for our purposes.\n\nOK, I agree with the changes proposed in this patch. git reset and git\ncheckout go through this function via unpack_trees(). Are there other\nways to write a file, e.g., in merge-recursive?\n\n-- Hannes\n"},{"id":"169094","messageId":"20110601041439.GH29958@ece.pdx.edu","threadId":"27485","inReplyTo":"1306512040-1468-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH maint 0/3] do not write files outside of work-dir","fromName":"Tait","fromEmail":"git.git@t41t.com","sentAt":"2011-06-01T04:14:39Z","receivedAt":"2011-06-01T04:14:39Z","isPatch":true,"sender":{"key":"git.git@t41t.com","avatar":null},"body":"> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,\n> where cloning an evil repository can arbitrarily overwrite files outside\n> the repository...\n\nFilenames starting with C: are not necessarily absolute. Consider\n\"c:foo.txt\" where c: is the current directory on drive C, or\n\"c:stream1\" where c is a single-letter filename in the current directory\nwith an alternate data stream such as would be shown by dir /r. The\nhas_dos_drive_prefix check is overly broad. Maybe this is intentional and\njust needs to be documented. Absolute paths like \\\\localhost\\C$\\file.txt\nand \\\\?\\C:\\file.txt do seem to be caught, because they start with '\\'.\n\nMicrosoft says[1] a path is relative unless:\n - it begins with \"\\\\\"\n - it begins with a disk designator followed by a directory separator\n - it begins with a single \"\\\"\n\nOn that basis, has_dos_drive_prefix(path) should be:\n isalpha(*(path)) && (path)[1] == ':' && is_dir_sep((path)[2])\n\nHowever, there are also paths within the NT namespace (as opposed to the\nWin32 namespace, [1] again) that might be considered absolute, or at least\nto which git should not try to write. Examples would be PRN, CONOUT$, AUX,\netc. These will not be caught by the current form of has_dos_drive_prefix,\nif that is even the right place to catch them. I think the QueryDosDevice\nfunction (given the part of the path up to the first directory separator,\nif one is present [2]) would detect them, and logical drive mappings as\nwell. However, QueryDosDevice seems to also include many things that are\nnot worthy of concern, like (on my computer) \"DISPLAY5\". Does anyone know\nthe correct approach here?\n\nI gather that other programs can create names like these (with\nDefineDosDevice), so a hard-coded exception list from [1] (that being: CON,\nPRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1,\nLPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, and LPT9) might not be adequate?\n\n[1] http://msdn.microsoft.com/en-us/library/aa365247(v=vs.85).aspx\n[2] http://msdn.microsoft.com/en-us/library/aa365461(v=vs.85).aspx\n"},{"id":"169096","messageId":"4DE5DCDD.4020303@viscovery.net","threadId":"27485","inReplyTo":"20110601041439.GH29958@ece.pdx.edu","subject":"Re: [PATCH maint 0/3] do not write files outside of work-dir","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-06-01T06:31:57Z","receivedAt":"2011-06-01T06:31:57Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/1/2011 6:14, schrieb Tait:\n>> Theo Niessink has uncovered a serious sercurity issue in Git for Windows,\n>> where cloning an evil repository can arbitrarily overwrite files outside\n>> the repository...\n> \n> Filenames starting with C: are not necessarily absolute. Consider\n> \"c:foo.txt\" where c: is the current directory on drive C, or\n\nWe have a different notion of \"absolute path\". This one *is* absolute per\nour definition. See below.\n\n> \"c:stream1\" where c is a single-letter filename in the current directory\n> with an alternate data stream such as would be shown by dir /r. The\n\nOn my system, this does not create a file in the current directory with an\nalternate data stream, but - while the working directory is somewhere on\ndrive D - a file is created on drive C.\n\n> has_dos_drive_prefix check is overly broad. Maybe this is intentional and\n> just needs to be documented. Absolute paths like \\\\localhost\\C$\\file.txt\n> and \\\\?\\C:\\file.txt do seem to be caught, because they start with '\\'.\n> \n> Microsoft says[1] a path is relative unless:\n> - it begins with \"\\\\\"\n> - it begins with a disk designator followed by a directory separator\n> - it begins with a single \"\\\"\n> \n> On that basis, has_dos_drive_prefix(path) should be:\n> isalpha(*(path)) && (path)[1] == ':' && is_dir_sep((path)[2])\n\nThis is not the definition of \"relative path\" that we are interested in.\nLet $PWD be the current directory. For our purposes, a path $P is relative\nif $P and $PWD/$P designate the same file system entry. Otherwise, $P is\nan absolute path.\n\nWith this definition, the current has_dos_drive_prefix() is good enough.\n\n> However, there are also paths within the NT namespace (as opposed to the\n> Win32 namespace, [1] again) that might be considered absolute, or at least\n> to which git should not try to write. Examples would be PRN, CONOUT$, AUX,\n\nFor our purposes, these names are all relative paths. It's a case of\n\"Doctor, it hurts when I stick my finger in my eye\" if you have a\nrepository with these names.\n\nNote that git never writes to these files: It always first allocates a\ntemporary file, eg. nul.123456; but this will already fail because these\nspecial file names are forbidden even when a file extension is attached.\n\n-- Hannes\n"},{"id":"169423","messageId":"7v39jm8fs0.fsf@alter.siamese.dyndns.org","threadId":"27485","inReplyTo":"BANLkTi=o6p=E4bM+CG77yKrFFvQ8sBS07g@mail.gmail.com","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-07T03:46:39Z","receivedAt":"2011-06-07T03:46:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n>> Nitpick: If you already know that c != '\\0' and !is_dir_sep(c), then why do\n>> continue? It will check for '\\0' and is_dir_sep(c) again, but you already\n>> know that both ifs will be false. So you could just as easy jump straight to\n>> c = *path++, which IMHO also makes the code easier to follow:\n>\n> Very good point, thanks for noticing. I just rewrote the logic from\n> switch/case to if/else, but with the rewrite these redundant compares\n> became more obvious. I think your version is better, indeed.\n\nLet's not add an unnecessary goto while at it. How about this on top\ninstead?\n\n read-cache.c | 13 +++----------\n 1 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 31cf0b5..3593291 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -784,16 +784,9 @@ int verify_path(const char *path)\n \t\tif (is_dir_sep(c)) {\n inside:\n \t\t\tc = *path++;\n-\t\t\tswitch (c) {\n-\t\t\tdefault:\n-\t\t\t\tcontinue;\n-\t\t\tcase '/': case '\\0':\n-\t\t\t\tbreak;\n-\t\t\tcase '.':\n-\t\t\t\tif (verify_dotfile(path))\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\treturn 0;\n+\t\t\tif ((c == '.' && !verify_dotfile(path)) ||\n+\t\t\t is_dir_sep(c) || c == '\\0')\n+\t\t\t\treturn 0;\n \t\t}\n \t\tc = *path++;\n \t}\n"},{"id":"169431","messageId":"BANLkTi=eC37opWnN4nmC5AP66M+m5nZ86Q@mail.gmail.com","threadId":"27485","inReplyTo":"7v39jm8fs0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-06-07T10:07:49Z","receivedAt":"2011-06-07T10:07:49Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 7, 2011 at 5:46 AM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>>> Nitpick: If you already know that c != '\\0' and !is_dir_sep(c), then why do\n>>> continue? It will check for '\\0' and is_dir_sep(c) again, but you already\n>>> know that both ifs will be false. So you could just as easy jump straight to\n>>> c = *path++, which IMHO also makes the code easier to follow:\n>>\n>> Very good point, thanks for noticing. I just rewrote the logic from\n>> switch/case to if/else, but with the rewrite these redundant compares\n>> became more obvious. I think your version is better, indeed.\n>\n> Let's not add an unnecessary goto while at it.  How about this on top\n> instead?\n>\n>  read-cache.c |   13 +++----------\n>  1 files changed, 3 insertions(+), 10 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 31cf0b5..3593291 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -784,16 +784,9 @@ int verify_path(const char *path)\n>                if (is_dir_sep(c)) {\n>  inside:\n>                        c = *path++;\n> -                       switch (c) {\n> -                       default:\n> -                               continue;\n> -                       case '/': case '\\0':\n> -                               break;\n> -                       case '.':\n> -                               if (verify_dotfile(path))\n> -                                       continue;\n> -                       }\n> -                       return 0;\n> +                       if ((c == '.' && !verify_dotfile(path)) ||\n> +                           is_dir_sep(c) || c == '\\0')\n> +                               return 0;\n>                }\n>                c = *path++;\n>        }\n\nThis change the \"c == '.' && verify_dotfile(path)\"-case to eat the '.'\ncharacter without testing it against is_dir_sep, which is exactly what\nwe want. The other cases return 0, as they used to. Good.\n\nIndeed, this is a cleaner approach. Thanks!\n"},{"id":"169442","messageId":"746198706CB145F79CD13E52567F1F58@martinic.local","threadId":"27485","inReplyTo":"7v39jm8fs0.fsf@alter.siamese.dyndns.org","subject":"RE: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Theo Niessink","fromEmail":"theo@taletn.com","sentAt":"2011-06-07T11:46:13Z","receivedAt":"2011-06-07T11:46:13Z","isPatch":true,"sender":{"key":"theo@taletn.com","avatar":"https://avatars.githubusercontent.com/u/5729397?v=4"},"body":"Junio C Hamano wrote: \n> Let's not add an unnecessary goto while at it. How about this on top\n> instead?\n\nYeah, that is much cleaner indeed.\n\n- Theo\n"},{"id":"169490","messageId":"BANLkTik3ywV91UtLSR_Dydjqg=pva+b0qg@mail.gmail.com","threadId":"27485","inReplyTo":"BANLkTi=eC37opWnN4nmC5AP66M+m5nZ86Q@mail.gmail.com","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-06-07T19:09:38Z","receivedAt":"2011-06-07T19:09:38Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 7, 2011 at 12:07 PM, Erik Faye-Lund wrote:\n> On Tue, Jun 7, 2011 at 5:46 AM, Junio C Hamano wrote:\n>> Erik Faye-Lund writes:\n>>\n>>>> Nitpick: If you already know that c != '\\0' and !is_dir_sep(c), then why do\n>>>> continue? It will check for '\\0' and is_dir_sep(c) again, but you already\n>>>> know that both ifs will be false. So you could just as easy jump straight to\n>>>> c = *path++, which IMHO also makes the code easier to follow:\n>>>\n>>> Very good point, thanks for noticing. I just rewrote the logic from\n>>> switch/case to if/else, but with the rewrite these redundant compares\n>>> became more obvious. I think your version is better, indeed.\n>>\n>> Let's not add an unnecessary goto while at it.  How about this on top\n>> instead?\n>>\n>>  read-cache.c |   13 +++----------\n>>  1 files changed, 3 insertions(+), 10 deletions(-)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 31cf0b5..3593291 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -784,16 +784,9 @@ int verify_path(const char *path)\n>>                if (is_dir_sep(c)) {\n>>  inside:\n>>                        c = *path++;\n>> -                       switch (c) {\n>> -                       default:\n>> -                               continue;\n>> -                       case '/': case '\\0':\n>> -                               break;\n>> -                       case '.':\n>> -                               if (verify_dotfile(path))\n>> -                                       continue;\n>> -                       }\n>> -                       return 0;\n>> +                       if ((c == '.' && !verify_dotfile(path)) ||\n>> +                           is_dir_sep(c) || c == '\\0')\n>> +                               return 0;\n>>                }\n>>                c = *path++;\n>>        }\n>\n> This change the \"c == '.' && verify_dotfile(path)\"-case to eat the '.'\n> character without testing it against is_dir_sep, which is exactly what\n> we want. The other cases return 0, as they used to. Good.\n>\n> Indeed, this is a cleaner approach. Thanks!\n>\n\nI forgot to ask; do you want me to resend? I would imagine the commit\nmessage should be updated to reflect this change as well...\n"},{"id":"169494","messageId":"7vhb815tvm.fsf@alter.siamese.dyndns.org","threadId":"27485","inReplyTo":"BANLkTik3ywV91UtLSR_Dydjqg=pva+b0qg@mail.gmail.com","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-06-07T19:22:37Z","receivedAt":"2011-06-07T19:22:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> I forgot to ask; do you want me to resend? I would imagine the commit\n> message should be updated to reflect this change as well...\n\nHere is what I queued last night. If it looks Ok then I'll merge it down\nto 'next'.\n\n-- >8 --\nSubject: [PATCH] verify_path(): simplify check at the directory boundary\n\nWe simply want to say \"At a directory boundary, be careful with a name\nthat begins with a dot, forbid a name that ends with the boundary\ncharacter or has duplicated bounadry characters\".\n\nSigned-off-by: Junio C Hamano \n---\n read-cache.c | 13 +++----------\n 1 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 31cf0b5..3593291 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -784,16 +784,9 @@ int verify_path(const char *path)\n \t\tif (is_dir_sep(c)) {\n inside:\n \t\t\tc = *path++;\n-\t\t\tswitch (c) {\n-\t\t\tdefault:\n-\t\t\t\tcontinue;\n-\t\t\tcase '/': case '\\0':\n-\t\t\t\tbreak;\n-\t\t\tcase '.':\n-\t\t\t\tif (verify_dotfile(path))\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\treturn 0;\n+\t\t\tif ((c == '.' && !verify_dotfile(path)) ||\n+\t\t\t is_dir_sep(c) || c == '\\0')\n+\t\t\t\treturn 0;\n \t\t}\n \t\tc = *path++;\n \t}\n-- \n1.7.6.rc0.129.gbe6ef\n"},{"id":"169496","messageId":"BANLkTinoGn-t28DFQCxk+dB+tw1oAjiWng@mail.gmail.com","threadId":"27485","inReplyTo":"7vhb815tvm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] verify_path: consider dos drive prefix","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-06-07T19:32:48Z","receivedAt":"2011-06-07T19:32:48Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 7, 2011 at 9:22 PM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>> I forgot to ask; do you want me to resend? I would imagine the commit\n>> message should be updated to reflect this change as well...\n>\n> Here is what I queued last night. If it looks Ok then I'll merge it down\n> to 'next'.\n>\n> -- >8 --\n> Subject: [PATCH] verify_path(): simplify check at the directory boundary\n>\n> We simply want to say \"At a directory boundary, be careful with a name\n> that begins with a dot, forbid a name that ends with the boundary\n> character or has duplicated bounadry characters\".\n>\n> Signed-off-by: Junio C Hamano \n> ---\n>  read-cache.c |   13 +++----------\n>  1 files changed, 3 insertions(+), 10 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 31cf0b5..3593291 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -784,16 +784,9 @@ int verify_path(const char *path)\n>                if (is_dir_sep(c)) {\n>  inside:\n>                        c = *path++;\n> -                       switch (c) {\n> -                       default:\n> -                               continue;\n> -                       case '/': case '\\0':\n> -                               break;\n> -                       case '.':\n> -                               if (verify_dotfile(path))\n> -                                       continue;\n> -                       }\n> -                       return 0;\n> +                       if ((c == '.' && !verify_dotfile(path)) ||\n> +                           is_dir_sep(c) || c == '\\0')\n> +                               return 0;\n>                }\n>                c = *path++;\n>        }\n\nLooks good to me, thanks for following up on it :)\n"}]}