Re: [PATCH] repack.c: chmod +w before rename()
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 zwanzi...@googlemail.com
Signed-off-by: Torsten Bögershausen tbo...@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
signature.asc
Description: Digital signature
Re: [PATCH] repack.c: chmod +w before rename()
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
Re: [PATCH] repack.c: chmod +w before rename()
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 signature.asc Description: Digital signature
Re: [PATCH] repack.c: chmod +w before rename()
Torsten Bögershausen tbo...@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 zwanzi...@googlemail.com
Signed-off-by: Torsten Bögershausen tbo...@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
--
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: chmod +w before rename()
Johannes Schindelin johannes.schinde...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] repack.c: chmod +w before rename()
On Fri, Jan 24, 2014 at 10:49:13PM +, 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
