[PATCH] repack.c: chmod +w before rename()

[PATCH] repack.c: chmod +w before rename()

[Torsten Bögershausen]

commit a1bbc6c0 "repack: rewrite the shell script in C" introduced
a possible regression, when a Git repo is located on a Windows network share.

When git gc is called on an already packed repository, it could fail like this:
"fatal: renaming '.git/objects/pack/.tmp-xyz.pack' failed: Permission denied"

Temporary *.pack and *.idx files remain in .git/objects/pack/

In a1bbc6c0 the rename() function replaced the "mv -f" shell command.

Obviously the -f option can also override a read-only file but
rename() can not on a network share.

Make the C-code to work similar to the shell script, by making the
files writable before calling rename().

Another solution could be to do the "chmod +x" in mingw_rename().
This may be done in another commit, because
a) It improves git gc only when Git for Windows is used
   on the client machine
b) Windows refuses to delete a file when the file is read-only.
   So setting a file to read-only under Windows is a way for a user
   to protect it from being deleted.
   Changing the behaviour of rename() globally may not be what we want.

Reported-by: Jochen Haag <zwanzig12@googlemail.com>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 builtin/repack.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/builtin/repack.c b/builtin/repack.c
index ba66c6e..033b4c2 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
 				chmod(fname_old, statbuffer.st_mode);
 			}
+			if (!stat(fname, &statbuffer)) {
+				statbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);
+				chmod(fname, statbuffer.st_mode);
+			}
 			if (rename(fname_old, fname))
 				die_errno(_("renaming '%s' failed"), fname_old);
 			free(fname);
-- 
1.9.rc0.143.g6fd479e

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] repack.c: chmod +w before rename()

[brian m. carlson]

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

On Fri, Jan 24, 2014 at 10:05:14PM +0100, Torsten Bögershausen wrote:
> commit a1bbc6c0 "repack: rewrite the shell script in C" introduced
> a possible regression, when a Git repo is located on a Windows network share.
> 
> When git gc is called on an already packed repository, it could fail like this:
> "fatal: renaming '.git/objects/pack/.tmp-xyz.pack' failed: Permission denied"
> 
> Temporary *.pack and *.idx files remain in .git/objects/pack/
> 
> In a1bbc6c0 the rename() function replaced the "mv -f" shell command.
> 
> Obviously the -f option can also override a read-only file but
> rename() can not on a network share.
> 
> Make the C-code to work similar to the shell script, by making the
> files writable before calling rename().
> 
> Another solution could be to do the "chmod +x" in mingw_rename().
> This may be done in another commit, because
> a) It improves git gc only when Git for Windows is used
>    on the client machine
> b) Windows refuses to delete a file when the file is read-only.
>    So setting a file to read-only under Windows is a way for a user
>    to protect it from being deleted.
>    Changing the behaviour of rename() globally may not be what we want.
> 
> Reported-by: Jochen Haag <zwanzig12@googlemail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  builtin/repack.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ba66c6e..033b4c2 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>  				chmod(fname_old, statbuffer.st_mode);
>  			}
> +			if (!stat(fname, &statbuffer)) {
> +				statbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);
> +				chmod(fname, statbuffer.st_mode);
> +			}

Are we sure that the file in question can never be a symlink?  Because
if it is, we'll end up changing the permissions on the wrong file.  In
general, I'm wary of changing permissions on a file to suit Windows's
rename because of the symlink issue and the security issues that can
result.  Hard links probably have the same issue.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] repack.c: chmod +w before rename()

[Johannes Schindelin]

Hi Brian,

On Fri, 24 Jan 2014, brian m. carlson wrote:

> On Fri, Jan 24, 2014 at 10:05:14PM +0100, Torsten Bögershausen wrote:
> > diff --git a/builtin/repack.c b/builtin/repack.c
> > index ba66c6e..033b4c2 100644
> > --- a/builtin/repack.c
> > +++ b/builtin/repack.c
> > @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
> >  				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
> >  				chmod(fname_old, statbuffer.st_mode);
> >  			}
> > +			if (!stat(fname, &statbuffer)) {
> > +				statbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);
> > +				chmod(fname, statbuffer.st_mode);
> > +			}
> 
> Are we sure that the file in question can never be a symlink?

On Windows: yes. Otherwise, no.

Having said that, the files in question are files generated by Git, so you
really would have to tamper with things in order to get into trouble.

> Because if it is, we'll end up changing the permissions on the wrong
> file.

In any case, I'd rather change the permissions only when the rename
failed. *And* I feel uncomfortable ignoring the return value...

> In general, I'm wary of changing permissions on a file to suit Windows's
> rename because of the symlink issue and the security issues that can
> result.

I agree on the Windows issue.

> Hard links probably have the same issue.

No, hard links have their own permissions. If you change the permissions
on a hardlink, any other hardlinks to the same content are unaffected.

Ciao,
Johannes

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] repack.c: chmod +w before rename()

[brian m. carlson]

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

