[PATCH] cygwin: set write permission before unlink

[PATCH] cygwin: set write permission before unlink

[Rei Thiessen]

On Cygwin 1.7, if filesystems are mounted with the "noacl" option,
files without write permission (in particular, the temp files created
in write_loose_object()) have their "read-only" flags set in NTFS.
"read-only" files can't be unlinked.

For Cygwin, set write permissions on files that are about to be unlinked.

Signed-off-by: Rei Thiessen <rei.thiessen@gmail.com>
---
 compat/cygwin.c |    7 +++++++
 compat/cygwin.h |    3 +++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/compat/cygwin.c b/compat/cygwin.c
index b4a51b9..f40ee6d 100644
--- a/compat/cygwin.c
+++ b/compat/cygwin.c
@@ -141,3 +141,10 @@ static int cygwin_lstat_stub(const char *file_name, struct stat *buf)
 stat_fn_t cygwin_stat_fn = cygwin_stat_stub;
 stat_fn_t cygwin_lstat_fn = cygwin_lstat_stub;
 
+#undef unlink
+int cygwin_unlink(const char *pathname)
+{
+	/* "read-only" files can't be unlinked */
+	chmod(pathname, 0666);
+	return unlink(pathname);
+}
diff --git a/compat/cygwin.h b/compat/cygwin.h
index a3229f5..aa2ba3e 100644
--- a/compat/cygwin.h
+++ b/compat/cygwin.h
@@ -7,3 +7,6 @@ extern stat_fn_t cygwin_lstat_fn;
 
 #define stat(path, buf) (*cygwin_stat_fn)(path, buf)
 #define lstat(path, buf) (*cygwin_lstat_fn)(path, buf)
+
+int cygwin_unlink(const char *pathname);
+#define unlink cygwin_unlink
-- 
1.7.4.1

Re: [PATCH] cygwin: set write permission before unlink

[Christof Krüger]

> +#undef unlink
> +int cygwin_unlink(const char *pathname)
> +{
> +	/* "read-only" files can't be unlinked */
> +	chmod(pathname, 0666);
> +	return unlink(pathname);
> +}

I've no idea on how cygwin maps file permissions to the underlying
filesystem, but the above raised my attention. Doesn't chmodding the file
to 0666 open a small windows where "group" and "other" users have read
access to the file? This might be unwanted by the user and could be
exploited by some attacker listening for changes on that file.
Or am I too paranoid?

Regards,
  Chris

Re: [PATCH] cygwin: set write permission before unlink

[Rei Thiessen]

Another point of concern is that the user might have specifically set
the read-only flag only certain files
to protect them from changes/deletion, but after the patch, git can
delete them with impunity.
But then, a file's permission isn't supposed to matter to unlink() anyways.
Interestingly, Cygwin's packaged unlink command-line utility will
delete read-only files,
so Cygwin's attempt to fake permissions through the read-only flag
when a filesystem is mounted with "noacl"
seems to be inconsistent.

I'll leave this issue up to Cygwin's package maintainer for git.

Regards,
Rei


2011/6/29 Christof Krüger <git@christof-krueger.de>:
>> +#undef unlink
>> +int cygwin_unlink(const char *pathname)
>> +{
>> +     /* "read-only" files can't be unlinked */
>> +     chmod(pathname, 0666);
>> +     return unlink(pathname);
>> +}
>
> I've no idea on how cygwin maps file permissions to the underlying
> filesystem, but the above raised my attention. Doesn't chmodding the file
> to 0666 open a small windows where "group" and "other" users have read
> access to the file? This might be unwanted by the user and could be
> exploited by some attacker listening for changes on that file.
> Or am I too paranoid?
>
> Regards,
>  Chris
>
>