[RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation

[RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation

[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)
--
2.37.0

Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation

[Johannes Sixt]

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

Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation

[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.

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.

Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation

[René Scharfe]

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

Re: [RFC][PATCH] mingw: avoid mktemp() in mkstemp() implementation

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

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
>