| Message ID | 7265e37f-fd29-3579-b840-19a1df52a59f@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mingw: avoid mktemp() in mkstemp() implementation | expand |
Am 15.07.22 um 05:58 schrieb René Scharfe: > The implementation of mkstemp() for MinGW uses mktemp() and open() > without the flag O_EXCL, which is racy. It's not a security problem > for now because all of its callers only create files within the > repository (incl. worktrees). Replace it with a call to our more > secure internal function, git_mkstemp_mode(), to prevent possible > future issues. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > compat/mingw.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/compat/mingw.c b/compat/mingw.c > index 2607de93af..b5502997e2 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template) > > int mkstemp(char *template) > { > - char *filename = mktemp(template); > - if (!filename) > - return -1; > - return open(filename, O_RDWR | O_CREAT, 0600); > + return git_mkstemp_mode(template, 0600); > } > > int gettimeofday(struct timeval *tv, void *tz) I hate such an obvious layering violation. But we have already a ton of them elsewhere (calling xmalloc from compat/, for example), so this is just a rant, not an objection. -- Hannes
Hi René, On Fri, 15 Jul 2022, René Scharfe wrote: > The implementation of mkstemp() for MinGW uses mktemp() and open() > without the flag O_EXCL, which is racy. It's not a security problem > for now because all of its callers only create files within the > repository (incl. worktrees). Replace it with a call to our more > secure internal function, git_mkstemp_mode(), to prevent possible > future issues. Excellent analysis! And thank you for noticing and fixing it! I agree with what you wrote, there is one instance where not only files inside the `.git` directory are created but also files in the worktree: `ll-merge.c` has some code to write out files in the worktree before calling an external merge driver. I believe that your assessment is correct, and that this cannot realistically be exploited (the only attack vector I came up with involved a shared repository, a symbolic link to overwrite/corrupt some files only writable by the attack's target, and some rather narrow TOCTOU window between that `mktemp()` and the `open()` call). > diff --git a/compat/mingw.c b/compat/mingw.c > index 2607de93af..b5502997e2 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template) > > int mkstemp(char *template) > { > - char *filename = mktemp(template); > - if (!filename) > - return -1; > - return open(filename, O_RDWR | O_CREAT, 0600); > + return git_mkstemp_mode(template, 0600); It is also much simpler to reason about the post image of this patch than about the pre image. ACK! Thank you so much! Dscho > } > > int gettimeofday(struct timeval *tv, void *tz) > -- > 2.37.0 >
Johannes Sixt <j6t@kdbg.org> writes: >> diff --git a/compat/mingw.c b/compat/mingw.c >> index 2607de93af..b5502997e2 100644 >> --- a/compat/mingw.c >> +++ b/compat/mingw.c >> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template) >> >> int mkstemp(char *template) >> { >> - char *filename = mktemp(template); >> - if (!filename) >> - return -1; >> - return open(filename, O_RDWR | O_CREAT, 0600); >> + return git_mkstemp_mode(template, 0600); >> } >> >> int gettimeofday(struct timeval *tv, void *tz) > > I hate such an obvious layering violation. But we have already a ton of > them elsewhere (calling xmalloc from compat/, for example), so this is > just a rant, not an objection. There is intended behaviour difference between xmalloc() and malloc() that justifies your "layering violation" observation. A low level library replacement implemented in compat/ should not call die() when it fails to obtain memory and instead report an error. But it is unclear git_mkstemp_mode() and some others in wrapper.c fall into the same category. With the posted implementation above, the end result is how the platform that lack working mkstemp() would implement it. If we were to do something to make it "cleaner", I suspect that the above implementatoin of mkstemp() can be moved out of compat/mingw.c and made reusable by _anybody_ who lack mkstemp(), just like we ship memmem() emulation for anybody who lack it in contrib/memmem.c Thanks.
Am 15.07.22 um 18:38 schrieb Junio C Hamano: > Johannes Sixt <j6t@kdbg.org> writes: > >>> diff --git a/compat/mingw.c b/compat/mingw.c >>> index 2607de93af..b5502997e2 100644 >>> --- a/compat/mingw.c >>> +++ b/compat/mingw.c >>> @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template) >>> >>> int mkstemp(char *template) >>> { >>> - char *filename = mktemp(template); >>> - if (!filename) >>> - return -1; >>> - return open(filename, O_RDWR | O_CREAT, 0600); >>> + return git_mkstemp_mode(template, 0600); >>> } >>> >>> int gettimeofday(struct timeval *tv, void *tz) >> >> I hate such an obvious layering violation. But we have already a ton of >> them elsewhere (calling xmalloc from compat/, for example), so this is >> just a rant, not an objection. > > There is intended behaviour difference between xmalloc() and > malloc() that justifies your "layering violation" observation. A > low level library replacement implemented in compat/ should not call > die() when it fails to obtain memory and instead report an error. > > But it is unclear git_mkstemp_mode() and some others in wrapper.c > fall into the same category. With the posted implementation above, > the end result is how the platform that lack working mkstemp() would > implement it. We'd have a problem if git_mkstemp_mode() depended on mkstemp(). It doesn't now, but if it is ever turned into a mkstemp() wrapper then it would fall flat on its face, but only on Windows. > If we were to do something to make it "cleaner", I suspect that the > above implementatoin of mkstemp() can be moved out of compat/mingw.c > and made reusable by _anybody_ who lack mkstemp(), just like we ship > memmem() emulation for anybody who lack it in contrib/memmem.c All other platforms seem to have a native mkstemp(). There may still be interest in using our own if the native implementation is inferior, e.g. uses a weak RNG and/or doesn't retry. We could also convert the handful of mkstemp() callers (two if we ignore reftable/) to use git_mkstemp_mode(). One of them is quite tempting for the code deduplication aspect alone (see below).. René --- wrapper.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/wrapper.c b/wrapper.c index 1c3c970080..77ca923040 100644 --- a/wrapper.c +++ b/wrapper.c @@ -439,24 +439,7 @@ FILE *fopen_or_warn(const char *path, const char *mode) int xmkstemp(char *filename_template) { - int fd; - char origtemplate[PATH_MAX]; - strlcpy(origtemplate, filename_template, sizeof(origtemplate)); - - fd = mkstemp(filename_template); - if (fd < 0) { - int saved_errno = errno; - const char *nonrelative_template; - - if (strlen(filename_template) != strlen(origtemplate)) - filename_template = origtemplate; - - nonrelative_template = absolute_path(filename_template); - errno = saved_errno; - die_errno("Unable to create temporary file '%s'", - nonrelative_template); - } - return fd; + return xmkstemp_mode(filename_template, 0600); } /* Adapted from libiberty's mkstemp.c. */ -- 2.37.0
diff --git a/compat/mingw.c b/compat/mingw.c index 2607de93af..b5502997e2 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1059,10 +1059,7 @@ char *mingw_mktemp(char *template) int mkstemp(char *template) { - char *filename = mktemp(template); - if (!filename) - return -1; - return open(filename, O_RDWR | O_CREAT, 0600); + return git_mkstemp_mode(template, 0600); } int gettimeofday(struct timeval *tv, void *tz)
The implementation of mkstemp() for MinGW uses mktemp() and open() without the flag O_EXCL, which is racy. It's not a security problem for now because all of its callers only create files within the repository (incl. worktrees). Replace it with a call to our more secure internal function, git_mkstemp_mode(), to prevent possible future issues. Signed-off-by: René Scharfe <l.s.r@web.de> --- compat/mingw.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) -- 2.37.0