patchrepack.c: chmod +w before rename()

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

On Fri, Jan 24, 2014 at 10:05:14PM +0100, Torsten Bögershausen wrote:

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.

Hi Brian,

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

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.

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

I agree on the Windows 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

On Fri, Jan 24, 2014 at 11:24:36PM +0100, Johannes Schindelin wrote:

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?

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.

On Fri, Jan 24, 2014 at 10:49:13PM +0000, brian m. carlson wrote:

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

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

Good judgement I'd agree with 100%.

Thanks.

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

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?