{"thread":{"id":"58164","subject":"[RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation","startedAt":"2022-07-15T03:59:15Z","lastAt":"2022-07-16T06:31:35Z","messageCount":5,"participants":["René Scharfe","Johannes Sixt","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"459121","messageId":"7265e37f-fd29-3579-b840-19a1df52a59f@web.de","threadId":"58164","inReplyTo":null,"subject":"[RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-07-15T03:58:50Z","receivedAt":"2022-07-15T03:59:15Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The implementation of mkstemp() for MinGW uses mktemp() and open()\nwithout the flag O_EXCL, which is racy.  It's not a security problem\nfor now because all of its callers only create files within the\nrepository (incl. worktrees).  Replace it with a call to our more\nsecure internal function, git_mkstemp_mode(), to prevent possible\nfuture issues.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n compat/mingw.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 2607de93af..b5502997e2 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template)\n\n int mkstemp(char *template)\n {\n-\tchar *filename = mktemp(template);\n-\tif (!filename)\n-\t\treturn -1;\n-\treturn open(filename, O_RDWR | O_CREAT, 0600);\n+\treturn git_mkstemp_mode(template, 0600);\n }\n\n int gettimeofday(struct timeval *tv, void *tz)\n--\n2.37.0\n"},{"id":"459122","messageId":"b7fffe99-63fb-3899-6a6d-882b72b9512f@kdbg.org","threadId":"58164","inReplyTo":"7265e37f-fd29-3579-b840-19a1df52a59f@web.de","subject":"Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2022-07-15T06:00:58Z","receivedAt":"2022-07-15T06:01:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 15.07.22 um 05:58 schrieb René Scharfe:\n> The implementation of mkstemp() for MinGW uses mktemp() and open()\n> without the flag O_EXCL, which is racy.  It's not a security problem\n> for now because all of its callers only create files within the\n> repository (incl. worktrees).  Replace it with a call to our more\n> secure internal function, git_mkstemp_mode(), to prevent possible\n> future issues.\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  compat/mingw.c | 5 +----\n>  1 file changed, 1 insertion(+), 4 deletions(-)\n> \n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 2607de93af..b5502997e2 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template)\n> \n>  int mkstemp(char *template)\n>  {\n> -\tchar *filename = mktemp(template);\n> -\tif (!filename)\n> -\t\treturn -1;\n> -\treturn open(filename, O_RDWR | O_CREAT, 0600);\n> +\treturn git_mkstemp_mode(template, 0600);\n>  }\n> \n>  int gettimeofday(struct timeval *tv, void *tz)\n\nI hate such an obvious layering violation. But we have already a ton of\nthem elsewhere (calling xmalloc from compat/, for example), so this is\njust a rant, not an objection.\n\n-- Hannes\n"},{"id":"459148","messageId":"s2ooq33p-sn76-o2q9-8471-4411rp1ppq7s@tzk.qr","threadId":"58164","inReplyTo":"7265e37f-fd29-3579-b840-19a1df52a59f@web.de","subject":"Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-07-15T14:27:54Z","receivedAt":"2022-07-15T14:28:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Fri, 15 Jul 2022, René Scharfe wrote:\n\n> The implementation of mkstemp() for MinGW uses mktemp() and open()\n> without the flag O_EXCL, which is racy.  It's not a security problem\n> for now because all of its callers only create files within the\n> repository (incl. worktrees).  Replace it with a call to our more\n> secure internal function, git_mkstemp_mode(), to prevent possible\n> future issues.\n\nExcellent analysis! And thank you for noticing and fixing it!\n\nI agree with what you wrote, there is one instance where not only files\ninside the `.git` directory are created but also files in the worktree:\n`ll-merge.c` has some code to write out files in the worktree before\ncalling an external merge driver.\n\nI believe that your assessment is correct, and that this cannot\nrealistically be exploited (the only attack vector I came up with involved\na shared repository, a symbolic link to overwrite/corrupt some files only\nwritable by the attack's target, and some rather narrow TOCTOU window\nbetween that `mktemp()` and the `open()` call).\n\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 2607de93af..b5502997e2 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template)\n>\n>  int mkstemp(char *template)\n>  {\n> -\tchar *filename = mktemp(template);\n> -\tif (!filename)\n> -\t\treturn -1;\n> -\treturn open(filename, O_RDWR | O_CREAT, 0600);\n> +\treturn git_mkstemp_mode(template, 0600);\n\nIt is also much simpler to reason about the post image of this patch than\nabout the pre image.\n\nACK!\n\nThank you so much!\nDscho\n\n>  }\n>\n>  int gettimeofday(struct timeval *tv, void *tz)\n> --\n> 2.37.0\n>\n"},{"id":"459155","messageId":"xmqq1qum2uvy.fsf@gitster.g","threadId":"58164","inReplyTo":"b7fffe99-63fb-3899-6a6d-882b72b9512f@kdbg.org","subject":"Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-07-15T16:38:57Z","receivedAt":"2022-07-15T16:39:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> diff --git a/compat/mingw.c b/compat/mingw.c\n>> index 2607de93af..b5502997e2 100644\n>> --- a/compat/mingw.c\n>> +++ b/compat/mingw.c\n>> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template)\n>> \n>>  int mkstemp(char *template)\n>>  {\n>> -\tchar *filename = mktemp(template);\n>> -\tif (!filename)\n>> -\t\treturn -1;\n>> -\treturn open(filename, O_RDWR | O_CREAT, 0600);\n>> +\treturn git_mkstemp_mode(template, 0600);\n>>  }\n>> \n>>  int gettimeofday(struct timeval *tv, void *tz)\n>\n> I hate such an obvious layering violation. But we have already a ton of\n> them elsewhere (calling xmalloc from compat/, for example), so this is\n> just a rant, not an objection.\n\nThere is intended behaviour difference between xmalloc() and\nmalloc() that justifies your \"layering violation\" observation.  A\nlow level library replacement implemented in compat/ should not call\ndie() when it fails to obtain memory and instead report an error.\n\nBut it is unclear git_mkstemp_mode() and some others in wrapper.c\nfall into the same category.  With the posted implementation above,\nthe end result is how the platform that lack working mkstemp() would\nimplement it.\n\nIf we were to do something to make it \"cleaner\", I suspect that the\nabove implementatoin of mkstemp() can be moved out of compat/mingw.c\nand made reusable by _anybody_ who lack mkstemp(), just like we ship\nmemmem() emulation for anybody who lack it in contrib/memmem.c\n\nThanks.\n\n\n"},{"id":"459182","messageId":"7da25e7e-e3c6-77a9-2b8f-7407e92605b9@web.de","threadId":"58164","inReplyTo":"xmqq1qum2uvy.fsf@gitster.g","subject":"Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-07-16T06:30:47Z","receivedAt":"2022-07-16T06:31:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.07.22 um 18:38 schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n>\n>>> diff --git a/compat/mingw.c b/compat/mingw.c\n>>> index 2607de93af..b5502997e2 100644\n>>> --- a/compat/mingw.c\n>>> +++ b/compat/mingw.c\n>>> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template)\n>>>\n>>>  int mkstemp(char *template)\n>>>  {\n>>> -\tchar *filename = mktemp(template);\n>>> -\tif (!filename)\n>>> -\t\treturn -1;\n>>> -\treturn open(filename, O_RDWR | O_CREAT, 0600);\n>>> +\treturn git_mkstemp_mode(template, 0600);\n>>>  }\n>>>\n>>>  int gettimeofday(struct timeval *tv, void *tz)\n>>\n>> I hate such an obvious layering violation. But we have already a ton of\n>> them elsewhere (calling xmalloc from compat/, for example), so this is\n>> just a rant, not an objection.\n>\n> There is intended behaviour difference between xmalloc() and\n> malloc() that justifies your \"layering violation\" observation.  A\n> low level library replacement implemented in compat/ should not call\n> die() when it fails to obtain memory and instead report an error.\n>\n> But it is unclear git_mkstemp_mode() and some others in wrapper.c\n> fall into the same category.  With the posted implementation above,\n> the end result is how the platform that lack working mkstemp() would\n> implement it.\n\nWe'd have a problem if git_mkstemp_mode() depended on mkstemp().  It\ndoesn't now, but if it is ever turned into a mkstemp() wrapper then it\nwould fall flat on its face, but only on Windows.\n\n> If we were to do something to make it \"cleaner\", I suspect that the\n> above implementatoin of mkstemp() can be moved out of compat/mingw.c\n> and made reusable by _anybody_ who lack mkstemp(), just like we ship\n> memmem() emulation for anybody who lack it in contrib/memmem.c\n\nAll other platforms seem to have a native mkstemp().  There may still\nbe interest in using our own if the native implementation is inferior,\ne.g. uses a weak RNG and/or doesn't retry.\n\nWe could also convert the handful of mkstemp() callers (two if we\nignore reftable/) to use git_mkstemp_mode().  One of them is quite\ntempting for the code deduplication aspect alone (see below)..\n\nRené\n\n\n---\n wrapper.c | 19 +------------------\n 1 file changed, 1 insertion(+), 18 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 1c3c970080..77ca923040 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -439,24 +439,7 @@ FILE *fopen_or_warn(const char *path, const char *mode)\n\n int xmkstemp(char *filename_template)\n {\n-\tint fd;\n-\tchar origtemplate[PATH_MAX];\n-\tstrlcpy(origtemplate, filename_template, sizeof(origtemplate));\n-\n-\tfd = mkstemp(filename_template);\n-\tif (fd < 0) {\n-\t\tint saved_errno = errno;\n-\t\tconst char *nonrelative_template;\n-\n-\t\tif (strlen(filename_template) != strlen(origtemplate))\n-\t\t\tfilename_template = origtemplate;\n-\n-\t\tnonrelative_template = absolute_path(filename_template);\n-\t\terrno = saved_errno;\n-\t\tdie_errno(\"Unable to create temporary file '%s'\",\n-\t\t\tnonrelative_template);\n-\t}\n-\treturn fd;\n+\treturn xmkstemp_mode(filename_template, 0600);\n }\n\n /* Adapted from libiberty's mkstemp.c. */\n--\n2.37.0\n\n"}]}