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
>