On Fri, Jan 24, 2014 at 11:24:36PM +0100, Johannes Schindelin wrote:
> > In general, I'm wary of changing permissions on a file to suit Windows's
> > rename because of the symlink issue and the security issues that can
> > result.
> 
> I agree on the Windows issue.

I personally feel that if Windows needs help to change permissions for a
rename, that code should only ever be used on Windows.  Doesn't
mingw_rename automatically do this anyway, and if it doesn't, shouldn't
we put the code there instead?  Furthermore, it makes me very nervous to
make the file 666.  Isn't 644 enough?

> > Hard links probably have the same issue.
> 
> No, hard links have their own permissions. If you change the permissions
> on a hardlink, any other hardlinks to the same content are unaffected.

Not according to link(2):

  This new name may be used exactly as the old one for any operation;
  both names refer to the same file (and so have the same permissions
  and ownership) and it is impossible to tell which name was the
  "original".

My testing confirms this.

More importantly, while one might not want to symlink a pack frequently,
git clone -l does use hard links.  This means that if a local clone is
made somewhere, and then the original repository is repacked and hits
this case, the clone is now vulnerable to tampering by a malicious user
(assuming that user can read the appropriate directory).  So unless I'm
reading this wrong, this patch would cause security problems on all Unix
systems.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] repack.c: chmod +w before rename()

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Another solution could be to do the "chmod +x" in mingw_rename().
> This may be done in another commit, because
> a) It improves git gc only when Git for Windows is used
>    on the client machine
> b) Windows refuses to delete a file when the file is read-only.
>    So setting a file to read-only under Windows is a way for a user
>    to protect it from being deleted.
>    Changing the behaviour of rename() globally may not be what we want.

But doesn't this affect non Windows platforms in negative ways?  We
unconditionally run stat(2) and chmod(2) unnecessarily, and we leave
the resulting file writable when it originally may have been marked
read-only on purpose.

Also, it feels to me that doing this in mingw_rename() the right
approach in the first place.  If the user wants "git mv a b" to
rename "a" in the working tree and if that path is read-only, what
happens, and what should happen?  Does "chmod -w" on a file in the
working tree mean "I want to make sure nobody accidentally writes to
it, and also I want to protect it from being renamed or deleted"?

So perhaps mingw_rename() can be taught to

 - First try to just do rename(), and if it succeeds, happily return
   without doing anything extra.

 - If it fails, then

   - check if the failure is due to read-only-ness (optional: if you
     cannot reliably tell this from the error code, do not bother),
     and if not, return failure.

   - otherwise, stat() to grab the current permission bits, do the
     chmod(), retry the rename, then restore the permission bits
     with another chmod().  Return failure if any of these steps
     fail.

That way, it can cover any call to rename(), be it for packfiles or
a path in the working tree, and you would need to pay the overhead
of stat/chmod only when it matters, no?

> Reported-by: Jochen Haag <zwanzig12@googlemail.com>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
>  builtin/repack.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/builtin/repack.c b/builtin/repack.c
> index ba66c6e..033b4c2 100644
> --- a/builtin/repack.c
> +++ b/builtin/repack.c
> @@ -324,6 +324,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
>  				statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | S_IWOTH);
>  				chmod(fname_old, statbuffer.st_mode);
>  			}
> +			if (!stat(fname, &statbuffer)) {
> +				statbuffer.st_mode |= (S_IWUSR | S_IWGRP | S_IWOTH);
> +				chmod(fname, statbuffer.st_mode);
> +			}
>  			if (rename(fname_old, fname))
>  				die_errno(_("renaming '%s' failed"), fname_old);
>  			free(fname);
> -- 
> 1.9.rc0.143.g6fd479e
>
> -- 

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] repack.c: chmod +w before rename()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> In any case, I'd rather change the permissions only when the rename
> failed. *And* I feel uncomfortable ignoring the return value...

Good judgement I'd agree with 100%.

Thanks.

-- 
-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to msysgit@googlegroups.com
To unsubscribe from this group, send email to
msysgit+unsubscribe@googlegroups.com
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to msysgit+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] repack.c: chmod +w before rename()

[Mike Hommey]

On Fri, Jan 24, 2014 at 10:49:13PM +0000, brian m. carlson wrote:
> On Fri, Jan 24, 2014 at 11:24:36PM +0100, Johannes Schindelin wrote:
> > > In general, I'm wary of changing permissions on a file to suit Windows's
> > > rename because of the symlink issue and the security issues that can
> > > result.
> > 
> > I agree on the Windows issue.
> 
> I personally feel that if Windows needs help to change permissions for a
> rename, that code should only ever be used on Windows.  Doesn't
> mingw_rename automatically do this anyway, and if it doesn't, shouldn't
> we put the code there instead?  Furthermore, it makes me very nervous to
> make the file 666.  Isn't 644 enough?

Arguably, umask is supposed to take care of making things right. OTOH,
since it's the destination file that's the problem not the renamed file,
the equivalent to mv -f would be to unlink() that file first, not to change
its permissions. That would work properly on unix too.

Mike