Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jul 15, 2022, 14:27 UTC
- Message-ID
- <s2ooq33p-sn76-o2q9-8471-4411rp1ppq7s@tzk.qr>
- In-Reply-To
- <7265e37f-fd29-3579-b840-19a1df52a59f@web.de>
Hi René,
On Fri, 15 Jul 2022, René Scharfe wrote:
Show 6 quoted lines
> 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).
Show 13 quoted lines
> 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
Show 6 quoted lines
> } > > int gettimeofday(struct timeval *tv, void *tz) > -- > 2.37.0 >