threads / patch / 27726

patchcygwin: set write permission before unlink

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

## tl;dr

3 messages between Jun 29, 2011 and Jun 29, 2011. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Rei Thiessen· Jun 29, 2011, 07:18 UTC · lore

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(-)

Show changes to 2 files +10 −0

compat/cygwin.c, compat/cygwin.h

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

Christof Krüger· Jun 29, 2011, 14:31 UTC · re: Rei Thiessen · lore

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

Show 7 quoted lines

> +#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

Rei Thiessen· Jun 29, 2011, 18:48 UTC · re: Christof Krüger · lore

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

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>:

Show 19 quoted lines

>> +#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
>
>